assumeutxo: fail early if snapshot block hash doesn't match AssumeUTXO parameters

theStack commented at 3:22 pm on October 15, 2023: contributor

Right now the loadtxoutset RPC call treats literally all files with a minimum size of 40 bytes (=size of metadata) as potential valid snapshot candidates and the waiting loop for seeing the metadata block hash in the headers chain is always entered, e.g.:

0$ ./src/bitcoin-cli loadtxoutset ~/.vimrc
1<wait>
2
3bitcoind log:
4...
52023-10-15T14:55:45Z [snapshot] waiting to see blockheader 626174207465730a7265626d756e207465730a656c62616e65207861746e7973 in headers chain before snapshot activation
6...

There is no point in doing any further action though if we already know from the start that the UTXO snapshot loading won’t be successful. This PR adds an assumeutxo parameter check immediately after the metadata is read in, so we can fail immediately on a mismatch:

0$ ./src/bitcoin-cli loadtxoutset ~/.vimrc                                                                                      
1error code: -32603                                                  
2error message:                                                      
3Unable to load UTXO snapshot, assumeutxo block hash in snapshot metadata not recognized (626174207465730a7265626d756e207465730a656c62616e
465207861746e7973)

This way, users who mistakenly try to load files that are not snapshots don’t have to wait 10 minutes (=the block header waiting timeout) anymore to get a negative response. If a file is loaded which is a valid snapshot (referencing to an existing block hash), but one which doesn’t match the parameters, the feedback is also faster, as we don’t have to wait anymore to see the hash in the headers chain before getting an error.

This is also partially fixes #28621.

DrahtBot commented at 3:22 pm on October 15, 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	ryanofsky, maflcko, pablomartin4btc

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28659 (assumeutxo, rpc: Add ‘start’ parameter to loadtxoutset by hernanmarino)
#28647 (test: Add assumeutxo test for wrong hash by maflcko)

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.

maflcko commented at 9:28 am on October 16, 2023: member

Wasn’t the goal to make the RPC async, in which case the wait loop can be replaced with an immediate error? This would also avoid having to duplicate error checks in the RPC, as well as the Activate function?

theStack commented at 10:28 am on October 16, 2023: contributor

Wasn’t the goal to make the RPC async, in which case the wait loop can be replaced with an immediate error? This would also avoid having to duplicate error checks in the RPC, as well as the Activate function?

Yes, I think that’s one of the long-term plans: #28620. It seems that currently no-one is working on that though and even if, it’s likely too late to include it into the upcoming release v26 anyway. This PR is based on the assumption that the loadtxoutset RPC is released in its current form (if that’s the case, we should add an “EXPERIMENTAL” warning) and given that, we shouldn’t waste users time and leave them up to 10 minutes in the dark (creating the illusion that something useful happens in the background) until they get an error for an RPC that currently can only fail on mainnet. Or is the plan to deactivate loadtxoutset until #28620 is implemented?

pablomartin4btc commented at 12:58 pm on October 16, 2023: member

Concept ACK.

It happened to me during assumeutxo testing so returning errors earlier if possible makes sense to me, even if loadtxoutset is made async at some point.

in src/rpc/blockchain.cpp:2754 in 164273c1e1 outdated

2750@@ -2751,6 +2751,10 @@ static RPCHelpMan loadtxoutset()
2751     afile >> metadata;
2752 
2753     uint256 base_blockhash = metadata.m_base_blockhash;
2754+    if (!Params().AssumeutxoForBlockhash(base_blockhash).has_value()) {

maflcko commented at 2:35 pm on October 16, 2023:

nit: Use chainman to get the params, instead of using the global?

theStack commented at 3:24 pm on October 16, 2023:

Thanks, done. Intentionally moved the chainman definition up more than needed (right after node) to fail early in the (hypothetical?) case that there’s no chainman.

assumeutxo: fail early if snapshot block hash doesn't match AssumeUTXO parameters 9620cb4493

theStack force-pushed on Oct 16, 2023

ryanofsky approved

ryanofsky commented at 3:41 pm on October 16, 2023: contributor

Code review ACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171. This should fix an annoyance and bad UX.

It would be possible to deduplicate the checks in the loadtxoutset function and ActivateSnapshot without making the RPC async by splitting ActivateSnapshot up and returning util::Result as suggested #27596 (review). But the current PR seems like a simple improvement for now.

DrahtBot requested review from pablomartin4btc on Oct 16, 2023

maflcko commented at 3:53 pm on October 16, 2023: member

lgtm ACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171

DrahtBot removed review request from pablomartin4btc on Oct 16, 2023

DrahtBot requested review from pablomartin4btc on Oct 16, 2023

fanquake added this to the milestone 26.0 on Oct 16, 2023

in src/rpc/blockchain.cpp:2752 in 9620cb4493

2751@@ -2751,14 +2752,16 @@ static RPCHelpMan loadtxoutset()
2752     afile >> metadata;

pablomartin4btc commented at 7:49 pm on October 16, 2023:

Since we are touching this file I think we could improved the iostream error that’s thrown when the file size is less than 40 bytes.

0    // Check if the file size is at least as large as the expected size of SnapshotMetadata
1    if (afile.size() < sizeof(SnapshotMetadata)) {
2        throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to load UTXO snapshot, "
3                "file size is invalid");
4    }
5    afile >> metadata;

