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

theStack · 2023-10-15T15:22:11Z

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

$ ./src/bitcoin-cli loadtxoutset ~/.vimrc
<wait>

bitcoind log:
...
2023-10-15T14:55:45Z [snapshot] waiting to see blockheader 626174207465730a7265626d756e207465730a656c62616e65207861746e7973 in headers chain before snapshot activation
...

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:

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

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 · 2023-10-15T15:22:14Z

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 · 2023-10-16T09:28:40Z

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 · 2023-10-16T10:28:04Z

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

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.

maflcko · 2023-10-16T14:35:49Z

src/rpc/blockchain.cpp

@@ -2751,6 +2751,10 @@ static RPCHelpMan loadtxoutset()
    afile >> metadata;

    uint256 base_blockhash = metadata.m_base_blockhash;
+    if (!Params().AssumeutxoForBlockhash(base_blockhash).has_value()) {


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

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.

…O parameters

ryanofsky

Code review ACK 9620cb4. 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 (comment). But the current PR seems like a simple improvement for now.

maflcko · 2023-10-16T15:53:32Z

lgtm ACK 9620cb4

pablomartin4btc

tACK 9620cb4

error ouput with a file which size >= 40 bytes (size of SnapshotMetadata)

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

error ouput with a file which size < 40 bytes (not enough to fit SnapshotMetadata structure)

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

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

pablomartin4btc · 2023-10-16T19:49:04Z

src/rpc/blockchain.cpp

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


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.

Suggested change

afile >> metadata;

// Check if the file size is at least as large as the expected size of SnapshotMetadata

if (afile.size() < sizeof(SnapshotMetadata)) {

throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to load UTXO snapshot, "

"file size is invalid");

}

afile >> metadata;

fjahr · 2023-10-17T08:26:35Z

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 · 2023-10-17T08:38:20Z

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 · 2023-10-17T08:39:57Z

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 (comment)

fjahr · 2023-10-17T15:01:33Z

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 · 2023-10-17T15:42:17Z

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.

…esn't match AssumeUTXO parameters

DrahtBot mentioned this pull request Oct 15, 2023

test: Add assumeutxo test for wrong hash #28647

Merged

pablomartin4btc reviewed Oct 16, 2023

View reviewed changes

maflcko reviewed Oct 16, 2023

View reviewed changes

assumeutxo: fail early if snapshot block hash doesn't match AssumeUTX…

9620cb4

…O parameters

theStack force-pushed the 202310-assumeutxo-fail_early_if_blockhash_not_in_params branch from 164273c to 9620cb4 Compare October 16, 2023 15:21

ryanofsky approved these changes Oct 16, 2023

View reviewed changes

DrahtBot requested a review from pablomartin4btc October 16, 2023 15:42

DrahtBot requested review from pablomartin4btc and removed request for pablomartin4btc October 16, 2023 15:53

fanquake added this to the 26.0 milestone Oct 16, 2023

pablomartin4btc reviewed Oct 16, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 16, 2023

assumeutxo, rpc: Add 'start' parameter to loadtxoutset #28659

Closed

fanquake merged commit 738ef44 into bitcoin:master Oct 17, 2023
16 checks passed

theStack deleted the 202310-assumeutxo-fail_early_if_blockhash_not_in_params branch October 17, 2023 09:23

Sjors mentioned this pull request Oct 18, 2023

rpc: loadtxoutset should return errors that currently go to logs #28621

Open

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2023

Merge bitcoin#28652: assumeutxo: fail early if snapshot block hash do…

06fc94d

…esn't match AssumeUTXO parameters

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theStack commented Oct 15, 2023

DrahtBot commented Oct 15, 2023 •

edited

maflcko commented Oct 16, 2023

theStack commented Oct 16, 2023 •

edited

pablomartin4btc left a comment

maflcko Oct 16, 2023

theStack Oct 16, 2023

ryanofsky left a comment •

edited

maflcko commented Oct 16, 2023

pablomartin4btc left a comment

pablomartin4btc Oct 16, 2023

fjahr commented Oct 17, 2023

theStack commented Oct 17, 2023

maflcko commented Oct 17, 2023

fjahr commented Oct 17, 2023

hernanmarino commented Oct 17, 2023

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

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

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

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

Conversation

theStack commented Oct 15, 2023

DrahtBot commented Oct 15, 2023 • edited

Code Coverage

Reviews

Conflicts

maflcko commented Oct 16, 2023

theStack commented Oct 16, 2023 • edited

pablomartin4btc left a comment

Choose a reason for hiding this comment

maflcko Oct 16, 2023

Choose a reason for hiding this comment

theStack Oct 16, 2023

Choose a reason for hiding this comment

ryanofsky left a comment • edited

Choose a reason for hiding this comment

maflcko commented Oct 16, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc Oct 16, 2023

Choose a reason for hiding this comment

fjahr commented Oct 17, 2023

theStack commented Oct 17, 2023

maflcko commented Oct 17, 2023

fjahr commented Oct 17, 2023

hernanmarino commented Oct 17, 2023

DrahtBot commented Oct 15, 2023 •

edited

theStack commented Oct 16, 2023 •

edited

ryanofsky left a comment •

edited