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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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)
ACK 9d5a89509031ce924b9685a6e2e4b19cfef50a9e Double checking #30403 (comment) would also be good as well.
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
Oh, sorry @BrandonOdiwuor , I missed #29681 😞
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
Done
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'])
nit: Would this be less than or equal to? (equal to in this case).
I think it's fine to keep the function name as is. The comment (now a log) provides the necessary context and I think it's just a special edge case of the less work behavior.
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.
I think it's a bit too verbose for the log but I added a comment. And I think it will be confusing to explicitly say everywhere in the normal case that the tip is an ancestor of the snapshot. But it's documented in one place now at least.
Yup, seems like its the most likely use case for assumeutxo, so I could see how that could be quite verbose.
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
Nit: Missing a "be", i.e. "which should be the normal case." Not sure if it's worth fixing, but just wanted to point it out
fixed, thanks!
Re ACK e523bcfc3403700debe1f286fa8c3db2d347e40b
Rebased
ACK e1018672672f39910655ab37080bf3213ca55a39
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 #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.
Rebased and since this is the last of the PRs addressing the TODOs comment in 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
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 https://github.com/bitcoin/bitcoin/issues/30288
ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1
left a nit (possibly a new test idea, but this can be done later).
Re ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1
ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1