validation: Do less work in NeedsRedownload #31714

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202501_simpler_segwit_check changing 2 files +9 −15
  1. mzumsande commented at 9:25 pm on January 22, 2025: contributor

    NeedsRedownload exists to detect when a non-segwit client upgrades to segwit after segwit activation. In #21009, it was simplified from an upgrade algorithm to a simple check / recommendation to reindex.

    Still, iterating over hundred thousands of block indexes with each startup for this purpose doesn’t seem necessary more than 7 years after segwit activation. This PR suggests to only check the first segwit block instead. This is less thorough, but it will speed up init a little bit and the typical case of upgrading a non-segwit client that has already synced beyond the segwit height would still lead to an error. I also thought about removing NeedsRedownload() completely if reviewers prefer that alternative.

  2. DrahtBot commented at 9:25 pm on January 22, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31714.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK tdb3

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31792 (Double check all block rules in ConnectBlock, not only CheckBlock by darosior)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Validation on Jan 22, 2025
  4. validation: Do less work in NeedsRedownload
    Iterating over hundred thousands of block indexes
    with each startup doesn't seem necessary more than 7 years after segwit
    activation. Only check the first segwit block instead.
    This is less thorough, but the most typical case of upgrading a non-segwit client
    that has synced beyond the segwit height will still lead to an error.
    5c5f88b80a
  5. mzumsande force-pushed on Jan 22, 2025
  6. maflcko commented at 9:02 am on January 23, 2025: member

    This is less thorough

    Would be nice to explain this a bit and also explain the failure case and why it is safe to accept the failure.

    I presume it is less thorough, because anyone could start a non-segwit (EOL) version of Bitcoin Core to submit a (possibly segwit-invalid) block without BLOCK_OPT_WITNESS that is not the block immediately after the segwit activation height.

    However, this is fine, because it can only happen on an ancient datadir, or someone running EOL versions of Bitcoin Core today. Also, the block (if it was invalid) should normally be reorged out, once connected to the network?

    So I wonder why keep the flag at all? If is is safe/acceptable to skip it for almost all heights, then it should also be safe to skip for this one specific remaining height?

  7. sipa commented at 3:48 pm on January 23, 2025: member
    What would actually happen if we’d drop the check entirely, and you start a modern Bitcoin Core on a datadir produced by syncing say 0.13.0 (pre-segwit activation that was added in 0.13.1) or 0.12.0 past the segwit activation block, and then you run the getblock RPC on such segwit block, or are sent GETDATA P2P message?
  8. mzumsande commented at 3:55 pm on January 23, 2025: contributor

    Would be nice to explain this a bit and also explain the failure case and why it is safe to accept the failure.

    I thought that maybe it could be possible to sync beyond the segwit height with a newer client, then downgrade to an older client where segwit isn’t activated, download more blocks without BLOCK_OPT_WITNESS, and then upgrade again to a newer client. In this scenario we wouldn’t fail anymore after this PR. However, I don’t know if this ever would have worked (the pre-segwit client might have failed on startup, even with -checkblocks=0?), and even if so, this doesn’t seem like a realistic scenario we need to accommodate.

    The scenario that is still covered (someone upgrades from an old non-segwit client to a current version) seems much more realistic (even if that old client is EOL for ages, there might still be a few nodes out there according https://bitnodes.io/nodes/), and the check costs almost nothing after this PR, so I wasn’t sure about complete removal.

  9. maflcko commented at 5:05 pm on January 24, 2025: member

    However, I don’t know if this ever would have worked (the pre-segwit client might have failed on startup, even with -checkblocks=0?), and even if so, this doesn’t seem like a realistic scenario we need to accommodate.

    Why not? It should be supported to go back and forth between two releases (e.g. 13.0 and 13.1). So if there is a goal to support pre-13.1 upgrades, it seems arbitrary to exclude pre-13.1 upgrades that happened to connect a block with post-13.0.

    Also, I think checkblock shouldn’t affect this, because it only checks connections, which are based on the txid, not wtxid.

    For reference, this can also be tested: I had 13.1 download some blocks after the activation, then 13.0 started normally (even with checkblocks) and even appended blocks:

     02025-01-24 14:54:01 Bitcoin version v0.13.1
     1...
     22025-01-24 14:54:25 UpdateTip: new best=00000000000000000039ab5b80102b36c3f6b7762d1908b128c1c599586d7dfa height=482161 version=0x20000002 log2_work=86.997483 tx=249724170 date='2017-08-27 06:19:28' progress=0.575793 cache=111.0MiB(29988tx)
     3...
     42025-01-24 14:54:27 Shutdown: done
     5...
     6...
     72025-01-24 14:56:49 Bitcoin version v0.13.0
     8...
     92025-01-24 14:56:57 init message: Rewinding blocks...
    102025-01-24 14:56:59 init message: Verifying blocks...
    112025-01-24 14:56:59 Verifying last 288 blocks at level 3
    122025-01-24 14:56:59 [0%]...[10%]...[20%]...[30%]...[40%]...[50%]...[60%]...[70%]...[80%]...[90%]...[DONE].
    132025-01-24 14:57:21 No coin database inconsistencies in last 38 blocks (61549 transactions)
    14...
    152025-01-24 14:59:50 UpdateTip: new best=0000000000000000001ae96111fa32620d598408b32bdb2dd72d78c49390a9b4 height=482163 version=0x20000000 log2_work=86.997554 tx=249727638 date='2017-08-27 06:33:23' progress=0.575798 cache=23.1MiB(11490tx)
    16...
    172025-01-24 14:59:51 Shutdown: done
    

    and then you run the getblock RPC on such segwit block, or are sent GETDATA P2P message?

    I’ll try to try it, but my guess would be that everything appears to behave normally, except that the witness is stripped. Only once you put the block (or a tx from it) into a validation context, you’ll see the error.

  10. mzumsande commented at 6:47 pm on January 24, 2025: contributor

    What would actually happen if we’d drop the check entirely, and you start a modern Bitcoin Core on a datadir produced by syncing say 0.13.0 (pre-segwit activation that was added in 0.13.1) or 0.12.0 past the segwit activation block, and then you run the getblock RPC on such segwit block, or are sent GETDATA P2P message?

    I just tried this out - downloaded the binary for 0.13.0, synced beyond segwit activation height, and then tried to load the datadir with current master. What happens is that the node doesn’t start - it tells me to reindex because CCoinsViewDB::NeedsUpgrade() is true - DB_COINS was deprecated in v0.15.0 (1088b02f0ccd7358d2b7076bb9e122d59d502d02).

    So, it seems to me that we’d need to reindex anyway for an unrelated reason when upgrading from any pre-v0.15.0 node to any post -v0.15.0 node. So maybe NeedsRedownload can be be safely removed entirely for this reason?

  11. luke-jr commented at 4:17 pm on January 25, 2025: member
    Shouldn’t it be impossible for the tip block to have the flag, if the blocks between activation and the tip didn’t also have it? So checking just the tip would be better and sufficient?
  12. TheCharlatan commented at 8:15 am on January 27, 2025: contributor

    So maybe NeedsRedownload can be be safely removed entirely for this reason?

    sgtm, though I’m a bit puzzled why this was not suggested in #21009.

  13. tdb3 commented at 5:54 pm on January 27, 2025: contributor

    Synced 0.12 to ~533,000. Stopped and started a binary built from this PR branch (with NeedsRedownload modified to always return false).

    0[error] Unsupported chainstate database format found. Please restart with -reindex-chainstate. This will rebuild the chainstate database.
    1Error: Unsupported chainstate database format found. Please restart with -reindex-chainstate. This will rebuild the chainstate database.
    

    Acting as a user, followed the direction, bitcoind -reindex-chainstate. Saw no additional error.

    Ran bitcoin-cli getrawtransaction 8871e3ae8a6bfd26b2168b8152fe01d0f45c6f188219e0c6e13024ac6676f42a 2 00000000000000000006d3e05bbc1baba1b0755f2e2a9527d37b8ec64e64b31b. (from block 508000, so past segwit activation, but before the 0.12 node had stopped syncing). Saw no txinwitness in the response.

    Ran bitcoin-cli getrawtransaction c114c04ff30940d733ca5eeb84e58afc2f1ef18d07947a1e31b2f26a65b0afa0 2 00000000000000000012a6f6b4328bda1b1e58957a9ea7954c4e551fbb0e505e. (from block 539200, one added by the newer bitcoind). Saw txinwitness.

  14. mzumsande commented at 8:53 pm on January 27, 2025: contributor

    Acting as a user, followed the direction, bitcoind -reindex-chainstate. Saw no additional error.

    Good point, I saw that too in my run: -reindex-chainstate doesn’t detect this with assumevalid being enabled so that script verification isn’t done - only a full reindex will because this is checked in AcceptBlock() (ContextualCheckBlock()) at the time a block is saved to disk, but not in ConnectBlock(), which is called during reindex-chainstate. This is also explained in this code comment: https://github.com/bitcoin/bitcoin/blob/0a931a9787b196d7a620863cc143d9319ffd356d/src/validation.cpp#L2454-L2461 I think that this is an argument for keeping a basic check ( as the current version of the PR does).

    Shouldn’t it be impossible for the tip block to have the flag, if the blocks between activation and the tip didn’t also have it? So checking just the tip would be better and sufficient?

    Both options - checking block 1 after segwit vs checking the tip should report an error if a user upgrades from a pre-segwit version to a current version - I can change it to that, but why is it “better” than checking the first block after activation?

  15. tdb3 commented at 11:56 pm on January 27, 2025: contributor

    Concept ACK

    I think that this is an argument for keeping a basic check ( as the current version of the PR does).

    Yes, the current approach seems like a reasonable tradeoff if there is a significant performance benefit. If there isn’t one currently, then it might make sense (and be more conservative) to just add a comment to NeedsRedownload() so if/when there is one that the change could be made then. Although we are talking about seems like a very small number of (pre-segwit) nodes, so it might be nicer to make the change now and not have to revisit later.

    Checked performance with the blocks dir created in #31714 (comment) (synced to block ~802000, with blocks before ~533000 missing BLOCK_OPT_WITNESS, so > 269k blocks checked from tip before the first block without BLOCK_OPT_WITNESS is encountered). Node startup until error took 5 seconds on both 5c5f88b80ad783f66cd0fe314bf657e128ddfa80 and the commit before it, so a significant performance improvement wasn’t observed (at least when checking backward from tip by 269k blocks). YMMV (different hardware,etc.). I’m curious if there would be a significant performance difference in a different environment (e.g. HDD).

    I do think the PR covers the “common” cases:

    • User is running a pre-segwit node that is up to date (i.e. synced past segwit activation). User upgrades to a modern release (pre this PR), which checks from tip backwards for missing BLOCK_OPT_WITNESS). Tip lacks BLOCK_OPT_WITNESS, so NeedsRedownload() returns true. Reindex is needed.
    • User is running a pre-segwit node that is up to date. User upgrades to a future release with this PR’s change to check for BLOCK_OPT_WITNESS only at segwit activation. Same result (NeedsRedownload() returns true. Reindex is needed).
    • User is running a post-segwit node that is up to date. User upgrades to a future release with this PR’s change. NeedsRedownload() returns false (faster).

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: 2025-02-07 15:12 UTC

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