assumeutxo: Check snapshot base block is not in invalid chain #30267

fjahr · 2024-06-11T08:49:29Z

This was discovered in a discussion in #29996

If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.

While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.

The behavior change described above is in the second commit.

The first commit refactors the early checks in the loadtxoutset RPC by moving them into ActivateSnapshot() in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between rpc/blockchain.cpp and validation.cpp. In order to be able to return the error message to users of the RPC, the return type of ActivateSnapshot() is changed from bool to util::Result.

The third commit removes an unnecessary restart introduced in #29428.

DrahtBot · 2024-06-11T08:49:32Z

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	mzumsande, alfonsoromanz, achow101
Stale ACK	tdb3, BrandonOdiwuor

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:

#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#30329 (fuzz: improve utxo_snapshot target by mzumsande)
#30320 (assumeutxo: Don't load a snapshot if it's not in the best header chain by mzumsande)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint 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.

tdb3

ACK cbc604f
Seems like an improvement to avoid expending resources loading a snapshot that won't be part of a valid chain.

Built and ran unit and functional tests (including feature_assumeutxo). All passed.

Tested that feature_assumeutxo would fail with:

bool start_block_invalid = false;

Test failed as expected.

test/functional/feature_assumeutxo.py

alfonsoromanz

tACK cbc604f

This PR successfully builds and passes the test when running test/functional/feature_assumeutxo.py.

