assumeutxo, blockstorage: Prevent core dump on invalid hash #28698

pull pablomartin4btc wants to merge 2 commits into bitcoin:master from pablomartin4btc:assumeutxo-safer-exit-on-init-core-dumped changing 2 files +25 −1
  1. pablomartin4btc commented at 10:41 pm on October 20, 2023: member

    While reviewing #27596 (ran loadtxoutset in mainnet before m_assumeutxo_data is empty as currently in master - back to 1b1d711), got a core dumped, so it seems there’s a potential issue if new releases ever remove snapshot details or a semi-experienced user performs a loadtxoutset on a different “customised” binary version (not sure if this is a real use case).

    02023-10-18T17:42:52Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
1node/blockstorage.cpp:390 LoadBlockIndex: Assertion `GetParams().AssumeutxoForBlockhash(*snapshot_blockhash)' failed.
2Aborted (core dumped)

     0/src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
 1{
 2  "headers": 813097,
 3  "chainstates": [
 4    {
 5      "blocks": 368249,
 6      "bestblockhash": "00000000000000000b7a08224a1cb00d337100ba7a46c03d04b2c2d8964efc37",
 7      "difficulty": 52278304845.59168,
 8      "verificationprogress": 0.086288278873286,
 9      "coins_db_cache_bytes": 7969177,
10      "coins_tip_cache_bytes": 14908338995,
11      "validated": true
12    },
13    {
14      "blocks": 813097,
15      "bestblockhash": "0000000000000000000270c9fdce7b17db64cca91f90106964b58e33a4d91089",
16      "difficulty": 61030681983175.59,
17      "verificationprogress": 0.999997140098457,
18      "coins_db_cache_bytes": 419430,
19      "coins_tip_cache_bytes": 784649420,
20      "snapshot_blockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
21      "validated": false
22    }
23  ]
24}
    1. Perform a loadtxoutset in mainnet on compiled bitcoind adding the block hash from Sjors’s commit.
    2. Once step 1 finishes, remove the added code from step 1 and compile again or just compile master without any changes on top.
    3. Run bitcoind, soon it’ll crash with:
     02023-10-18T17:42:52Z [init] init message: Loading block index…
 12023-10-18T17:42:52Z [init] Assuming ancestors of block 00000000000000000001a0a448d6cf2546b06801389cc030b2b18c6491266815 have valid signatures.
 22023-10-18T17:42:52Z [init] Setting nMinimumChainWork=000000000000000000000000000000000000000052b2559353df4117b7348b64
 32023-10-18T17:42:52Z [init] Prune configured to target 3000 MiB on disk for block and undo files.
 42023-10-18T17:42:52Z [init] [snapshot] detected active snapshot chainstate (/tmp/.test_utxo_2/chainstate_snapshot) - loading
 52023-10-18T17:42:52Z [init] [snapshot] switching active chainstate to Chainstate [snapshot] @ height -1 (null)
 62023-10-18T17:42:52Z [init] Opening LevelDB in /tmp/.test_utxo_2/blocks/index
 72023-10-18T17:42:52Z [init] Opened LevelDB successfully
 82023-10-18T17:42:52Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
 9node/blockstorage.cpp:390 LoadBlockIndex: Assertion `GetParams().AssumeutxoForBlockhash(*snapshot_blockhash)' failed.
10Aborted (core dumped)

     02023-10-20T15:49:12Z [init] init message: Loading block index…
 12023-10-20T15:49:12Z [init] Assuming ancestors of block 00000000000000000001a0a448d6cf2546b06801389cc030b2b18c6491266815 have valid signatures.
 22023-10-20T15:49:12Z [init] Setting nMinimumChainWork=000000000000000000000000000000000000000052b2559353df4117b7348b64
 32023-10-20T15:49:12Z [init] Prune configured to target 3000 MiB on disk for block and undo files.
 42023-10-20T15:49:12Z [init] [snapshot] detected active snapshot chainstate (/tmp/.test_utxo_2/chainstate_snapshot) - loading
 52023-10-20T15:49:12Z [init] [snapshot] switching active chainstate to Chainstate [snapshot] @ height -1 (null)
 62023-10-20T15:49:12Z [init] Opening LevelDB in /tmp/.test_utxo_2/blocks/index
 72023-10-20T15:49:12Z [init] Opened LevelDB successfully
 82023-10-20T15:49:12Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
 92023-10-20T15:49:13Z [init] *** Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
