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

pablomartin4btc · 2023-10-20T22:41:31Z

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

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

This is also happening before IBD is completed (background validation still being performed as it can be seen in rpc getchainstates)

/src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
{
  "headers": 813097,
  "chainstates": [
    {
      "blocks": 368249,
      "bestblockhash": "00000000000000000b7a08224a1cb00d337100ba7a46c03d04b2c2d8964efc37",
      "difficulty": 52278304845.59168,
      "verificationprogress": 0.086288278873286,
      "coins_db_cache_bytes": 7969177,
      "coins_tip_cache_bytes": 14908338995,
      "validated": true
    },
    {
      "blocks": 813097,
      "bestblockhash": "0000000000000000000270c9fdce7b17db64cca91f90106964b58e33a4d91089",
      "difficulty": 61030681983175.59,
      "verificationprogress": 0.999997140098457,
      "coins_db_cache_bytes": 419430,
      "coins_tip_cache_bytes": 784649420,
      "snapshot_blockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
      "validated": false
    }
  ]
}

Steps to reproduce the core dump error and its output:

Perform a loadtxoutset in mainnet on compiled bitcoind adding the block hash from Sjors's commit.
Once step 1 finishes, remove the added code from step 1 and compile again or just compile master without any changes on top.
Run bitcoind, soon it'll crash with:

