This add a mechanism that will automatically park deep reorg and reconsider them only if enough work piles up.
Details
- Reviewers
jasonbcox Fabien - Group Reviewers
Restricted Project - Commits
- rSTAGING321b2b6363c2: Reconsider parked chains when enough works piles up
rABC321b2b6363c2: Reconsider parked chains when enough works piles up
Added integration test for the new behavior.
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- reconsiderparked
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3758 Build 5591: Bitcoin ABC Buildbot (legacy) Build 5590: arc lint + arc unit
Event Timeline
| src/validation.cpp | ||
|---|---|---|
| 2404 | This seems to undo the parking rather than simply ignoring it during initialization. Either fix to not unpark or fix the comment. | |
| src/validation.cpp | ||
|---|---|---|
| 2416 | Is there any way that this could be exploited to cause a nullptr deref? I don't think that being able to fork at gensis block + 1 is possible... However, just want to point it out. | |
| 2423 | This is really confusing to read. Suggest: CBlockIndex const *pindexExtraPow = pindexTip;
uint32 blocksForked = pindexTip->nHeight - pindexFork->nHeight;
// Limit the penality for depth 2 and 3 to a block worth of
// work to ensure we don't fork accidentaly.
switch (blocksForked) {
case 3:
pindexExtraPow = pindexExtraPow->pprev;
// FALLTHROUGH
case 2:
pindexExtraPow = pindexExtraPow->pprev;
}
arith_uint256 extraWork = pindexExtraPow->nChainWork - pindexFork->nChainWork; | |
| 2754 | I know you didn't write this, but wuuuut... Shouldn't this block just be this? nBlockReverseSequenceId = std::max(std::numeric_limits<int32_t>::min(), nBlockReverseSequenceId-1); | |
| 3664 | If I'm understanding this correctly, this means the reorg is more than one block? If so can we define "deep reorg" in the comment to clarify it. // If this is a deep reorg(i.e. more than one block), preemptively mark the chain as parked. If it has | |
| test/functional/abc-parkedchain.py | ||
| 160 | I think we should also assert that this is not true during the loop above. | |
| 168 | The comment in the code says twice as much pow Am I misreading the test in that this reorgs when you have over the same amount of work + 1 more block? | |
| test/functional/abc-parkedchain.py | ||
|---|---|---|
| 168 | So why does this pass? | |
| src/validation.cpp | ||
|---|---|---|
| 2416 | I wonder if this could also be an issue with a node running the -pruning option ? | |
| test/functional/abc-parkedchain.py | ||
|---|---|---|
| 152 | I missed this portion. Can we add a comment here.
| |
| src/validation.cpp | ||
|---|---|---|
| 2416 | Good point. | |
My first comment still needs to be addressed, as it causes significant confusion I think
| src/validation.cpp | ||
|---|---|---|
| 3657 ↗ | (On Diff #5639) | egorg -> reorg |
| src/validation.cpp | ||
|---|---|---|
| 2402 ↗ | (On Diff #5641) | teh -> the |
Help on getbestblockhash is no longer valid.
"\nReturns the hash of the best (tip) block in the " "longest blockchain.\n"
| test/functional/abc-parkedchain.py | ||
|---|---|---|
| 160 | No, you check that node.getbestblockhash is parked on parking_node You don't assert that parking_node.getbestblockhash() != node.getbestblockhash(). I am presuming though that getbestblockhash() should always be the active chain tip? | |
| 141 ↗ | (On Diff #5644) | There is a behavior change here for getbestblockhash() that is not being tested. It previously returned the most PoW tip as an invariant. Now it returns the non-parked best PoW chain. That's only implied by the fat that on line 160 you have the assertion that parking_node.getbestblockhash() == node.getbestblockhash() There is no assertion that this is NOT true anywhere else. I would strongly encourage an: assert(parking_node.getbestblockhash() != node.getbestblockhash() added on line 157 |