102023-10-20T15:49:13Z [init] Error: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
11Error: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
122023-10-20T15:49:13Z [init] Shutdown requested. Exiting.
132023-10-20T15:49:13Z [init] Shutdown: In progress...
142023-10-20T15:49:13Z [scheduler] scheduler thread exit
152023-10-20T15:49:13Z [shutoff] Flushed fee estimates to fee_estimates.dat.
162023-10-20T15:49:13Z [shutoff] Shutdown: done

     02023-10-20T21:45:58Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
 12023-10-20T21:45:59Z [init] ERROR: Assumeutxo data not found for the given blockhash '00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054'.
 22023-10-20T21:45:59Z [init] : Error loading block database.
 3Please restart with -reindex or -reindex-chainstate to recover.
 4: Error loading block database.
 5Please restart with -reindex or -reindex-chainstate to recover.
 62023-10-20T21:45:59Z [init] Aborted block database rebuild. Exiting.
 72023-10-20T21:45:59Z [init] Shutdown: In progress...
 82023-10-20T21:45:59Z [scheduler] scheduler thread exit
 92023-10-20T21:45:59Z [shutoff] Flushed fee estimates to fee_estimates.dat.
102023-10-20T21:45:59Z [shutoff] Shutdown: done

    02023-10-25T02:29:57Z [init] Using obfuscation key for /home/pablo/.test_utxo_2/regtest/blocks/index: 0000000000000000
12023-10-25T02:29:57Z [init] *** Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
22023-10-25T02:29:57Z [init] Error: A fatal internal error occurred, see debug.log for details
3Error: A fatal internal error occurred, see debug.log for details
42023-10-25T02:29:57Z [init] Shutdown requested. Exiting.
52023-10-25T02:29:57Z [init] Shutdown: In progress...
62023-10-25T02:29:57Z [scheduler] scheduler thread exit
72023-10-25T02:29:57Z [shutoff] Flushed fee estimates to fee_estimates.dat.
82023-10-25T02:29:57Z [shutoff] Shutdown: done
  2. DrahtBot commented at 10:41 pm on October 20, 2023: 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.

    Type Reviewers
    ACK theStack, ryanofsky, naumenkogs
    Stale ACK Sjors, fjahr

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

  3. DrahtBot added the label CI failed on Oct 21, 2023
  4. theStack commented at 9:07 pm on October 21, 2023: contributor

    Concept ACK

    I think in practice the most likely scenario for triggering this condition is that a user first successfully loads an UTXO snapshot with AssumeUTXO hash H in release version Y, and at some later point runs an earlier release version X on that same datadir (i.e. X < Y and Y includes H in the AssumeUTXO parameters, but X doesn’t yet). This will probably not happen too often, but nevertheless the crash should be fixed.

  5. in src/node/blockstorage.cpp:390 in caae1c935e outdated 
    386@@ -387,7 +387,13 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