2023-10-18T17:42:52Z [init] init message: Loading block index…
2023-10-18T17:42:52Z [init] Assuming ancestors of block 00000000000000000001a0a448d6cf2546b06801389cc030b2b18c6491266815 have valid signatures.
2023-10-18T17:42:52Z [init] Setting nMinimumChainWork=000000000000000000000000000000000000000052b2559353df4117b7348b64
2023-10-18T17:42:52Z [init] Prune configured to target 3000 MiB on disk for block and undo files.
2023-10-18T17:42:52Z [init] [snapshot] detected active snapshot chainstate (/tmp/.test_utxo_2/chainstate_snapshot) - loading
2023-10-18T17:42:52Z [init] [snapshot] switching active chainstate to Chainstate [snapshot] @ height -1 (null)
2023-10-18T17:42:52Z [init] Opening LevelDB in /tmp/.test_utxo_2/blocks/index
2023-10-18T17:42:52Z [init] Opened LevelDB successfully
2023-10-18T17:42:52Z [init] Using obfuscation key for /tmp/.test_utxo_2/blocks/index: 0000000000000000
node/blockstorage.cpp:390 LoadBlockIndex: Assertion `GetParams().AssumeutxoForBlockhash(*snapshot_blockhash)' failed.
Aborted (core dumped)

After original change, error message output:

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

Alternative on error handling using return error() instead of return FatalError() used in this PR, which produces a different output and perhaps confusing:

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

Current state (including ryanofsky suggestion), after code change, error message output:

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

DrahtBot · 2023-10-20T22:41:34Z

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.

theStack · 2023-10-21T21:07:07Z

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.

Sjors

Concept ACK

caae1c9 looks good, just two suggestions

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

src/node/blockstorage.cpp

ryanofsky

Code review ACK caae1c9

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:

mkdir ~/.bitcoin/regtest/chainstate_snapshot
dd bs=32 count=1 if=/dev/urandom of=~/.bitcoin/regtest/chainstate_snapshot/base_blockhash
bitcoin -regtest

src/node/blockstorage.cpp

theStack · 2023-10-24T12:07:16Z

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 theStack@fb4eb16 (branch https://github.com/theStack/bitcoin/commits/pr28698_test_followup), feel free to include it here @pablomartin4btc.

pablomartin4btc · 2023-10-24T23:17:55Z

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

True, tested it, thanks @ryanofsky!

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

./src/bitcoind -datadir=${AU_DATADIR} -regtest
2023-10-24T23:13:19Z [init] Using obfuscation key for /home/pablo/.test_utxo_2/regtest/blocks/index: 0000000000000000
2023-10-24T23:13:19Z [init] *** Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
2023-10-24T23:13:19Z [init] Error: Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
Error: Assumeutxo data not found for the given blockhash 'f09b5835f3f8b39481f2af3257bbc2e82845552d4d2d6d31cf520fc24263ed5b'.
2023-10-24T23:13:19Z [init] Shutdown requested. Exiting.
2023-10-24T23:13:19Z [init] Shutdown: In progress...
2023-10-24T23:13:19Z [scheduler] scheduler thread exit
2023-10-24T23:13:19Z [shutoff] Flushed fee estimates to fee_estimates.dat.
2023-10-24T23:13:19Z [shutoff] Shutdown: done

pablomartin4btc · 2023-10-25T03:13:51Z

Rebased.

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

Sjors · 2023-10-25T08:01:58Z

ACK 3d7c103

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

ryanofsky

Code review ACK 3d7c103. New test is nice and implementation is a little cleaner

test/functional/feature_assumeutxo.py

fjahr · 2023-10-25T13:56:04Z

Code review ACK 3d7c103

…rameters Co-authored-by: Russell Yanofsky <russ@yanofsky.org> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

theStack

ACK 811067c

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.

Sjors · 2023-10-26T06:31:37Z

Agree with @theStack's suggestion, forgot to say that yesterday.

ryanofsky

Code review ACK 811067c.

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

naumenkogs · 2023-10-27T10:41:55Z

ACK 811067c

Github-Pull: bitcoin#28698 Rebased-from: 4a5be10

…rameters Co-authored-by: Russell Yanofsky <russ@yanofsky.org> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com> Github-Pull: bitcoin#28698 Reabsed-From: 811067c

fanquake · 2023-10-30T16:44:38Z

Backported this to 26.x in #28754.

Github-Pull: bitcoin#28698 Rebased-from: 4a5be10

…rameters Co-authored-by: Russell Yanofsky <russ@yanofsky.org> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com> Github-Pull: bitcoin#28698 Reabsed-From: 811067c

e4e8479 doc: update manual pages for v26.0rc2 (fanquake) 0b189a9 build: bump version to v26.0rc2 (fanquake) e097d4c gui: fix crash on selecting "Mask values" in transaction view (Sebastian Falbesoner) 05e8874 guix: update signapple (fanquake) deccc50 guix: Zip needs to include all files with time as SOURCE_DATE_EPOCH (Andrew Chow) fe57abd test: add coverage for snapshot chainstate not matching AssumeUTXO parameters (pablomartin4btc) b761a58 assumeutxo, blockstorage: prevent core dump on invalid hash (pablomartin4btc) d3ebf6e [test] Test i2p private key constraints (Vasil Dimov) 1f11784 [net] Check i2p private key constraints (dergoegge) 6544ffa bugfix: Mark CNoDestination and PubKeyDestination constructor explicit (MarcoFalke) Pull request description: Backports for v26.0rc2: * #28695 * #28698 * #28728 * #28757 * #28759 * bitcoin-core/gui#774 ACKs for top commit: josibake: ACK e4e8479 hebasto: re-ACK e4e8479, only a backport of bitcoin-core/gui#774 added since my [recent](#28754 (review)) review. TheCharlatan: Re-ACK e4e8479 Tree-SHA512: 4b95afd26b8bf91250cb883423de8b274cefa48dc474734f5900aeb756eee3a6c656116efcfa2caff3c250678c16b70cc6b7a5d840018dc7e2c1e8161622cd61

This is a follow-up on the invalid hash dump fix PR bitcoin#28698. bitcoin#28698 (review)

…bug_log on the assumeutxo invalid hash dump - follow-up #28698 7de7685 test, assumeutxo: Use assert_debug_log for error details (pablomartin4btc) Pull request description: This is a follow-up on the invalid hash dump fix #28698, [suggested](bitcoin/bitcoin#28698 (review)) by theStack and agreed by Sjors and ryanofsky. ACKs for top commit: Sjors: ACK 7de7685 maflcko: lgtm ACK 7de7685 Tree-SHA512: 036b3cef3084e3ead8923e8dcabe4fa7ebe97fb514d223aa38bc38df10337e3fe3113e42322178b58fb03fcd4511af4b5b56bceecbb7ded5b9758842c70db3f2

This is a follow-up on the invalid hash dump fix PR #28698. bitcoin/bitcoin#28698 (review)

DrahtBot added the CI failed label Oct 21, 2023

Sjors reviewed Oct 23, 2023

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Oct 23, 2023

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from theStack and Sjors October 23, 2023 13:23

assumeutxo, blockstorage: prevent core dump on invalid hash

4a5be10

pablomartin4btc force-pushed the assumeutxo-safer-exit-on-init-core-dumped branch from caae1c9 to 3d7c103 Compare October 25, 2023 03:08

DrahtBot requested review from ryanofsky and removed request for Sjors October 25, 2023 08:02

DrahtBot removed the CI failed label Oct 25, 2023

ryanofsky approved these changes Oct 25, 2023

View reviewed changes

test/functional/feature_assumeutxo.py Outdated Show resolved Hide resolved

test: add coverage for snapshot chainstate not matching AssumeUTXO pa…

811067c

…rameters Co-authored-by: Russell Yanofsky <russ@yanofsky.org> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

pablomartin4btc force-pushed the assumeutxo-safer-exit-on-init-core-dumped branch from 3d7c103 to 811067c Compare October 25, 2023 14:15

theStack approved these changes Oct 25, 2023

View reviewed changes

DrahtBot requested review from fjahr, ryanofsky and Sjors October 25, 2023 20:15

ryanofsky approved these changes Oct 26, 2023

View reviewed changes

fanquake merged commit feae4e0 into bitcoin:master Oct 29, 2023
16 checks passed

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 30, 2023

assumeutxo, blockstorage: prevent core dump on invalid hash

fdb143c

Github-Pull: bitcoin#28698 Rebased-from: 4a5be10

fanquake mentioned this pull request Oct 30, 2023

[26.x] Backports for rc2 #28754

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 31, 2023

assumeutxo, blockstorage: prevent core dump on invalid hash

b761a58

Github-Pull: bitcoin#28698 Rebased-from: 4a5be10

pablomartin4btc mentioned this pull request Nov 1, 2023

validation: assumeutxo params mainnet #28553

Draft

pablomartin4btc added a commit to pablomartin4btc/bitcoin that referenced this pull request Nov 9, 2023

test, assumeutxo: Use assert_debug_log for error details

7de7685

This is a follow-up on the invalid hash dump fix PR bitcoin#28698. bitcoin#28698 (review)

pablomartin4btc mentioned this pull request Nov 9, 2023

test: Check error details with assert_debug_log on the assumeutxo invalid hash dump - follow-up #28698 #28835

Merged

janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Apr 1, 2024

test, assumeutxo: Use assert_debug_log for error details

1e624ab

This is a follow-up on the invalid hash dump fix PR #28698. bitcoin/bitcoin#28698 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pablomartin4btc commented Oct 20, 2023 •

edited by fanquake

DrahtBot commented Oct 20, 2023 •

edited

theStack commented Oct 21, 2023

Sjors left a comment •

edited

ryanofsky left a comment

theStack commented Oct 24, 2023

pablomartin4btc commented Oct 24, 2023

pablomartin4btc commented Oct 25, 2023

Sjors commented Oct 25, 2023

ryanofsky left a comment

fjahr commented Oct 25, 2023

theStack left a comment

Sjors commented Oct 26, 2023

ryanofsky left a comment

naumenkogs commented Oct 27, 2023

fanquake commented Oct 30, 2023

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

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

Conversation

pablomartin4btc commented Oct 20, 2023 • edited by fanquake

DrahtBot commented Oct 20, 2023 • edited

Code Coverage

Reviews

theStack commented Oct 21, 2023

Sjors left a comment • edited

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

theStack commented Oct 24, 2023

pablomartin4btc commented Oct 24, 2023

pablomartin4btc commented Oct 25, 2023

Sjors commented Oct 25, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

fjahr commented Oct 25, 2023

theStack left a comment

Choose a reason for hiding this comment

Sjors commented Oct 26, 2023

ryanofsky left a comment

Choose a reason for hiding this comment

naumenkogs commented Oct 27, 2023

fanquake commented Oct 30, 2023

pablomartin4btc commented Oct 20, 2023 •

edited by fanquake

DrahtBot commented Oct 20, 2023 •

edited

Sjors left a comment •

edited