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

alfonsoromanz · 2024-04-29T15:13:24Z

This PR adds a test to cover the scenario of loading an assumeutxo snapshot when the current chain tip is not an ancestor of the snapshot block but has less work.

During the review process, a bug was discovered where blocks between the last common ancestor and the background tip were not being requested if the background tip was not an ancestor of the snapshot block. mzumsande suggested a fix (65343ec) to start downloading historical blocks from the last common ancestor to address this issue. This fix has been incorporated into the PR with a slight modification.

Related to #28648

DrahtBot · 2024-04-29T15:13:27Z

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	fjahr, mzumsande, achow101
Concept ACK	naiyoma
Stale ACK	rkrux

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:

#30403 (test, assumeutxo: Remove resolved todo comments and add new test by fjahr)
#30320 (assumeutxo: Don't load a snapshot if it's not in the best header chain by mzumsande)

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.

alfonsoromanz · 2024-04-30T19:25:18Z

I splitted the original commit into two commits (one for each test). The second test (af0f401) may be redundant with this one: #29428. The only difference is that my test is executed on a node that has a divergent chain after block 199. I did that to cover this scenario described in the comments

[...] Loading a snapshot when the current chain tip is: [...] Not an ancestor or a descendant of the snapshot block and has more work

If it's redundant I can delete the second commit. Thoughts?

naiyoma · 2024-04-30T22:05:06Z

Concept ACK, covers more tests scenarios for AssumeUTXO

test/functional/feature_assumeutxo.py

rkrux

tACK ce80f7e

Make and tests successful, thanks for adding this test - it's easy to follow through.

test/functional/feature_assumeutxo.py

alfonsoromanz · 2024-05-13T15:18:56Z

Rebased. I also addressed some feedback from rkrux's comments: changed hardcoded value of nblocks from 99 to a dynamic value, i.e SNAPSHOT_BASE_HEIGHT-START_HEIGHT-1

naiyoma · 2024-05-16T20:43:23Z

Test passes for all three scenarios:

a7501f7 loading a snapshot when chain tip isn't ancestor but has less work
3f03354 loading a snapshot when chain tip isn't ancestor/descendant, has more work and A valid snapshot file & snapshot block, but the block is not on the most-work chain

naiyoma · 2024-05-16T21:17:14Z

I splitted the original commit into two commits (one for each test). The second test (af0f401) may be redundant with this one: #29428. The only difference is that my test is executed on a node that has a divergent chain after block 199. I did that to cover this scenario described in the comments

[...] Loading a snapshot when the current chain tip is: [...] Not an ancestor or a descendant of the snapshot block and has more work

If it's redundant I can delete the second commit. Thoughts?

IMO you should not delete the second commit, both tests verify that the snapshot load fails but under different scenarios

test/functional/feature_assumeutxo.py

ryanofsky · 2024-06-11T13:49:51Z

This PR adds tests to cover two scenarios of loading a snapshot when the current chain tip is:

Not an ancestor of the snapshot block but has less work

Not an ancestor or a descendant of the snapshot block and has more work

In the second scenario, the snapshot block does not belong to the most-work chain anymore so I believe it covers this scenario too: TODO: Valid snapshot file and snapshot block, but the block is not on the most-work chain. Therefore I deleted that TODO as well.

I'm not sure about this last part. It should be possible for the current chain tip to have more work than the snapshot block, which is on a different chain, and for the chain the snapshot block is on to ultimately be the most-work chain. Maybe this is not an interesting scenario to test though, since it involves a later-reorg. I'm not sure.

alfonsoromanz · 2024-06-12T07:16:54Z

This PR adds tests to cover two scenarios of loading a snapshot when the current chain tip is:

Not an ancestor of the snapshot block but has less work

Not an ancestor or a descendant of the snapshot block and has more work

In the second scenario, the snapshot block does not belong to the most-work chain anymore so I believe it covers this scenario too: TODO: Valid snapshot file and snapshot block, but the block is not on the most-work chain. Therefore I deleted that TODO as well.

I'm not sure about this last part. It should be possible for the current chain tip to have more work than the snapshot block, which is on a different chain, and for the chain the snapshot block is on to ultimately be the most-work chain. Maybe this is not an interesting scenario to test though, since it involves a later-reorg. I'm not sure.

