Page MenuHomePhabricator

Reconsider parked chains when enough works piles up
ClosedPublic

Authored by deadalnix on Nov 3 2018, 01:40.

Details

Summary

This add a mechanism that will automatically park deep reorg and reconsider them only if enough work piles up.

Test Plan

Added integration test for the new behavior.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix changed the visibility from "Public (No Login Required)" to "Restricted Project (Project)".Nov 3 2018, 01:40
deadalnix changed the edit policy from "All Users" to "Restricted Project (Project)".
jasonbcox requested changes to this revision.Nov 3 2018, 03:18
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
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.

This revision now requires changes to proceed.Nov 3 2018, 03:18
schancel added inline comments.
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?

src/validation.cpp
2416 ↗(On Diff #5637)

Might as well protect against it.

test/functional/abc-parkedchain.py
168 ↗(On Diff #5637)

Should be "greater than 2x pow"

test/functional/abc-parkedchain.py
168 ↗(On Diff #5637)

So why does this pass?

Fabien added inline comments.
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.

  1. Add 1x total PoW worth of blocks
deadalnix added inline comments.
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.

jasonbcox requested changes to this revision.Nov 4 2018, 19:37

My first comment still needs to be addressed, as it causes significant confusion I think

src/validation.cpp
3657 ↗(On Diff #5639)

egorg -> reorg

This revision now requires changes to proceed.Nov 4 2018, 19:37

Update comments as per request

jasonbcox added inline comments.
src/validation.cpp
2402 ↗(On Diff #5641)

teh -> the

This revision is now accepted and ready to land.Nov 5 2018, 01:41

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

Relax a bit the penality under 3 blocks.

deadalnix added a child revision: Restricted Differential Revision.Nov 13 2018, 01:10
deadalnix changed the visibility from "Restricted Project (Project)" to "Public (No Login Required)".Nov 20 2018, 16:27
This revision was automatically updated to reflect the committed changes.