Additionally, I tested this in the context of my PR (#29996) back when the code looked something like this:

    def test_snapshot_in_a_divergent_chain(self, dump_output_path):
        node = self.nodes[2]
        # Rollback the chain down to START_HEIGHT
        block_hash = node.getblockhash(START_HEIGHT + 1)
        node.invalidateblock(block_hash)
        assert_equal(node.getblockcount(), START_HEIGHT)

        self.log.info(f"Check importing a snapshot where current chain-tip is not an ancestor of the snapshot block but has less work")
        # Generate a divergent chain up to 298
        self.generate(node, nblocks=99, sync_fun=self.no_op)
        assert_equal(node.getblockcount(), SNAPSHOT_BASE_HEIGHT - 1)

        # Try importing the snapshot and assert its success
        self.log.info('Importing the snapshot')
        loaded = node.loadtxoutset(dump_output_path)

The snapshot load failed with the expected message: test_framework.authproxy.JSONRPCException: The base block header is part of an invalid chain. (-32603)

BrandonOdiwuor

Code Review ACK cbc604f

src/rpc/blockchain.cpp

mzumsande · 2024-06-13T18:25:33Z

src/rpc/blockchain.cpp

+
+    bool start_block_invalid = WITH_LOCK(::cs_main,
+            return snapshot_start_block->nStatus & BLOCK_FAILED_MASK);
+    if (start_block_invalid) {


Some checks are within ActivateSnapshot(), others, like the added one, are in the rpc code (which means that they can't be covered in unit tests for that function). Do you know if there is a reason for that?

re: #30267 (comment)

Some checks are within ActivateSnapshot(), others, like the added one, are in the rpc code (which means that they can't be covered in unit tests for that function). Do you know if there is a reason for that?

I think this is just a mistake, and it makes the checks less reliable because they are done without keeping cs_main locked. I suggested fixing this before in #27596 (comment).

I think these were always separated like that. But sounds good to me to move them. I have added a refactoring commit which moves the other two early checks into ActivateSnapshot() and then I am adding the new check there as well.

Looks good (except for the compiler issue with Join()) - I think it would be helpful to not only log the reason, but also return them with the RPC - for that, ActivateSnapshot() would need to return it, possibly as util::Result as suggested by ryanofsky, but that could be a follow-up.
Though for now, it might be helpful to expand the error ("Unable to load UTXO snapshot") to point the user to the debug log for the reason.

Ah, yeah, I gave it a shot and it pretty simple so I am changing the return type as well in the refactoring commit now.

DrahtBot · 2024-06-18T16:02:30Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/26368413924}

fjahr · 2024-06-19T19:42:27Z

Needed to rebase because of a silent merge conflict.

Also changes the return type of ActiveSnapshot to allow returning the error message to the user of the loadtxoutset RPC.

mzumsande

Code Review ACK 750f8b5

test/functional/feature_assumeutxo.py

mzumsande · 2024-06-20T18:50:38Z

src/rpc/blockchain.cpp

-        throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to load UTXO snapshot " + fs::PathToString(path));
+    auto activation_result{chainman.ActivateSnapshot(afile, metadata, false)};
+    if (!activation_result) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf(_("Unable to load UTXO snapshot: %s\n"), util::ErrorString(activation_result)).original);


This changes it such that the path is no longer included in the RPC error. I think that it wasn't really helpful anyway (the user already provided the path when calling the RPC, so why the need to report it back to the user?), but just wanted to mention it in case someone would miss that.

I think the path is transformed to an absolute one, so the string representation may be different from the one passed in by the user.

Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>

fjahr · 2024-06-21T08:40:38Z

Just fixed typo noticed by @mzumsande : https://github.com/bitcoin/bitcoin/compare/750f8b50a749e577fc11f2f9f79e06cdd29e66f5..2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28

mzumsande · 2024-06-21T15:04:51Z

re-ACK 2f9bde6

fa5b892 rpc: Use untranslated error strings in loadtxoutset (MarcoFalke) fa45865 refactor: Use named arguments to get path arg in loadtxoutset (MarcoFalke) Pull request description: Motivation: * Some are not translated at all, anyway. See #30267 (comment) * For others translation is not yet needed, because they are not called by the GUI (yet) * For others translations will never be needed, because they are RPC code. See #30267 (comment) Also, while touching this: * Remove the trailing `\n`. See #30267 (comment) * Add back the path. See #30267 (comment) * Use named args to get the path. ACKs for top commit: fjahr: re-ACK fa5b892 tdb3: ACK fa5b892 ryanofsky: Code review ACK fa5b892 Tree-SHA512: 46504dc5fd55a6274ef885dbe071aa9efb25bca247cd68cd86fb2ff066d70d295e0522e1fe42e63f1fdf7e4c89bd696220edaf06e33b804aba746492eafd852e

- "Valid snapshot file, but referencing a snapshot block that turns out to be invalid, or has an invalid parent" has been addressed in bitcoin#30267 - "An ancestor of snapshot block" - If chain tip refers to blocks in this context then any successful load is addressing this because if we have synced past the snapshot base block we fail because we don't need assumeutxo anymore. And if this is about headers then this is the `test_headers_not_synced()` case. - "A descendant of the snapshot block" - If this refers to blocks the `test_snapshot_with_less_work()` addressed this and if it is just headers in this case again it would be represented in all of the successful loads in the test. Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>

Use result.Update in CompleteChainstateInit() and ChainstateManager::ActivateSnapshot() so it is possible for them to return warning messages (about flush failures) in upcoming commits. CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665 and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but previously these functions only returned Result values themselves, and did not call other functions that return Result values. Now, some functions they are calling will also return Result values, so refactor these functions to use result.Update so they can merge results and return complete error and warning messages.

- "Valid snapshot file, but referencing a snapshot block that turns out to be invalid, or has an invalid parent" has been addressed in bitcoin#30267 - "An ancestor of snapshot block" - If chain tip refers to blocks in this context then any successful load is addressing this because if we have synced past the snapshot base block we fail because we don't need assumeutxo anymore. And if this is about headers then this is the `test_headers_not_synced()` case. - "A descendant of the snapshot block" - If this refers to blocks the `test_snapshot_with_less_work()` addressed this and if it is just headers in this case again it would be represented in all of the successful loads in the test. Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>

Use result.Update in CompleteChainstateInit() and ChainstateManager::ActivateSnapshot() so it is possible for them to return warning messages (about flush failures) in upcoming commits. CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665 and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but previously these functions only returned Result values themselves, and did not call other functions that return Result values. Now, some functions they are calling will also return Result values, so refactor these functions to use result.Update so they can merge results and return complete error and warning messages.

- "Valid snapshot file, but referencing a snapshot block that turns out to be invalid, or has an invalid parent" has been addressed in bitcoin#30267 - "An ancestor of snapshot block" - If chain tip refers to blocks in this context then any successful load is addressing this because if we have synced past the snapshot base block we fail because we don't need assumeutxo anymore. And if this is about headers then this is the `test_headers_not_synced()` case. - "A descendant of the snapshot block" - If this refers to blocks the `test_snapshot_with_less_work()` addressed this and if it is just headers in this case again it would be represented in all of the successful loads in the test. Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>

Use result.Update in CompleteChainstateInit() and ChainstateManager::ActivateSnapshot() so it is possible for them to return warning messages (about flush failures) in upcoming commits. CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665 and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but previously these functions only returned Result values themselves, and did not call other functions that return Result values. Now, some functions they are calling will also return Result values, so refactor these functions to use result.Update so they can merge results and return complete error and warning messages.

- "Valid snapshot file, but referencing a snapshot block that turns out to be invalid, or has an invalid parent" has been addressed in bitcoin#30267 - "An ancestor of snapshot block" - If chain tip refers to blocks in this context then any successful load is addressing this because if we have synced past the snapshot base block we fail because we don't need assumeutxo anymore. And if this is about headers then this is the `test_headers_not_synced()` case. - "A descendant of the snapshot block" - If this refers to blocks the `test_snapshot_with_less_work()` addressed this and if it is just headers in this case again it would be represented in all of the successful loads in the test. Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>

Use result.Update in CompleteChainstateInit() and ChainstateManager::ActivateSnapshot() so it is possible for them to return warning messages (about flush failures) in upcoming commits. CompleteChainstateInit() was previously changed to use util::Result in bitcoin#25665 and ChainstateManager::ActivateSnapshot() was changed to use it in bitcoin#30267, but previously these functions only returned Result values themselves, and did not call other functions that return Result values. Now, some functions they are calling will also return Result values, so refactor these functions to use result.Update so they can merge results and return complete error and warning messages.

Summary: > rpc: Fix race in loadtxoutset > > The tip may have advanced, also if it did not, there is no reason to > have two variables point to the same block. bitcoin/bitcoin@fa91089 The potential race just means that tip_hash in the RPC response could be different from "the hash of the base of the snapshot" (as documented in the RPC doc). Note that this race cannot really happen in the functional test, as there are no blocks coming over the network during the loadtxoutset call (blocks are mined locally). It could have happened in the wild, with very limited impact. The node would keep working fine, but some users may have been confused by the RPC response. > refactor: Move early loadtxoutset checks into ActiveSnapshot > > Also changes the return type of ActiveSnapshot to allow returning the > error message to the user of the loadtxoutset RPC. bitcoin/bitcoin@80315c0 > refactor: Use named arguments to get path arg in loadtxoutset > > rpc: Use untranslated error strings in loadtxoutset [[bitcoin/bitcoin#30395 | core#30395]] This is a partial backport of [[bitcoin/bitcoin#29262 | core#29262]], [[bitcoin/bitcoin#30267 | core#30267]] and [[bitcoin/bitcoin#30395 | core#30395]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18055

Summary: Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com> This is a partial backport of [[bitcoin/bitcoin#30267 | core#30267]] bitcoin/bitcoin@80315c0 Depends on D18055 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18056

This was referenced Jun 11, 2024

Assumeutxo: bugfix on loadtxoutset with a divergent chain + test #29996

Merged

AssumeUTXO Mainnet Readiness Tracking #29616

Closed

tdb3 approved these changes Jun 11, 2024

View reviewed changes

test/functional/feature_assumeutxo.py Outdated Show resolved Hide resolved

alfonsoromanz approved these changes Jun 12, 2024

View reviewed changes

BrandonOdiwuor reviewed Jun 12, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

mzumsande reviewed Jun 13, 2024

View reviewed changes

fjahr force-pushed the 2024-06-invalid-snapshot-block branch from cbc604f to 1a6f1ff Compare June 18, 2024 12:57

DrahtBot added the CI failed label Jun 18, 2024

DrahtBot mentioned this pull request Jun 18, 2024

refactor: Improve assumeutxo state representation #30214

Open

fjahr force-pushed the 2024-06-invalid-snapshot-block branch 2 times, most recently from 605586c to fe25b9b Compare June 19, 2024 19:42

refactor: Move early loadtxoutset checks into ActiveSnapshot

80315c0

Also changes the return type of ActiveSnapshot to allow returning the error message to the user of the loadtxoutset RPC.

fjahr force-pushed the 2024-06-invalid-snapshot-block branch from fe25b9b to 750f8b5 Compare June 19, 2024 20:32

DrahtBot removed the CI failed label Jun 19, 2024

This was referenced Jun 20, 2024

kernel, refactor: return error status on all fatal errors #29700

Draft

scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] #29641

Draft

mzumsande reviewed Jun 20, 2024

View reviewed changes

DrahtBot requested review from tdb3, BrandonOdiwuor and alfonsoromanz June 20, 2024 18:51

fjahr and others added 2 commits June 21, 2024 10:39

assumeutxo: Check snapshot base block is not marked invalid

19ce3d4

Co-authored-by: Alfonso Roman Zubeldia <alfonsoromanz24@gmail.com>

test: Remove unnecessary restart in assumeutxo test

2f9bde6

fjahr force-pushed the 2024-06-invalid-snapshot-block branch from 750f8b5 to 2f9bde6 Compare June 21, 2024 08:39

mzumsande mentioned this pull request Jun 21, 2024

assumeutxo: Don't load a snapshot if it's not in the best header chain #30320

Merged

ryanofsky mentioned this pull request Jul 9, 2024

assumeutxo: Remove translated strings added in 30267 #30417

Closed

bitcoin locked and limited conversation to collaborators Jul 9, 2025

assumeutxo: Check snapshot base block is not in invalid chain #30267

assumeutxo: Check snapshot base block is not in invalid chain #30267

Uh oh!

Conversation

fjahr commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande Jun 13, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

mzumsande Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

fjahr Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 18, 2024

Uh oh!

fjahr commented Jun 19, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jun 21, 2024

Uh oh!

mzumsande commented Jun 21, 2024

Uh oh!

Uh oh!

fjahr commented Jun 11, 2024 •

edited

Loading

DrahtBot commented Jun 11, 2024 •

edited

Loading

ryanofsky Jun 17, 2024 •

edited

Loading

mzumsande Jun 20, 2024 •

edited

Loading