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

fjahr · 2024-07-06T16:11:21Z

The first commit removes three Todos that have been addressed previously (see commit message for details).

The second message resolves another todo by adding the missing test case. This is a special case of "the tip has more work than the snapshot" where the tip is the same block as the snapshot base block.

Related to #28648.

DrahtBot · 2024-07-06T16:11:24Z

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

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:

#30497 (rpc: Return errors in loadtxoutset that currently go to logs 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.

alfonsoromanz · 2024-07-06T16:52:48Z

ACK 9d5a89509031ce924b9685a6e2e4b19cfef50a9e

I wonder if this is a good place to discuss other (potentially addressed) TODOs?

For example, interesting starting states could be loading a snapshot when the current chain tip is:

- TODO: An ancestor of snapshot block: isn't this already addressed when successfully loading the snapshot into node1 here? Node1's chain tip is at START_HEIGTH (199) before loading the snapshot, and the block 199 is an ancestor of the snapshot block (299)

- TODO: A descendant of the snapshot block: isn't this addressed by the function test_snapshot_with_less_work? (link). Node0 is at FINAL_HEIGHT (399) with the tip being a descendant of the snapshot block (at 299)

tdb3

ACK 9d5a89509031ce924b9685a6e2e4b19cfef50a9e
Double checking #30403 (comment) would also be good as well.

fjahr · 2024-07-08T11:13:34Z

I wonder if this is a good place to discuss other (potentially addressed) TODOs?

Right, good idea. I avoided this for a while because I thought I might miss something because I misinterpret them but since we had these detailed discussions in #29996 I am now pretty confident we can remove some that are addressed.

For example, interesting starting states could be loading a snapshot when the current chain tip is:

- TODO: An ancestor of snapshot block: isn't this already addressed when successfully loading the snapshot into node1 here? Node1's chain tip is at START_HEIGTH (199) before loading the snapshot, and the block 199 is an ancestor of the snapshot block (299)

I think this is addressed and I am also unsure if this was ever not addressed :) Elsewhere I realized that I am not sure if "chain tip" in this comment refers to sync of headers or of blocks. But either way: If this is about blocks 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.

- TODO: A descendant of the snapshot block: isn't this addressed by the function test_snapshot_with_less_work? (link). Node0 is at FINAL_HEIGHT (399) with the tip being a descendant of the snapshot block (at 299)

Yeah, I agree and if it is just headers in this case again it would be represented in all of the successful loads in the test.

fjahr · 2024-07-08T11:16:14Z

I have amended the commit to remove the previously named comments + made @alfonsoromanz co-author.

In the second commit I now resolve one of the few remaining cases where the tip is on the same block as the snapshot. This is a special case of the "more work" case so this was quite simple to do.

I think this means with this and the remaining open test PRs from @alfonsoromanz and @mzumsande we could resolve this list completely :)

fjahr · 2024-07-08T11:27:16Z

To give more detail where they are resolved: I think #30320 resolves two of the remaining three, it's just not part of the PR yet, see: #30320 (comment) and #29996 removes the third as already done in the commit here: 5b7f70b.

The last one to get merged gets to remove the whole comment section 🥳

alfonsoromanz

Re ACK 29d19c414c9e138c99f8de84e398528db92ff07e. The test execution is successful.

I agree that this PR, along with #30320 and #29996, would resolve the TODO list completely.

Thanks!

BrandonOdiwuor

Code Review ACK 29d19c414c9e138c99f8de84e398528db92ff07e

I will close PR #29681 in favor of this

fjahr · 2024-07-08T16:38:21Z

Oh, sorry @BrandonOdiwuor , I missed #29681 😞

tdb3

ACK 29d19c414c9e138c99f8de84e398528db92ff07e

Left some nits.

test/functional/feature_assumeutxo.py

alfonsoromanz

Re ACK e523bcfc3403700debe1f286fa8c3db2d347e40b

test/functional/feature_assumeutxo.py

fjahr · 2024-07-10T22:29:41Z

Rebased

tdb3

ACK e1018672672f39910655ab37080bf3213ca55a39

alfonsoromanz

Re ACK e1018672672f39910655ab37080bf3213ca55a39

- "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 <[email protected]>

Also pulls out the guarding assert and calls it explicitly before the test function is called. This is already done before the existing call of the test function so it was not needed there.

fjahr · 2024-07-18T23:57:05Z

Rebased and since this is the last of the PRs addressing the TODOs comment in feature_assumeutxo.py, it now removes that whole section.