387     }
388 
389     if (snapshot_blockhash) {
390-        const AssumeutxoData au_data = *Assert(GetParams().AssumeutxoForBlockhash(*snapshot_blockhash));
391+        const auto& maybe_au_data = GetParams().AssumeutxoForBlockhash(*snapshot_blockhash);
    Sjors commented at 5:36 am on October 23, 2023:

    std::optional<AssumeutxoData> maybe_au_data

    The code below relies on the fact that this an std::optional, so it seems safer to not use auto, in light of future refactors.

    pablomartin4btc commented at 10:51 pm on October 24, 2023:
    Ok, let me change it, I solved it that way because I saw how it’s managed in src/validation.cpp: https://github.com/bitcoin/bitcoin/blob/d53400e75e2a4573229dba7f1a0da88eb936811c/src/validation.cpp#L5359-L5368
    pablomartin4btc commented at 3:10 am on October 25, 2023:
    done!
  6. in src/node/blockstorage.cpp:396 in caae1c935e outdated 
    392+        if (!maybe_au_data) {
393+            BlockValidationState state_dummy;
394+            const std::string au_data_error = strprintf("Assumeutxo data not found for the given blockhash '%s'.", snapshot_blockhash->ToString());
395+            return FatalError(m_opts.notifications, state_dummy, au_data_error, _(au_data_error.c_str()));
396+        }
397+        const AssumeutxoData& au_data = *maybe_au_data;
    Sjors commented at 5:36 am on October 23, 2023:
    Could keep the assert: = *Assert(maybe_au_data)
    pablomartin4btc commented at 10:50 pm on October 24, 2023:
    ok
    pablomartin4btc commented at 3:10 am on October 25, 2023:
    done!
  7. Sjors commented at 5:39 am on October 23, 2023: member

    Concept ACK

    caae1c935e3ee8541fe065da43bfb2c3c8e57554 looks good, just two suggestions

    I haven’t tried reproducing the crash or the fix.

  8. in src/node/blockstorage.cpp:394 in caae1c935e outdated 
    386@@ -387,7 +387,13 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
387     }
388 
389     if (snapshot_blockhash) {
390-        const AssumeutxoData au_data = *Assert(GetParams().AssumeutxoForBlockhash(*snapshot_blockhash));
391+        const auto& maybe_au_data = GetParams().AssumeutxoForBlockhash(*snapshot_blockhash);
392+        if (!maybe_au_data) {
393+            BlockValidationState state_dummy;
394+            const std::string au_data_error = strprintf("Assumeutxo data not found for the given blockhash '%s'.", snapshot_blockhash->ToString());
395+            return FatalError(m_opts.notifications, state_dummy, au_data_error, _(au_data_error.c_str()));
    ryanofsky commented at 1:09 pm on October 23, 2023:

    In commit “assumeutxo, blockstorage: prevent core dump on invalid hash” (caae1c935e3ee8541fe065da43bfb2c3c8e57554)

    Would be good to drop the state_dummy variable and _(au_data_error.c_str()) translation which doesn’t really make sense, and just call m_opts.notifications->fatalError() instead of FatalError

    pablomartin4btc commented at 3:06 am on October 25, 2023:

    If i remove the translation string (the second parameter of the fatalError() function) the propagated error would be “Error: A fatal internal error occured...”:

    02023-10-25T02:29:57Z [init] Using obfuscation key for /home/pablo/.test_utxo_2/regtest/blocks/index: 0000000000000000
12023-10-25T02:29:57Z [init] *** Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
22023-10-25T02:29:57Z [init] Error: A fatal internal error occurred, see debug.log for details
3Error: A fatal internal error occurred, see debug.log for details
42023-10-25T02:29:57Z [init] Shutdown requested. Exiting.
52023-10-25T02:29:57Z [init] Shutdown: In progress...
62023-10-25T02:29:57Z [scheduler] scheduler thread exit
72023-10-25T02:29:57Z [shutoff] Flushed fee estimates to fee_estimates.dat.
82023-10-25T02:29:57Z [shutoff] Shutdown: done

    So I have to update also the expected_error value in the new test test_invalid_chainstate_scenarios added by @theStack.

    pablomartin4btc commented at 3:11 am on October 25, 2023:
    Updated it.
    pablomartin4btc commented at 0:21 am on October 26, 2023:
    Ah… I’d need to do what @theStack mentioned below.
  9. ryanofsky approved
  10. ryanofsky commented at 1:23 pm on October 23, 2023: contributor

    Code review ACK caae1c935e3ee8541fe065da43bfb2c3c8e57554

    Nice catch. It would be good to have a python test for this. A test can trigger the crash / error by just creating a chainstate_snapshot/base_blockhash file in the datadir with a random block hash.

    From the command line this is also easy to test by running:

    0mkdir ~/.bitcoin/regtest/chainstate_snapshot
1dd bs=32 count=1 if=/dev/urandom of=~/.bitcoin/regtest/chainstate_snapshot/base_blockhash
2bitcoin -regtest
  11. DrahtBot requested review from theStack on Oct 23, 2023
  12. DrahtBot requested review from Sjors on Oct 23, 2023
  13. theStack commented at 12:07 pm on October 24, 2023: contributor

    Nice catch. It would be good to have a python test for this. A test can trigger the crash / error by just creating a chainstate_snapshot/base_blockhash file in the datadir with a random block hash.

    I’ve written a test using this idea, based on this PR on commit https://github.com/theStack/bitcoin/commit/fb4eb165958448b9a7bc5aba100b2c5e2ee1ed86 (branch https://github.com/theStack/bitcoin/commits/pr28698_test_followup), feel free to include it here @pablomartin4btc.

  14. pablomartin4btc commented at 11:17 pm on October 24, 2023: member

    From the command line this is also easy to test by running:

    0dd bs=32 count=1 if=/dev/urandom of=${AU_DATADIR}/regtest/chainstate_snapshot/base_blockhash
11+0 records in
21+0 records out
332 bytes copied, 7,6874e-05 s, 416 kB/s

    0./src/bitcoind -datadir=${AU_DATADIR} -regtest
12023-10-24T23:13:19Z [init] Using obfuscation key for /home/pablo/.test_utxo_2/regtest/blocks/index: 0000000000000000
22023-10-24T23:13:19Z [init] *** Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
32023-10-24T23:13:19Z [init] Error: Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
4Error: Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
52023-10-24T23:13:19Z [init] Shutdown requested. Exiting.
62023-10-24T23:13:19Z [init] Shutdown: In progress...
72023-10-24T23:13:19Z [scheduler] scheduler thread exit
82023-10-24T23:13:19Z [shutoff] Flushed fee estimates to fee_estimates.dat.
92023-10-24T23:13:19Z [shutoff] Shutdown: done
  15. assumeutxo, blockstorage: prevent core dump on invalid hash 4a5be10b92
  16. pablomartin4btc force-pushed on Oct 25, 2023
  17. pablomartin4btc commented at 3:13 am on October 25, 2023: member

    Rebased.

    I’ve also included suggestions from @Sjors, @ryanofsky and functional test from @theStack. Thanks to all for reviewing!

  18. Sjors commented at 8:01 am on October 25, 2023: member

    ACK 3d7c103930f5625e83fbbf7e4311f7389e399003

    I tested that reverting 4a5be10b928d4ed33d223972537c1cb79163e79c breaks the new test, and shows the original Assertion.

  19. DrahtBot removed review request from Sjors on Oct 25, 2023
  20. DrahtBot requested review from ryanofsky on Oct 25, 2023
  21. DrahtBot removed the label CI failed on Oct 25, 2023
  22. in test/functional/feature_assumeutxo.py:119 in 3d7c103930 outdated 
    114+
115+        self.log.info("  - snapshot chainstate refering to a block that is not in the assumeutxo parameters")
116+        self.stop_node(0)
117+        chainstate_snapshot_path = self.nodes[0].chain_path / "chainstate_snapshot"
118+        chainstate_snapshot_path.mkdir()
119+        invalid_hash = bytes(list(range(32)))
    ryanofsky commented at 1:05 pm on October 25, 2023:

    In commit “test: add coverage for snapshot chainstate not matching AssumeUTXO parameters” (3d7c103930f5625e83fbbf7e4311f7389e399003)

    Can just say bytes(range(32)) without the list constructor

    Also technically no need to reverse the hash with [::-1] below, but maybe that’s kept intentionally to follow a convention

    maflcko commented at 1:13 pm on October 25, 2023:
    I think if you re-touch, this symbol name can be removed completely and you can just use f.write(b'z' * 32) # invalid hash (or similar) to achieve the same without bytes, list, range, and reverse.
    pablomartin4btc commented at 1:53 pm on October 25, 2023:
    Ok, I’ll do, thanks!
    maflcko commented at 2:14 pm on October 25, 2023:
    It may be best to leave this as-is, unless there is another reason to touch. Otherwise the reviewers will have to come around again.
    pablomartin4btc commented at 2:17 pm on October 25, 2023:
    Already done, next time I’ll keep that in mind.
  23. ryanofsky approved
  24. ryanofsky commented at 1:05 pm on October 25, 2023: contributor
    Code review ACK 3d7c103930f5625e83fbbf7e4311f7389e399003. New test is nice and implementation is a little cleaner
  25. fjahr commented at 1:56 pm on October 25, 2023: contributor
    Code review ACK 3d7c103930f5625e83fbbf7e4311f7389e399003
  26. test: add coverage for snapshot chainstate not matching AssumeUTXO parameters 
    Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
    811067ca1c
  27. pablomartin4btc force-pushed on Oct 25, 2023
  28. theStack approved
  29. theStack commented at 8:15 pm on October 25, 2023: contributor

    ACK 811067ca1cbbd4a697791cbe3ecd4bee19fe6193

    nit, if you have to retouch: probably it would be good to also check for the specific error message ("Assumeutxo data not found for the given blockhash...") via assert_debug_log, rather than only the generic “fatal internal error” one which goes to stderr.

  30. DrahtBot requested review from fjahr on Oct 25, 2023
  31. DrahtBot requested review from ryanofsky on Oct 25, 2023
  32. DrahtBot requested review from Sjors on Oct 25, 2023
  33. Sjors commented at 6:31 am on October 26, 2023: member
    Agree with @theStack’s suggestion, forgot to say that yesterday.
  34. ryanofsky approved
  35. ryanofsky commented at 8:54 pm on October 26, 2023: contributor

    Code review ACK 811067ca1cbbd4a697791cbe3ecd4bee19fe6193.

    Just suggested test simplifications since last review. I also agree the suggestion to check specifically for the right error message would be nice.

  36. naumenkogs commented at 10:41 am on October 27, 2023: member
    ACK 811067ca1cbbd4a697791cbe3ecd4bee19fe6193
  37. fanquake merged this on Oct 29, 2023
  38. fanquake closed this on Oct 29, 2023
  39. fanquake referenced this in commit fdb143c3fa on Oct 30, 2023
  40. fanquake referenced this in commit d3ae16fff2 on Oct 30, 2023
  41. fanquake commented at 4:44 pm on October 30, 2023: member
    Backported this to 26.x in #28754.


pablomartin4btc DrahtBot theStack Sjors ryanofsky maflcko fjahr naumenkogs fanquake


Sjors fjahr

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-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me