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

pull sipa wants to merge 3 commits into bitcoin:master from sipa:202401_better_block_tiebreak changing 4 files +149 −10
  1. sipa commented at 9:28 pm on January 19, 2024: member

    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:

    0graph RL
    1   a1["A"];
    2   b1["B"];
    3   c1["C"];
    4   b2["B'"];
    5   c2["C'"];
    6   c1 --> b1 --> a1;
    7   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.

  2. DrahtBot commented at 9:28 pm on January 19, 2024: contributor

    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.

  3. sipa force-pushed on Jan 19, 2024
  4. DrahtBot added the label CI failed on Jan 19, 2024
  5. DrahtBot commented at 9:31 pm on January 19, 2024: contributor

    🚧 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

  6. sipa force-pushed on Jan 20, 2024
  7. log: avoid division by zero in header sync progress log 4294ef7e96
  8. Use earliest-accepteability as block work tie breaker eef4e44cae
  9. test: add functional test for complex reorgs 7b34a18f50
  10. sipa force-pushed on Jan 21, 2024
  11. DrahtBot removed the label CI failed on Jan 21, 2024
  12. willcl-ark added the label Validation on Jan 24, 2024
  13. mzumsande commented at 5:43 pm on February 21, 2024: contributor

    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).

  14. sr-gi commented at 4:30 pm on February 22, 2024: member

    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 (https://github.com/sr-gi/bitcoin/commit/e9631043cc42d853d633035c03a832adddfddb01) and it also passes on master

  15. sipa commented at 7:35 pm on March 12, 2024: member

    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.

  16. sipa closed this on Mar 12, 2024

  17. sipa added the label Up for grabs on Mar 12, 2024
  18. in src/node/blockstorage.cpp:159 in eef4e44cae outdated
    155@@ -156,14 +156,42 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex* pa, const CBlockIn
    156     if (pa->nChainWork > pb->nChainWork) return false;
    157     if (pa->nChainWork < pb->nChainWork) return true;
    158 
    159-    // ... then by earliest time received, ...
    


    mzumsande commented at 7:39 pm on March 12, 2024:
    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.
  19. maflcko removed the label Up for grabs on Mar 12, 2024
  20. maflcko commented at 8:13 pm on March 12, 2024: member
    Picked up in #29640
  21. ryanofsky commented at 2:41 am on March 13, 2024: contributor

    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

  22. mzumsande commented at 11:00 am on March 13, 2024: contributor

    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.

  23. sipa commented at 11:15 am on March 13, 2024: member

    @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.

  24. sr-gi commented at 12:45 pm on March 13, 2024: member

    @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 https://github.com/sr-gi/bitcoin/commit/51af76eaefa027fc958d53f1c3619130e633159a, 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)


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me