jrakibi · 2024-07-20T11:34:04Z

ACK d63ef73

maflcko

ACK d63ef73

left a nit (possibly a new test idea, but this can be done later).

maflcko · 2024-07-23T09:07:34Z

test/functional/feature_assumeutxo.py

- TODO: Valid snapshot file, but referencing a snapshot block that turns out to be
-      invalid, or has an invalid parent


nit: I think that test in #30267 only checks for a known-invalid header, not for a valid header of a block that later "turns out to be invalid". Also, a test could be added where a block later turns out to have less work, but this is discussed in #30288

alfonsoromanz

Re ACK d63ef73

achow101 · 2024-07-23T17:24:54Z

ACK d63ef73

DrahtBot added the Tests label Jul 6, 2024

fjahr mentioned this pull request Jul 6, 2024

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

Merged

tdb3 approved these changes Jul 6, 2024

View reviewed changes

fjahr force-pushed the 2024-07-au-todo branch from 9d5a895 to 29d19c4 Compare July 8, 2024 11:13

fjahr changed the title ~~test: Remove already resolved assumeutxo todo comment~~ test, assumeutxo: Remove resolved todo comments and add new test Jul 8, 2024

fjahr mentioned this pull request Jul 8, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Closed

32 tasks

alfonsoromanz approved these changes Jul 8, 2024

View reviewed changes

DrahtBot requested a review from tdb3 July 8, 2024 12:27

BrandonOdiwuor approved these changes Jul 8, 2024

View reviewed changes

BrandonOdiwuor mentioned this pull request Jul 8, 2024

test: loading assumeutxo snapshot start states #29681

Closed

fjahr mentioned this pull request Jul 8, 2024

test: Write assumeutxo tests #28648

Open

This was referenced Jul 9, 2024

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

Merged

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

Merged

tdb3 approved these changes Jul 10, 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

fjahr force-pushed the 2024-07-au-todo branch from 29d19c4 to e523bcf Compare July 10, 2024 08:54

alfonsoromanz approved these changes Jul 10, 2024

View reviewed changes

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

DrahtBot requested review from BrandonOdiwuor and tdb3 July 10, 2024 13:07

fjahr force-pushed the 2024-07-au-todo branch from e523bcf to 3dc175e Compare July 10, 2024 14:39

DrahtBot added the Needs rebase label Jul 10, 2024

fjahr force-pushed the 2024-07-au-todo branch from 3dc175e to e101867 Compare July 10, 2024 22:28

DrahtBot removed the Needs rebase label Jul 10, 2024

tdb3 approved these changes Jul 11, 2024

View reviewed changes

DrahtBot requested a review from alfonsoromanz July 11, 2024 00:48

alfonsoromanz approved these changes Jul 11, 2024

View reviewed changes

DrahtBot added the Needs rebase label Jul 18, 2024

fjahr and others added 2 commits July 19, 2024 00:54

test: Add loadtxoutset test with tip on snapshot block

d63ef73

Also pulls out the guarding assert and calls it explicitly before the test function is called. This is already done before the existing call of the test function so it was not needed there.

fjahr force-pushed the 2024-07-au-todo branch from e101867 to d63ef73 Compare July 18, 2024 23:55

DrahtBot removed the Needs rebase label Jul 19, 2024

DrahtBot requested review from alfonsoromanz and tdb3 July 20, 2024 11:34

DrahtBot mentioned this pull request Jul 22, 2024

rpc: Return errors in loadtxoutset that currently go to logs #30497

Merged

maflcko approved these changes Jul 23, 2024

View reviewed changes

alfonsoromanz approved these changes Jul 23, 2024

View reviewed changes

achow101 merged commit 8ae79f1 into bitcoin:master Jul 23, 2024

bitcoin locked and limited conversation to collaborators Jul 23, 2025

		- TODO: Valid snapshot file, but referencing a snapshot block that turns out to be
		invalid, or has an invalid parent

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DrahtBot commented Jul 6, 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 Jul 6, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jul 8, 2024

Uh oh!

fjahr commented Jul 8, 2024

Uh oh!

fjahr commented Jul 8, 2024

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment • 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 commented Jul 8, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjahr commented Jul 10, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jul 18, 2024

Uh oh!

jrakibi commented Jul 20, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fjahr commented Jul 6, 2024 •

edited

Loading

DrahtBot commented Jul 6, 2024 •

edited

Loading

BrandonOdiwuor left a comment •

edited

Loading