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 3817 Build 5707: Bitcoin ABC Buildbot (legacy) Build 5706: arc lint + arc unit
Event Timeline
src/validation.cpp | ||
---|---|---|
2404 ↗ | (On Diff #5637) | 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 ↗ | (On Diff #5637) | 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 ↗ | (On Diff #5637) | 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 ↗ | (On Diff #5637) | 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 ↗ | (On Diff #5637) | 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 ↗ | (On Diff #5637) | I think we should also assert that this is not true during the loop above. |
168 ↗ | (On Diff #5637) | 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 ↗ | (On Diff #5637) | So why does this pass? |
src/validation.cpp | ||
---|---|---|
2416 ↗ | (On Diff #5637) | I wonder if this could also be an issue with a node running the -pruning option ? |
test/functional/abc-parkedchain.py | ||
---|---|---|
152 ↗ | (On Diff #5637) | I missed this portion. Can we add a comment here.
|
src/validation.cpp | ||
---|---|---|
2416 ↗ | (On Diff #5637) | How is pindexTip supposed to have a fork point witht he other chain without any predecessor ? |
2754 ↗ | (On Diff #5637) | Absolutely not. |
test/functional/abc-parkedchain.py | ||
160 ↗ | (On Diff #5637) | We check the chain is parked. |
168 ↗ | (On Diff #5637) | Because 2x the PoW, when the fork is n block deep, means you got to produce n extra blocks. |
src/validation.cpp | ||
---|---|---|
2416 ↗ | (On Diff #5637) | 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 ↗ | (On Diff #5637) | 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 |