pablomartin4btc commented at 7:49 pm on October 16, 2023: member

tACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171

0./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset ${AU_DATADIR}/utxo-111.dat
1error code: -32603
2error message:
3Unable to load UTXO snapshot, assumeutxo block hash in snapshot metadata not recognized (3634353634363536363435363436353636343536343635363634353634363536)

0./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset ${AU_DATADIR}/utxo-111.dat
1error code: -1
2error message:
3AutoFile::read: end of file: iostream error

I left a suggestion to handle iostream error when utxo file size is less than 40 bytes.

fjahr commented at 8:26 am on October 17, 2023: contributor

Since this adds the test for a bad hash, that Todo should be removed from the list at the top of feature_assumeutxo.py

theStack commented at 8:38 am on October 17, 2023: contributor

Since this adds the test for a bad hash, that Todo should be removed from the list at the top of feature_assumeutxo.py

This PR doesn’t add a new test; the “bad hash” TODO (which refers to the UTXO set hash, not the block hash in the metadata, as far as I understand) is tackled in #28647.

maflcko commented at 8:39 am on October 17, 2023: member

Since this adds the test for a bad hash, that Todo should be removed from the list at the top of feature_assumeutxo.py

This PR doesn’t add a new test; the “bad hash” TODO (which refers to the UTXO set hash, not the block hash in the metadata, as far as I understand) is tackled in #28647.

No, I’d say it is still up for grabs: #28647 (review)

fanquake merged this on Oct 17, 2023

fanquake closed this on Oct 17, 2023

theStack deleted the branch on Oct 17, 2023

fjahr commented at 3:01 pm on October 17, 2023: contributor

Since this adds the test for a bad hash, that Todo should be removed from the list at the top of feature_assumeutxo.py

This PR doesn’t add a new test; the “bad hash” TODO (which refers to the UTXO set hash, not the block hash in the metadata, as far as I understand) is tackled in #28647.

Sorry, I actually meant the block hash one, just misspelled/confused: TODO: Valid snapshot file, but referencing an unknown block. But I guess the “unknown” part is not covered yet, so I addressed it here: #28666

hernanmarino commented at 3:42 pm on October 17, 2023: contributor

Wasn’t the goal to make the RPC async, in which case the wait loop can be replaced with an immediate error? This would also avoid having to duplicate error checks in the RPC, as well as the Activate function?

Yes, I think that’s one of the long-term plans: #28620. It seems that currently no-one is working on that though and even if, it’s likely too late to include it into the upcoming release v26 anyway. This PR is based on the assumption that the loadtxoutset RPC is released in its current form (if that’s the case, we should add an “EXPERIMENTAL” warning) and given that, we shouldn’t waste users time and leave them up to 10 minutes in the dark (creating the illusion that something useful happens in the background) until they get an error for an RPC that currently can only fail on mainnet. Or is the plan to deactivate loadtxoutset until #28620 is implemented?

I implemented a (partial) fix for #28620 , you might want to take a look.

Frank-GER referenced this in commit 06fc94df7f on Oct 21, 2023

bitcoin locked this on Oct 16, 2024

assumeutxo: fail early if snapshot block hash doesn’t match AssumeUTXO parameters #28652

Code Coverage

Reviews

Conflicts