-
Notifications
You must be signed in to change notification settings - Fork 36.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
assumeutxo (2) #27596
assumeutxo (2) #27596
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
CI's passing after a silent conflict in the rebase. I've added a link to @Sjors' snapshot torrent in the PR description. No known outstanding issues here; ready for testing! |
Rebased. |
When running 8f431ad I noticed (and reproduced) that |
@jamesob What's your feeling on how important figuring out the fix for the pruning issue is at the moment? People don't consider it blocking #24008, right? Just curious about where to spend review time right now. FWIW, I noted the same issue on |
Right, pruning issues shouldn't block #24008 - those changes need to happen regardless of how we manage pruning with snapshots. I'll reproduce/investigate the pruning issues over the coming days. But it's worth noting that @Sjors didn't see any regressions in pruning on this branch before loading the snapshot, meaning the degraded pruning behavior only kicks in if the user loads a snapshot. And even though, the code just doesn't prune as aggressively as it seems it should. |
Hm, for what it's worth, pruning is working as expected for me on this branch.
Will attempt to repro with a fresh run, I guess? |
Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post-merge re tACK edbed31
Tested on mainnet
using utxo-800000.dat
file from @Sjors.
./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
{
"headers": 811416,
"chainstates": [
{
"blocks": 49020,
"bestblockhash": "00000000283ec5677b8757c0b652e67087b968e337e9779b35d9092bf4003882",
"difficulty": 6.085476906000794,
"verificationprogress": 5.482400033922018e-05,
"coins_db_cache_bytes": 419430,
"coins_tip_cache_bytes": 784649420,
"validated": true
},
{
"blocks": 800000,
"bestblockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
"difficulty": 53911173001054.59,
"verificationprogress": 0.9591139668014443,
"coins_db_cache_bytes": 7969177,
"coins_tip_cache_bytes": 14908338995,
"snapshot_blockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
"validated": false
}
]
}
I think description should be updated removing references to previous snapshot file from @jamesob, as if a reviewer uses that file without updating the code in src/kernel/chainparams.cpp
according to @Sjors commit but with correct details of 788000 block, running loadtxoutset
will error with:
/src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset ${AU_DATADIR}/utxo-788000.dat
error code: -32603
error message:
Unable to load UTXO snapshot .../.test_utxo/utxo-788000.dat
Also when re-try to run again loadtxoutset
after its completion user would obtain:
./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset ${AU_DATADIR}/utxo-800000.dat
error code: -32603
error message:
Unable to load UTXO snapshot .../.test_utxo_2/utxo-800000.dat
While in the bitcoind
logs a user would see the correct issue:
2023-10-09T20:26:13Z [httpworker.2] [snapshot] waiting to see blockheader 00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054 in headers chain before snapshot activation
2023-10-09T20:26:13Z [httpworker.2] [snapshot] can't activate a snapshot-based chainstate more than once
And I think when I previously tested 1b1d711, unless I dreamt it, I saw the correct error description can't activate a snapshot-based chainstate more than once
as the output from cli
.
I agree loadtxoutset
timeout issue should be solved.
03f8208 doc: assumeutxo prune and index notes (Sjors Provoost) Pull request description: Based on recent comments on #27596. ACKs for top commit: pablomartin4btc: re ACK 03f8208 ryanofsky: ACK 03f8208. Nice changes, these seem like very helpful notes Tree-SHA512: fe651b49f4d667400a3655899f27a96dd1eaf67cf9215fb35db5f44fb8c0313e7d541518be6791fec93392df24b909793f3886adb808e53228ed2a291165639d
…hash 811067c test: add coverage for snapshot chainstate not matching AssumeUTXO parameters (pablomartin4btc) 4a5be10 assumeutxo, blockstorage: prevent core dump on invalid hash (pablomartin4btc) Pull request description: While reviewing #27596 (ran `loadtxoutset` in `mainnet` before `m_assumeutxo_data` is empty as [currently](https://github.com/jamesob/bitcoin/blob/434495a8c1496ca23fe35b84499f3daf668d76b8/src/kernel/chainparams.cpp#L175-L177) 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) ``` <details> <summary>This is also happening before IBD is completed (<code>background validation</code> still being performed as it can be seen in rpc <code>getchainstates</code>)</summary> ``` /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 } ] } ``` </details> <details> <summary>Steps to reproduce the core dump error and its output:</summary> 1. Perform a `loadtxoutset` in `mainnet` on compiled `bitcoind` adding the block hash from Sjors's [commit](Sjors@24deb20). 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: ``` 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) ``` </details> <details> <summary>After original change, error message output:</summary> ``` 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 ``` </details> <details> <summary>Alternative on error handling using <code>return error()</code> instead of <code>return FatalError()</code> used in this PR, which produces a different output and perhaps confusing:</summary> ``` 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 ``` </details> <details> <summary>Current state (including ryanofsky <a href="https://github.com/bitcoin/bitcoin/pull/28698#discussion_r1368635965">suggestion</a>), after code change, error message output:</summary> ``` 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 ``` </details> ACKs for top commit: naumenkogs: ACK 811067c theStack: ACK 811067c ryanofsky: Code review ACK 811067c. Tree-SHA512: cfc137b0a4f638b99fd7dac2c35cc729ef71ae1166a2a8960a91055ec90841cb33aed589834012cfe0e157937e2a76a88d1020ea1df2bc98e1114eb1fc8eaae4
03f8208 doc: assumeutxo prune and index notes (Sjors Provoost) Pull request description: Based on recent comments on bitcoin#27596. ACKs for top commit: pablomartin4btc: re ACK 03f8208 ryanofsky: ACK 03f8208. Nice changes, these seem like very helpful notes Tree-SHA512: fe651b49f4d667400a3655899f27a96dd1eaf67cf9215fb35db5f44fb8c0313e7d541518be6791fec93392df24b909793f3886adb808e53228ed2a291165639d
5555d8d test: Use blocks_path where possible (MarcoFalke) fa91089 rpc: Fix race in loadtxoutset (MarcoFalke) Pull request description: The tip may have advanced, also if it did not, there is no reason to have two variables point to the same block. Fixes #27596 (comment) ACKs for top commit: achow101: ACK 5555d8d pablomartin4btc: ACK 5555d8d BrandonOdiwuor: Code Review ACK 5555d8d Tree-SHA512: 23a82924a915b61bb1adab8ad20ec8914139c8ee647817af34ca27ee310a2e45833d8b285503e0feebe63e4667193d6d98cfcbbc1509bf40712225e04dd19e8b
…ol not empty 8d20602 test, assumeutxo: Add test to ensure failure when mempool not empty (Hernan Marino) Pull request description: Add a test to ensure that loadtxoutset fails when the node's mempool is not empty, as suggested by maflcko here: #27596 (comment) ACKs for top commit: maflcko: re-ACK 8d20602 BrandonOdiwuor: ACK 8d20602 Tree-SHA512: 97c9668c0f38897934bf0d326515d898d4e682ff219deba9d751b35125b5cf33d51c9df116a74117ecf0394f28995a3d0cae1266b1e5acb4365ff4f309ce3f6c
Similar changes in bitcoin#27596: assumeutxo (2)
Similar changes in bitcoin#27596: assumeutxo (2)
Similar changes in bitcoin#27596: assumeutxo (2)
bf377d4 fix: correct is_snapshot_cs in VerifyDB (James O'Beirne) Pull request description: ## Issue being fixed or feature implemented Flag `is_snapshot_cs` has been inverted in bitcoin#21584 Discovered during investigation of issue: ``` Verifying last 6 blocks at level 3 2024-08-14T14:51:55Z [0%]...*** Found EvoDB inconsistency, you must reindex to continue ``` So far as code below does: ``` if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) { // If pruning or running under an assumeutxo snapshot, only go // back as far as we have data. LogPrintf("VerifyDB(): block verification stopping at height %d (pruning, no data)\n", pindex->nHeight); break; } ``` In case of missing data in evo db we will get instead of "block verification stopping at height" we may get data inconsistency issue. ## What was done? Inverted condition back (same fix in bitcoin#27596) ## How Has This Been Tested? Unit/functional tests doesn't cover it, but they do no fail after fix. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK bf377d4 PastaPastaPasta: utACK bf377d4 Tree-SHA512: ac21e6db6e23c4c7dc150fb16171aef47c9f42c29466b403bca7d56ed6faa2fccc41df92e1fabec4d6e9fd56991e152dea168593a4550fc3583631a63009c27f
bf377d4 fix: correct is_snapshot_cs in VerifyDB (James O'Beirne) Pull request description: ## Issue being fixed or feature implemented Flag `is_snapshot_cs` has been inverted in bitcoin#21584 Discovered during investigation of issue: ``` Verifying last 6 blocks at level 3 2024-08-14T14:51:55Z [0%]...*** Found EvoDB inconsistency, you must reindex to continue ``` So far as code below does: ``` if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) { // If pruning or running under an assumeutxo snapshot, only go // back as far as we have data. LogPrintf("VerifyDB(): block verification stopping at height %d (pruning, no data)\n", pindex->nHeight); break; } ``` In case of missing data in evo db we will get instead of "block verification stopping at height" we may get data inconsistency issue. ## What was done? Inverted condition back (same fix in bitcoin#27596) ## How Has This Been Tested? Unit/functional tests doesn't cover it, but they do no fail after fix. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK bf377d4 PastaPastaPasta: utACK bf377d4 Tree-SHA512: ac21e6db6e23c4c7dc150fb16171aef47c9f42c29466b403bca7d56ed6faa2fccc41df92e1fabec4d6e9fd56991e152dea168593a4550fc3583631a63009c27f
This changeset finishes the first phase of the assumeutxo project. It makes UTXO snapshots loadable via RPC (
loadtxoutset
) and addsassumeutxo
parameters to chainparams. It contains all the remaining changes necessary to both use an assumedvalid snapshot chainstate and do a full validation sync in the background.This may look like a lot to review, but note that
So it shouldn't be as burdensome to review as the linecount might suggest.
init.cpp
andnet_processing.cpp
to make simultaneous IBD across multiple chainstates work.CValidationInterface
events are given with an additional parameter, ChainstateRole, and all indexers ignore events from ChainstateRole::ASSUMEDVALID so that indexation only happens sequentially.-reindex
properly wipe snapshot chainstates.loadtxoutset
and (hidden)getchainstates
.The next phase, if it were to be pursued, would be coming up with a way to distribute the UTXO snapshots over the P2P network.
UTXO snapshots
Create your own with
./contrib/devtools/utxo_snapshot.sh
, e.g.or use the pre-generated ones listed below.
magnet:?xt=urn:btih:511e09f4bf853aefab00de5c070b1e031f0ecbe9&dn=utxo-testnet-2500000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
79db4b025448cc0ac388d8589a28eab02de53055d181e34eb47391717aa16388
magnet:?xt=urn:btih:9da986cb27b3980ea7fd06b21e199b148d486880&dn=utxo-signet-160000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
eeeca845385ba91e84ef58c09d38f98f246a24feadaad57fe1e5874f3f92ef8c
magnet:?xt=urn:btih:50ee955bef37f5ec3e5b0df4cf0288af3d715a2e&dn=utxo-800000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
Testing
For fun (~5min)
If you want to do a quick test, you can run
./contrib/devtools/test_utxo_snapshots.sh
and follow the instructions. This is mostly obviated by the functional tests, though.For real (longer)
If you'd like to experience a real usage of assumeutxo, you can do that too.
I've cut a new snapshot at height 788'000 (http://img.jameso.be/utxo-788000.dat - but you can do it yourself with
./contrib/devtools/utxo_snapshot.sh
if you want). Download that, and then create a datadir for testing:Obtain this branch, build it, and then start bitcoind:
Then, in some other window, load the snapshot
You'll see some log messages about headers retrieval and waiting to see the snapshot in the headers chain. Once you get the full headers chain, you'll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk. After all that happens, you should be syncing to tip in pretty short order, and you'll see the occasional
[background validation]
log message go by.In yet another window, you can check out chainstate status with
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
as well as usual favorites like
getblockchaininfo
.