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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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)
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 atSTART_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 functiontest_snapshot_with_less_work? (link). Node0 is atFINAL_HEIGHT(399) with the tip being a descendant of the snapshot block (at299)
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.
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 :)
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 (review) and #29996 removes the third as already done in the commit here: https://github.com/bitcoin/bitcoin/pull/29996/commits/5b7f70ba2661a194a3c476b81e03785feddb4e1c.
The last one to get merged gets to remove the whole comment section 🥳
Code Review ACK 29d19c414c9e138c99f8de84e398528db92ff07e
I will close PR #29681 in favor of this
260@@ -267,6 +261,12 @@ def run_test(self):
261 self.log.info(f"Creating a UTXO snapshot at height {SNAPSHOT_BASE_HEIGHT}")
262 dump_output = n0.dumptxoutset('utxos.dat')
263
264+ # Test special case where the tip of the node is on the same block as
265+ # the snapshot
nit: Would be good to make this comment a self.log.info() message instead.
https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#general-test-writing-advice
Instead of inline comments or no test documentation at all, log the comments to the test log
260@@ -267,6 +261,12 @@ def run_test(self):
261 self.log.info(f"Creating a UTXO snapshot at height {SNAPSHOT_BASE_HEIGHT}")
262 dump_output = n0.dumptxoutset('utxos.dat')
263
264+ # Test special case where the tip of the node is on the same block as
265+ # the snapshot
266+ assert_equal(n0.getblockcount(), SNAPSHOT_BASE_HEIGHT)
267+ assert_equal(n0.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT)
268+ self.test_snapshot_with_less_work(dump_output['path'])
18 - TODO: Valid snapshot file and snapshot block, but the block is not on the
19 most-work chain
20
21 Interesting starting states could be loading a snapshot when the current chain tip is:
22
23-- TODO: An ancestor of snapshot block
-TODO: An ancestor of snapshot block: isn’t this already addressed when successfully loading the snapshot into node1 here?
nit: Would be good to be more explicit in the log message near the line (or add one more) to document for future readers what the test covers. This way if there are changes in the future, the changes don’t accidentally remove case coverage.
ACK 29d19c414c9e138c99f8de84e398528db92ff07e
Left some nits.
305@@ -307,6 +306,8 @@ def run_test(self):
306 self.test_snapshot_block_invalidated(dump_output['path'])
307
308 self.log.info(f"Loading snapshot into second node from {dump_output['path']}")
309+ # This node's tip is on an ancestor block of the snapshot, which should
310+ # the normal case
- "Valid snapshot file, but referencing a snapshot block that turns out
to be invalid, or has an invalid parent" has been addressed in #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>
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.
feature_assumeutxo.py, it now removes that whole section.
10@@ -11,16 +11,9 @@
11
12 ## Possible test improvements
13
14-Interesting test cases could be loading an assumeutxo snapshot file with:
15-
16-- TODO: Valid snapshot file, but referencing a snapshot block that turns out to be
17- invalid, or has an invalid parent
ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1
left a nit (possibly a new test idea, but this can be done later).