Yes, that makes sense. I am not sure what exact scenario the comment refers to. If my statement is not correct, I can undo the comment deletion.

alfonsoromanz · 2024-06-27T08:36:06Z

Rearranging the order of the commits to make it easier to test the bugfix: the first test (53f714d) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix (65343ec).

I also updated the PR title and description to reflect that this PR is not a test-only PR anymore

mzumsande · 2024-06-28T15:59:06Z

Rearranging the order of the commits to make it easier to test the bugfix: the first test (53f714d) can be run without the bugfix and is expected to fail, but it should succeed after applying the fix (65343ec).

I'd suggest to revert that: The CI should pass on each commit, we even have a designated "test each commit" job that checks this (and now obviously fails, see below). It's ok if reviewers have to rearrange commits locally for testing.

Would also consider tweaking the fix to call LastCommonAncestor() before calling TryDownloadingHistoricalBlocks(), so it is easier to understand from_tip variable being the immediate predecessor of blocks to download next instead of having a more complicated meaning.

Have you considered addressing this suggestion by @ryanofsky above? Without having tried it out, it makes sense to me.

Otherwise, if the background tip is not an ancestor of the snapshot, blocks in between that ancestor up to the height of the background tip will never be requested. Co-authored-by: Martin Zumsande <mzumsande@gmail.com> Co-authored-by: Alfonso Roman Zubeldia <19962151+alfonsoromanz@users.noreply.github.com>

DrahtBot · 2024-06-29T12:09:43Z

🚧 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/26839553678}

alfonsoromanz · 2024-06-29T13:10:51Z

Thanks @mzumsande. I have reverted the change regarding the commit order and have also addressed the feedback from @ryanofsky by calling LastCommonAncestor() before TryDownloadingHistoricalBlocks().

… chain 2f9bde6 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr) 19ce3d4 assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr) 80315c0 refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr) Pull request description: 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. ACKs for top commit: mzumsande: re-ACK 2f9bde6 alfonsoromanz: Re-ACK 2f9bde6. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user. achow101: ACK 2f9bde6 Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4

fjahr

The first two commits look very good but I am not sure the third commit is still needed, but I am not completely sure yet and would like to hear what others think.

test/functional/feature_assumeutxo.py

alfonsoromanz · 2024-07-05T10:18:57Z

