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. |
|
Left some low level review comments. I guess the only question I have so far is why I'll try to compile and test later. |
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
Didn't get too deep with my review before this got merged - but it looked good to me on a high level. Just two comments that may or may not be considered in a follow-up.
|
|
||
| Chainstate* bg_chain{WITH_LOCK(cs_main, return BackgroundSyncInProgress() ? m_ibd_chainstate.get() : nullptr)}; | ||
| BlockValidationState bg_state; | ||
| if (bg_chain && !bg_chain->ActivateBestChain(bg_state, block)) { |
There was a problem hiding this comment.
a9ea542: it's probably not significant, but if we split up the two ifs we could avoid creating bg_state object each time a new block comes in when not using AssumeUtxo.
| ------- | ||
|
|
||
| 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.
Maybe mention that if an index is enabled when pruning, also the 1100MB might not be sufficient because the prune locks wouldn't allow us to prune anything from the snapshot CS until the background CS has finished loading.
|
tACK on WSL Ubuntu 22.04. I tested On On Both The only difference I found is that the |
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
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.
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.
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. |
|
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:
|
pablomartin4btc
left a comment
pablomartin4btc
left a comment
There was a problem hiding this comment.
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.
19 participants
This changeset finishes the first phase of the assumeutxo project. It makes UTXO snapshots loadable via RPC (
loadtxoutset) and addsassumeutxoparameters 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.cppandnet_processing.cppto make simultaneous IBD across multiple chainstates work.CValidationInterfaceevents are given with an additional parameter, ChainstateRole, and all indexers ignore events from ChainstateRole::ASSUMEDVALID so that indexation only happens sequentially.-reindexproperly wipe snapshot chainstates.loadtxoutsetand (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%3A696979db4b025448cc0ac388d8589a28eab02de53055d181e34eb47391717aa16388magnet:?xt=urn:btih:9da986cb27b3980ea7fd06b21e199b148d486880&dn=utxo-signet-160000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969eeeca845385ba91e84ef58c09d38f98f246a24feadaad57fe1e5874f3f92ef8cmagnet:?xt=urn:btih:50ee955bef37f5ec3e5b0df4cf0288af3d715a2e&dn=utxo-800000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969Testing
For fun (~5min)
If you want to do a quick test, you can run
./contrib/devtools/test_utxo_snapshots.shand 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.shif 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} getchainstatesas well as usual favorites like
getblockchaininfo.