Skip to content

Commit eb15d7e

Browse files
Alexey-N-ChernyshovAlexey-N-Chernyshov
authored
fix deadlock in mpool (head_mutex and ts_branch_mutex) (#511)
Signed-off-by: Alexey Chernyshov <[email protected]>
1 parent 7825e9d commit eb15d7e

File tree

9 files changed

+72
-58
lines changed

9 files changed

+72
-58
lines changed

‎core/node/blocksync_common.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ namespace fc::sync::blocksync {
2727
using MsgIncudes = std::vector<std::vector<uint64_t>>;
2828

2929
struct TipsetBundle {
30-
// TODO use not so heap consuming containers, like small_vector
30+
// TODO(turuslan): use not so heap consuming containers, like small_vector
3131

3232
std::vector<BlockHeader> blocks;
3333

‎core/node/chain_store_impl.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ namespace fc::sync {
2828
: ipld_(std::move(ipld)),
2929
ts_load_(std::move(ts_load)),
3030
put_block_header_(std::move(put_block_header)),
31-
block_validator_(std::move(block_validator)) {
31+
block_validator_(std::move(block_validator)),
32+
head_{std::move(head)},
33+
heaviest_weight_{std::move(weight)} {
3234
assert(ipld_);
3335
assert(block_validator_);
34-
head_ = head;
35-
heaviest_weight_ = weight;
3636
}
3737

3838
outcome::result<void> ChainStoreImpl::start(
@@ -73,7 +73,7 @@ namespace fc::sync {
7373
}
7474

7575
void ChainStoreImpl::update(const Path &path, const BigInt &weight) {
76-
auto &[revert, apply]{path};
76+
const auto &[revert, apply]{path};
7777
using primitives::tipset::HeadChange;
7878
using primitives::tipset::HeadChangeType;
7979
HeadChange event;

‎core/node/sync_job.cpp

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,9 @@ namespace fc::sync {
2525
}
2626
} // namespace
2727

28-
outcome::result<TipsetCPtr> stepUp(TsLoadPtr ts_load,
29-
TsBranchPtr branch,
30-
TipsetCPtr ts) {
28+
outcome::result<TipsetCPtr> stepUp(const TsLoadPtr &ts_load,
29+
const TsBranchPtr &branch,
30+
const TipsetCPtr &ts) {
3131
if (branch->chain.rbegin()->second.key != ts->key) {
3232
OUTCOME_TRY(it, find(branch, ts->height() + 1, false));
3333
if (!it.first) {
@@ -68,6 +68,7 @@ namespace fc::sync {
6868
attached_.insert(ts_main_);
6969
}
7070

71+
// NOLINTNEXTLINE(readability-function-cognitive-complexity)
7172
void SyncJob::start(std::shared_ptr<events::Events> events) {
7273
if (events_) {
7374
log()->error("already started");
@@ -128,7 +129,7 @@ namespace fc::sync {
128129
}
129130

130131
TipsetCPtr SyncJob::getLocal(const TipsetCPtr &ts) {
131-
for (auto &block : ts->blks) {
132+
for (const auto &block : ts->blks) {
132133
if (!ipld_->contains(block.messages).value()) {
133134
return nullptr;
134135
}
@@ -145,8 +146,8 @@ namespace fc::sync {
145146

146147
void SyncJob::compactBranches() {
147148
std::pair<TsBranchPtr, size_t> longest{};
148-
for (auto &head : *ts_branches_) {
149-
if (!attached_.count(head)) {
149+
for (const auto &head : *ts_branches_) {
150+
if (attached_.count(head) == 0) {
150151
size_t length{};
151152
for (auto branch{head}; branch; branch = branch->parent) {
152153
length += branch->chain.size();
@@ -200,7 +201,7 @@ namespace fc::sync {
200201
while (true) {
201202
std::vector<TsBranchPtr> children;
202203
auto branch{insert(*ts_branches_, ts, &children).first};
203-
if (attached_.count(branch)) {
204+
if (attached_.count(branch) != 0) {
204205
auto last{attached_heaviest_.first};
205206
for (auto &child : children) {
206207
attach(child);
@@ -252,7 +253,7 @@ namespace fc::sync {
252253
}
253254
}
254255

255-
void SyncJob::updateTarget(TsBranchPtr last) {
256+
void SyncJob::updateTarget(const TsBranchPtr &last) {
256257
auto branch{attached_heaviest_.first};
257258
if (branch == last) {
258259
return;
@@ -279,8 +280,9 @@ namespace fc::sync {
279280
}
280281
}
281282

282-
void SyncJob::onInterpret(TipsetCPtr ts, const InterpreterResult &result) {
283-
auto &weight{result.weight};
283+
void SyncJob::onInterpret(const TipsetCPtr &ts,
284+
const InterpreterResult &result) {
285+
const auto &weight{result.weight};
284286
if (weight > chain_store_->getHeaviestWeight()) {
285287
auto branch{find(*ts_branches_, ts)};
286288
if (!branch.first) {
@@ -289,10 +291,10 @@ namespace fc::sync {
289291
return;
290292
}
291293
if (const auto _update{update(ts_main_, branch)}) {
292-
auto &[path, removed]{_update.value()};
293-
for (auto &branch : removed) {
294-
ts_branches_->erase(branch);
295-
attached_.erase(branch);
294+
const auto &[path, removed]{_update.value()};
295+
for (const auto &removed_branch : removed) {
296+
ts_branches_->erase(removed_branch);
297+
attached_.erase(removed_branch);
296298
}
297299
chain_store_->update(path, weight);
298300
} else {
@@ -301,7 +303,7 @@ namespace fc::sync {
301303
}
302304
}
303305

304-
bool SyncJob::checkParent(TipsetCPtr ts) {
306+
bool SyncJob::checkParent(const TipsetCPtr &ts) {
305307
if (ts->height() != 0) {
306308
if (auto _res{interpreter_cache_->tryGet(ts->getParents())}) {
307309
if (*_res) {
@@ -346,7 +348,7 @@ namespace fc::sync {
346348
auto ts{interpret_ts_};
347349
auto branch{find(*ts_branches_, ts).first};
348350
if (!checkParent(ts)) {
349-
// TODO: detach and ban branches
351+
// TODO(turuslan): detach and ban branches
350352
interpret_ts_ = nullptr;
351353
return;
352354
}
@@ -374,7 +376,7 @@ namespace fc::sync {
374376
interpret_ts_ = nullptr;
375377
}
376378
} else {
377-
// TODO: detach and ban branches
379+
// TODO(turuslan): detach and ban branches
378380
interpret_ts_ = nullptr;
379381
}
380382
}

‎core/node/sync_job.hpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,11 @@ namespace fc::sync {
5454

5555
void attach(TsBranchPtr branch);
5656

57-
void updateTarget(TsBranchPtr last);
57+
void updateTarget(const TsBranchPtr &last);
5858

59-
void onInterpret(TipsetCPtr ts, const InterpreterResult &result);
59+
void onInterpret(const TipsetCPtr &ts, const InterpreterResult &result);
6060

61-
bool checkParent(TipsetCPtr ts);
61+
bool checkParent(const TipsetCPtr &ts);
6262

6363
void interpretDequeue();
6464

@@ -82,6 +82,7 @@ namespace fc::sync {
8282
IpldPtr ipld_;
8383
TsBranches attached_;
8484
std::pair<TsBranchPtr, BigInt> attached_heaviest_;
85+
/** Tipset being interpreted at the moment. */
8586
TipsetCPtr interpret_ts_;
8687
bool interpreting_{false};
8788
IoThread thread;

‎core/primitives/tipset/chain.cpp

Lines changed: 28 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515

1616
namespace fc::primitives::tipset::chain {
1717

18-
void attach(TsBranchPtr parent, TsBranchPtr child) {
18+
void attach(const TsBranchPtr &parent, const TsBranchPtr &child) {
1919
auto bottom{child->chain.begin()};
2020
parent->lazyLoad(bottom->first);
2121
assert(*parent->chain.find(bottom->first) == *bottom);
@@ -28,7 +28,7 @@ namespace fc::primitives::tipset::chain {
2828
child->parent_key = boost::none;
2929
}
3030

31-
void detach(TsBranchPtr parent, TsBranchPtr child) {
31+
void detach(TsBranchPtr parent, const TsBranchPtr &child) {
3232
assert(child->parent == parent);
3333
child->parent = nullptr;
3434
parent->children.erase(std::find_if(
@@ -49,7 +49,7 @@ namespace fc::primitives::tipset::chain {
4949
return branch;
5050
}
5151

52-
outcome::result<TsBranchPtr> TsBranch::make(TsLoadPtr ts_load,
52+
outcome::result<TsBranchPtr> TsBranch::make(const TsLoadPtr &ts_load,
5353
const TipsetKey &key,
5454
TsBranchPtr parent) {
5555
if (parent->chain.rbegin()->second.key == key) {
@@ -85,6 +85,7 @@ namespace fc::primitives::tipset::chain {
8585
return lazy ? lazy->bottom : *chain.begin();
8686
}
8787

88+
// NOLINTNEXTLINE(readability-function-cognitive-complexity)
8889
void TsBranch::lazyLoad(ChainEpoch height) {
8990
const auto bottom{chain.begin()->first};
9091
if (height < bottom && lazy && updater) {
@@ -95,26 +96,26 @@ namespace fc::primitives::tipset::chain {
9596
auto i{updater->counts.size() - 1};
9697
auto offset{updater->count_sum};
9798
while (true) {
98-
if (counts[i]) {
99+
if (counts[i] != 0) {
99100
offset -= counts[i];
100101
++batch;
101102
if (static_cast<ChainEpoch>(min_height + i) <= height
102103
&& batch >= lazy->min_load) {
103104
break;
104105
}
105106
}
106-
if (!i) {
107+
if (i == 0) {
107108
break;
108109
}
109110
--i;
110111
}
111112
std::unique_lock lock{updater->read_mutex};
112-
updater->file_hash_read.seekg(sizeof(file::Seed)
113-
+ offset * sizeof(CbCid));
113+
updater->file_hash_read.seekg(gsl::narrow<std::streamoff>(
114+
sizeof(file::Seed) + offset * sizeof(CbCid)));
114115
std::vector<CbCid> tsk;
115116
while (i != counts.size()
116117
&& static_cast<ChainEpoch>(min_height + i) < bottom) {
117-
if (counts[i]) {
118+
if (counts[i] != 0) {
118119
tsk.resize(counts[i]);
119120
if (!common::read(updater->file_hash_read, gsl::make_span(tsk))) {
120121
outcome::raise(ERROR_TEXT("TsBranch::lazyLoad read error"));
@@ -127,7 +128,7 @@ namespace fc::primitives::tipset::chain {
127128
}
128129
}
129130

130-
outcome::result<Path> findPath(TsBranchPtr from, TsBranchIter to_it) {
131+
outcome::result<Path> findPath(const TsBranchPtr &from, TsBranchIter to_it) {
131132
Path path;
132133
auto &[revert, apply]{path};
133134
auto &[to, _to]{to_it};
@@ -157,25 +158,26 @@ namespace fc::primitives::tipset::chain {
157158
_child = branch->children.erase(_child);
158159
}
159160
while (!queue.empty()) {
160-
auto [_branch, parent]{queue.back()};
161+
auto [current_branch, parent]{queue.back()};
161162
queue.pop_back();
162163
auto _parent{parent->chain.begin()};
163164
auto _child{parent->children.begin()};
164-
while (_parent != parent->chain.end() && _branch != branch->chain.end()
165-
&& *_parent == *_branch) {
165+
while (_parent != parent->chain.end()
166+
&& current_branch != branch->chain.end()
167+
&& *_parent == *current_branch) {
166168
while (_child != parent->children.end()
167-
&& _child->first == _branch->first) {
169+
&& _child->first == current_branch->first) {
168170
if (auto child{_child->second.lock()}) {
169-
queue.emplace_back(_branch, child);
171+
queue.emplace_back(current_branch, child);
170172
child->parent = branch;
171173
}
172174
_child = parent->children.erase(_child);
173175
}
174176
++_parent;
175-
++_branch;
177+
++current_branch;
176178
}
177179
--_parent;
178-
--_branch;
180+
--current_branch;
179181
parent->chain.erase(parent->chain.begin(), _parent);
180182
branch->children.emplace(_parent->first, parent);
181183
if (parent->chain.size() <= 1) {
@@ -185,10 +187,11 @@ namespace fc::primitives::tipset::chain {
185187
return removed;
186188
}
187189

188-
outcome::result<std::vector<TsBranchPtr>> update(TsBranchPtr branch,
190+
// NOLINTNEXTLINE(readability-function-cognitive-complexity)
191+
outcome::result<std::vector<TsBranchPtr>> update(const TsBranchPtr &branch,
189192
const Path &path) {
190193
auto &from{branch->chain};
191-
auto &[revert, apply]{path};
194+
const auto &[revert, apply]{path};
192195
auto revert_to{from.find(revert.begin()->first)};
193196

194197
assert(*revert.begin() == *revert_to);
@@ -235,13 +238,13 @@ namespace fc::primitives::tipset::chain {
235238
}
236239

237240
outcome::result<std::pair<Path, std::vector<TsBranchPtr>>> update(
238-
TsBranchPtr branch, TsBranchIter to_it) {
241+
const TsBranchPtr &branch, const TsBranchIter &to_it) {
239242
OUTCOME_TRY(path, findPath(branch, to_it));
240243
OUTCOME_TRY(removed, update(branch, path));
241244
return std::make_pair(std::move(path), std::move(removed));
242245
}
243246

244-
TsBranchIter find(const TsBranches &branches, TipsetCPtr ts) {
247+
TsBranchIter find(const TsBranches &branches, const TipsetCPtr &ts) {
245248
for (auto branch : branches) {
246249
branch->lazyLoad(ts->height());
247250
auto it{branch->chain.find(ts->height())};
@@ -258,14 +261,14 @@ namespace fc::primitives::tipset::chain {
258261
}
259262

260263
TsBranchIter insert(TsBranches &branches,
261-
TipsetCPtr ts,
264+
const TipsetCPtr &ts,
262265
std::vector<TsBranchPtr> *children) {
263266
TsChain::value_type p{ts->height(),
264267
TsLazy{ts->key, 0}}; // we don't know index
265268
auto [branch, it]{find(branches, ts)};
266-
for (auto &child : branches) {
269+
for (const auto &child : branches) {
267270
if (child->parent_key && child->parent_key == ts->key) {
268-
if (children) {
271+
if (children != nullptr) {
269272
children->push_back(child);
270273
}
271274
child->chain.emplace(p);
@@ -343,12 +346,12 @@ namespace fc::primitives::tipset::chain {
343346
return it;
344347
}
345348

346-
outcome::result<BeaconEntry> latestBeacon(TsLoadPtr ts_load,
349+
outcome::result<BeaconEntry> latestBeacon(const TsLoadPtr &ts_load,
347350
TsBranchIter it) {
348351
// magic number from lotus
349352
for (auto i{0}; i < 20; ++i) {
350353
OUTCOME_TRY(ts, ts_load->lazyLoad(it.second->second));
351-
auto &beacons{ts->blks[0].beacon_entries};
354+
const auto &beacons{ts->blks[0].beacon_entries};
352355
if (!beacons.empty()) {
353356
return *beacons.rbegin();
354357
}

‎core/primitives/tipset/chain.hpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ namespace fc::primitives::tipset::chain {
2727
* @return parent if chain size is 1 else valid branch attached to parent
2828
*/
2929
static TsBranchPtr make(TsChain chain, TsBranchPtr parent = nullptr);
30-
static outcome::result<TsBranchPtr> make(TsLoadPtr ts_load,
30+
static outcome::result<TsBranchPtr> make(const TsLoadPtr &ts_load,
3131
const TipsetKey &key,
3232
TsBranchPtr parent);
3333

@@ -54,13 +54,13 @@ namespace fc::primitives::tipset::chain {
5454
using Path = std::pair<TsChain, TsChain>;
5555

5656
outcome::result<std::pair<Path, std::vector<TsBranchPtr>>> update(
57-
TsBranchPtr branch, TsBranchIter to_it);
57+
const TsBranchPtr &branch, const TsBranchIter &to_it);
5858

5959
using TsBranches = std::set<TsBranchPtr>;
6060
using TsBranchesPtr = std::shared_ptr<TsBranches>;
61-
TsBranchIter find(const TsBranches &branches, TipsetCPtr ts);
61+
TsBranchIter find(const TsBranches &branches, const TipsetCPtr &ts);
6262
TsBranchIter insert(TsBranches &branches,
63-
TipsetCPtr ts,
63+
const TipsetCPtr &ts,
6464
std::vector<TsBranchPtr> *children = {});
6565

6666
std::vector<TsBranchIter> children(TsBranchIter ts_it);
@@ -74,7 +74,8 @@ namespace fc::primitives::tipset::chain {
7474

7575
outcome::result<TsBranchIter> stepParent(TsBranchIter it);
7676

77-
outcome::result<BeaconEntry> latestBeacon(TsLoadPtr ts_load, TsBranchIter it);
77+
outcome::result<BeaconEntry> latestBeacon(const TsLoadPtr &ts_load,
78+
TsBranchIter it);
7879

7980
outcome::result<TsBranchIter> getLookbackTipSetForRound(TsBranchIter it,
8081
ChainEpoch epoch);

‎core/primitives/tipset/load.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ namespace fc::primitives::tipset {
1616
outcome::result<TipsetCPtr> TsLoadIpld::load(const TipsetKey &key) {
1717
std::vector<BlockHeader> blocks;
1818
blocks.reserve(key.cids().size());
19-
for (auto &cid : key.cids()) {
19+
for (const auto &cid : key.cids()) {
2020
OUTCOME_TRY(block, getCbor<BlockHeader>(ipld, CID{cid}));
2121
blocks.emplace_back(std::move(block));
2222
}

‎core/primitives/tipset/tipset.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ namespace fc::primitives::tipset {
149149
* @struct HeadChange represents atomic chain change
150150
*/
151151
struct HeadChange {
152-
HeadChangeType type;
152+
HeadChangeType type{};
153153
TipsetCPtr value;
154154
};
155155

0 commit comments

Comments
 (0)