-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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.
Reviewed the remaining commits. I'll try to pick off some comments from the past few days as followup(s) and/or issues.
------- | ||
|
||
When using assumeutxo with `-prune`, the prune budget may be exceeded if it is set | ||
lower than 1100MB (i.e. `MIN_DISK_SPACE_FOR_BLOCK_FILES * 2`). Prune budget is normally |
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.
In general a user for who 0.5 GB makes a difference is going to have a problem. The downloaded snapshot itself is multiple GB and the chainstate (even without assumeutxo) is growing at 0.5 GB per month. We could add a hint that it's safe to delete utxo-xxxxxx.dat
once the RPC returns (if there's no crash).
// (sequentially) after it is fully verified by the background chainstate. This | ||
// is to avoid any out-of-order indexing. | ||
// | ||
// TODO at some point we could parameterize whether a particular index can be |
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.
373cf91 being able to build out of order is necessary for a pruned node.
[note: currently running an IBD on mainnet with -prune=2000
and -blockfilterindex=1
to see what happens… I'm guessing it will be missing a range of blocks above the snapshot? If so then we should either refuse to load a snapshot or delay pruning with this combination. If we delay pruning, we should warn the user that enabling the index will temporarily require a lot of extra storage (x GB per month of how old the snapshot is)]
post merge ACK . I've been testing thoroughly on mainnet, and everything worked as expected. I 'll provide more detalied feedback on follow ups. |
5d227a6 rpc: Use Ensure(Any)Chainman in assumeutxo related RPCs (Fabian Jahr) 710e5db doc: Drop references to assumevalid in assumeutxo docs (Fabian Jahr) 1ff1c34 test: Rename wait_until_helper to wait_until_helper_internal (Fabian Jahr) a482f86 chain: Rename HaveTxsDownloaded to HaveNumChainTxs (Fabian Jahr) 82e48d2 blockstorage: Let FlushChainstateBlockFile return true in case of missing cursor (Fabian Jahr) 73700fb validation, test: Improve and document nChainTx check for testability (Fabian Jahr) 2c9354f doc: Add snapshot chainstate removal warning to reindexing documentation (Fabian Jahr) 4e915e9 test: Improvements of feature_assumeutxo (Fabian Jahr) a47fbe7 doc: Add and edit some comments around assumeutxo (Fabian Jahr) 0a39b8c validation: remove unused mempool param in DetectSnapshotChainstate (Fabian Jahr) Pull request description: Addressing what I consider to be non- or not-too-controversial comments from #27596. Let me know if I missed anything among the many comments that can be easily included here. ACKs for top commit: ryanofsky: Code review ACK 5d227a6. Just suggested doc change and new EnsureChainman RPC cleanup commit since last review. Tree-SHA512: 6f7c762100e18f82946b881676db23e67da7dc3a8bf04e4999a183e90b4f150a0b1202bcb95920ba937a358867bbf2eca300bd84b9b1776c7c490410e707c267
Imo most post-merge comments have now been addressed in either followup PRs or open issues: #28608, #28616, #28618, #28620, #28621, #28619, (already merged: #28590, #28589, #28562) The tracing issue could be updated to point to them. These seem unaddressed:
|
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
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
.