Choose earliest-activatable as tie breaker between equal-work chains

[sipa]

UPDATE: the concern mentioned below doesn't exist, see comments below.

Current logic

Since #3370, our logic for deciding which chain is active has been:

1. Among chains for which all transactions have been received, and are not known to contain invalid blocks...
2. Pick the one with the highest cumulative work.
3. If there are multiple such chaintips, pick the one for which the tip was received first (full block data, not just the header)

The reason for this last condition is protecting against block withholding. If an attacker manages to mine a block, quickly spread its header around the network but withhold the full block data, they can wait until a competing miner finds their block, at which point they broadcast the full block data. If rule (3) would use "earliest received header" as condition, they'd be able to retroactively make their block win.

Remaining issue

It appears to me however that strictly speaking, the tie-breaker (3) doesn't fully solve this. It is still possible to play the same withholding game, but with an extra block depth:

graph RL
   a1["A"];
   b1["B"];
   c1["C"];
   b2["B'"];
   c2["C'"];
   c1 --> b1 --> a1;
   c2 --> b2 --> a1;

The attacker mines blocks B and C in secret, broadcasts the header of B, and the full block data of C. Other miners cannot build on top of this (except empty blocks). As soon as the rest of the network catches up (by mining B' and C'), the attacker broadcasts B. With the current logic, the attacker's A-B-C chain will become the active chain.

I consider this very hard to pull off, as it requires a full block advantage already, and headers do not propagate through the network on their own. Still, it deserves addressing.

Solution

This pull requests replaces the tie-breaker (3) "earliest received tip" with "earliest activateable chain", as in: among the acceptable chains according to (2), pick the one for which the entire chain was received first.

Abstractly, the condition can be described as:

• Assign with each block a "timedata", the sorted high to low list of all timestamps at which all the blocks' ancestors were received.
• Among the (2) acceptable chaintips, pick the one which has the lowest timedata (comparing lexicographically after sorting).

In practice this is implemented by finding the maximum sequence value in each chain since the fork point between them, and then picking the one with the lowest maximum.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20673873298}

[mzumsande]

The attacker mines blocks B and C in secret, broadcasts the header of B, and the full block data of C. Other miners cannot build on top of this (except empty blocks). As soon as the rest of the network catches up (by mining B' and C'), the attacker broadcasts B. With the current logic, the attacker's A-B-C chain will become the active chain.

I don't think that would happen. Since #6010 nSequenceId is only set if the block and all of its predecessors have transactions, see here. Since the attacker never sent us the transactions of block B, C wouldn't get a nSequenceId even though we received the full block. C would only receive an nSequenceId once the full block B is received, which would then be larger than the one of C'. Therefore, the attacker's A-B-C chain wouldn't become the active chain.

I tried to run the functional test on master, and it succeeds (but didn't look at the test in detail).

[sr-gi]

I don't think that would happen. Since #6010 nSequenceId is only set if the block and all of its predecessors have transactions, see here. Since the attacker never sent us the transactions of block B, C wouldn't get a nSequenceId even though we received the full block. C would only receive an nSequenceId once the full block B is received, which would then be larger than the one of C'. Therefore, the attacker's A-B-C chain wouldn't become the active chain.

I tried to run the functional test on master, and it succeeds (but didn't look at the test in detail).

I agree with @mzumsande here. I've created a test replicating the example provided in the PR description (sr-gi@e963104) and it also passes on master

[sipa]

Thanks @mzumsande and @sr-gi. Looking more carefully at the code, I agree. Since nSequence is only set when a block and all its ancestors have their transactions received already, this concern does not exist.

I'm closing this PR, though the logging/test improvements here are perhaps still useful for someone to pick up.

[mzumsande]

If someone wants to pick this up:
"// ... then by earliest time received, ..."
in the existing code is not correct and should be something like "// ... then by earliest time activatable, ...", or a more verbose explanation.

[maflcko]

Picked up in #29640

[ryanofsky]

Since nSequence is only set when a block and all its ancestors have their transactions received already, this concern does not exist.

Not sure if it is relevant, but one limitation of nSequenceId is that it is not serialized, so when the node is shut down and restarted nSequenceId of all previously downloaded blocks is 0. I'm assuming it probably doesn't matter in this case, but would be interested to confirm

[mzumsande]

Not sure if it is relevant, but one limitation of nSequenceId is that it is not serialized, so when the node is shut down and restarted nSequenceId of all previously downloaded blocks is 0. I'm assuming it probably doesn't matter in this case, but would be interested to confirm

Yes, in that case the pointer address is used in CBlockIndexWorkComparator (probably in lack of a better tie-breaker being available). Also, if the user has a preference, they can use preciousblock rpc to overrule the automatic logic.

[sipa]

@ryanofsky Hmm, I'm not sure what would happen if there were two equal-length forks that existed at the time the software was shut down.

If there is no protection against reorging on restart in that case, my suggestion would be to, on startup, give all active-chain blocks nSequence 0 and all other ones nSequence=1, making whatever was previously active preferred.

[sr-gi]

@ryanofsky Hmm, I'm not sure what would happen if there were two equal-length forks that existed at the time the software was shut down.

If there is no protection against reorging on restart in that case, my suggestion would be to, on startup, give all active-chain blocks nSequence 0 and all other ones nSequence=1, making whatever was previously active preferred.

Looks like we may actually switch tips in this case. I wrote a test to demonstrate it at sr-gi@51af76e, I'll add it to #29640 and address it as suggested by @sipa.

Notice you may need to run the test multiple times given failures are flaky (but should always succeed once fixed)