I removed the third commit (af0f401) since there is agreement that it is redundant with test_snapshot_with_less_work() (#29428). See discussion: #29996 (comment)

fjahr · 2024-07-05T19:20:42Z

tACK 5b7f70b

Reviewed the code and confirmed the test fails without the behavior change.

fjahr · 2024-07-08T17:23:06Z

nit: Could mention #28648 as related in the PR description.

mzumsande

Code Review ACK 5b7f70b

achow101 · 2024-07-10T19:05:54Z

ACK 5b7f70b

Summary: Otherwise, if the background tip is not an ancestor of the snapshot, blocks in between that ancestor up to the height of the background tip will never be requested. Test loadtxoutset in divergent chain with less work. Also guard against a m_mempool nullptr dereference which can happen in this situation, as the assumeutxo background chain does not maintain a mempool. Co-authored-by: Martin Zumsande <mzumsande@gmail.com> Co-authored-by: Alfonso Roman Zubeldia <19962151+alfonsoromanz@users.noreply.github.com> This is a backport of [[bitcoin/bitcoin#29996 | core#29996]] Depends on D17962 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D17966

DrahtBot added the Tests label Apr 29, 2024

fjahr mentioned this pull request Apr 29, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Closed

32 tasks

alfonsoromanz mentioned this pull request Apr 29, 2024

test: Assumeutxo: snapshots with less work should not be loaded #29428

Merged

DrahtBot mentioned this pull request Apr 29, 2024

test: loading assumeutxo snapshot start states #29681

Closed

alfonsoromanz force-pushed the assumeutxo_tests branch from 4c07a79 to af0f401 Compare April 30, 2024 19:15

kevkevinpal reviewed May 2, 2024

View reviewed changes

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

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

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

alfonsoromanz force-pushed the assumeutxo_tests branch from af0f401 to ce80f7e Compare May 2, 2024 11:34

DrahtBot mentioned this pull request May 7, 2024

test: added test coverage to loadtxoutset could not open file #30053

Merged

rkrux approved these changes May 8, 2024

View reviewed changes

DrahtBot added the Needs rebase label May 13, 2024

alfonsoromanz force-pushed the assumeutxo_tests branch from ce80f7e to 3f03354 Compare May 13, 2024 15:09

DrahtBot removed the Needs rebase label May 13, 2024

DrahtBot mentioned this pull request May 23, 2024

test: Added test to ensure log and failure happen when work is less than active chainstate #30105

Closed

DrahtBot added the Needs rebase label Jun 5, 2024

alfonsoromanz force-pushed the assumeutxo_tests branch from 3f03354 to 50cdb24 Compare June 6, 2024 09:57

maflcko reviewed Jun 6, 2024

View reviewed changes

test/functional/feature_assumeutxo.py Show resolved Hide resolved

DrahtBot removed the Needs rebase label Jun 6, 2024

fjahr mentioned this pull request Jun 11, 2024

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

Merged

fjahr reviewed Jun 11, 2024

View reviewed changes

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

fjahr reviewed Jun 11, 2024

View reviewed changes

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

fjahr reviewed Jun 11, 2024

View reviewed changes

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

alfonsoromanz force-pushed the assumeutxo_tests branch from 50cdb24 to 4b34a0c Compare June 12, 2024 06:35

fjahr mentioned this pull request Jun 20, 2024

RFC: Assumeutxo and large forks and reorgs #30288

Open

DrahtBot mentioned this pull request Jun 22, 2024

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

Merged

alfonsoromanz force-pushed the assumeutxo_tests branch from 5478d9c to 0a62076 Compare June 27, 2024 08:27

alfonsoromanz changed the title ~~test: Assumeutxo: import snapshot in a node with a divergent chain~~ Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests Jun 27, 2024

alfonsoromanz force-pushed the assumeutxo_tests branch from 0a62076 to 599e26e Compare June 29, 2024 11:53

alfonsoromanz force-pushed the assumeutxo_tests branch from 599e26e to 556ff68 Compare June 29, 2024 12:09

DrahtBot added the CI failed label Jun 29, 2024

DrahtBot removed the CI failed label Jun 29, 2024

fjahr reviewed Jul 4, 2024

View reviewed changes

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

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

test: loadtxoutset in divergent chain with less work

5b7f70b

alfonsoromanz force-pushed the assumeutxo_tests branch from 556ff68 to 5b7f70b Compare July 5, 2024 10:09

alfonsoromanz changed the title ~~Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests~~ Assumeutxo: bugfix on loadtxoutset with a divergent chain + test Jul 5, 2024

DrahtBot requested a review from rkrux July 5, 2024 19:20

This was referenced Jul 8, 2024

test, assumeutxo: Remove resolved todo comments and add new test #30403

Merged

test: Write assumeutxo tests #28648

Open

mzumsande reviewed Jul 8, 2024

View reviewed changes

achow101 merged commit 394651f into bitcoin:master Jul 10, 2024

ryanofsky mentioned this pull request Jul 19, 2024

refactor: Improve assumeutxo state representation #30214

Open

bitcoin locked and limited conversation to collaborators Jul 10, 2025

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

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

Uh oh!

Conversation

alfonsoromanz commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

alfonsoromanz commented Apr 30, 2024

Uh oh!

naiyoma commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alfonsoromanz commented May 13, 2024

Uh oh!

naiyoma commented May 16, 2024

Uh oh!

naiyoma commented May 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

alfonsoromanz commented Jun 12, 2024

Uh oh!

alfonsoromanz commented Jun 27, 2024

Uh oh!

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

Uh oh!

Uh oh!

DrahtBot commented Jun 29, 2024

Uh oh!

alfonsoromanz commented Jun 29, 2024

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alfonsoromanz commented Jul 5, 2024

Uh oh!

fjahr commented Jul 5, 2024

Uh oh!

fjahr commented Jul 8, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 10, 2024

Uh oh!

Uh oh!

alfonsoromanz commented Apr 29, 2024 •

edited

Loading

DrahtBot commented Apr 29, 2024 •

edited

Loading

naiyoma commented Apr 30, 2024 •

edited

Loading

ryanofsky commented Jun 11, 2024 •

edited

Loading

mzumsande commented Jun 28, 2024 •

edited

Loading