Fixes #30514 which has more context and shows how the issue can be reproduced. Since the value in question is removed, there is no test to add to reproduce anything.
This is an alternative approach to #30516 with much of the code being suggested there.
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 | maflcko, theStack, ismaelsadeeq, mzumsande, achow101 |
| Concept ACK | TheCharlatan, BrandonOdiwuor |
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28432306724
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Well, I wrote the code, but for clarity:
review ACK 6a020f019f709b595b68516bb3772b811a962e7 👌
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 6a020f019f709b595b68516bb3772b811a962e7 👌
3jS2HHppPfD+tzX6e9AslIRHlJ8EUjBeobC1IScJiN3mMyocR35ATxpOKXOZ7fJWm5OHSJeknBuKINW2mw3ecBA==
I have a slight preference for keeping the block height for informative purposes
Ditto. If the user tries to load a snapshot during header sync, we can give an indication of how long they have to wait. If we’re already past the expected height, we can say something useful about potential forks.
I’ll read up on the discussion.
Update: longer explanation of why I prefer to keep the height in the metadata: #30514 (comment)
27@@ -28,16 +28,17 @@ class Chainstate;
28 namespace node {
29 //! Metadata describing a serialized version of a UTXO set from which an
30 //! assumeutxo Chainstate can be constructed.
31+//! All metadata fields come from an untrusted file, so must be validated
ShapShotMetadata version was also bumped.
Concept ACK
The block height should also be removed from the SnapshotMetadata constructor and the utxo_snapshot fuzz target.
Concept ACK, now that #30516 is history.
While I still think having the block height in the meta-data could have potentially been useful for external tooling, not having it is also not terrible: it will be very likely somehow included in the filename of popular distributed snapshots (so users can at least see it), and for tools that don’t have access to the block index, it can at least be implicitly determined by tracking the maximum of all UTXO entries’ height values (I’ll use that approach in #27432 to print out the snapshot’s height at least at the end of the conversion, since after this PR gets merged that information is not available anymore before processing all coins).
The Snapshot format version is updated to 2 to indicate this change.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>The block height should also be removed from the
SnapshotMetadataconstructor and theutxo_snapshotfuzz target.
Thanks, I fixed that.
The CI failure is known and can be ignored (https://github.com/bitcoin/bitcoin/actions/runs/10310090865/job/28541115356?pr=30598#step:27:229)
Only change since my last review is a small style-only cleanup to remove the unused parameter from the constructor.
re-ACK 00618e8745192d209c23e3ae873c077e58168957 🎌
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 00618e8745192d209c23e3ae873c077e58168957 🎌
3mb+RYeTncQeKoOZTwq6wWc8zuxvPLDxuqiCpdNP9BdIYw2BYD3vbd9H9TvenfMARnJJ41hGFM3eLSYjeI5PtBQ==
32+//! before being used. Thus, new fields should be added only if needed.
33 class SnapshotMetadata
34 {
35- const uint16_t m_version{1};
36- const std::set<uint16_t> m_supported_versions{1};
37+ inline static const uint16_t VERSION{2};
m_version? That seems incompatible with the idea of being able to load older versions. Only when creating a snapshot it makes sense to hardcore one version (since we can also create older version snapshots using a previous release if really needed).
I would be fine with keeping the version at 1
What is the benefit? An unsigned 16-bit integer should have more than enough versions for any need, so incrementing is close to 0-cost, with the minimal benefit that a testing person who happens to have the old metadata will get a nice error message. Even if there was a shortage at some point, re-using 1 at that point in time should be trivially possible for the reasons given by you.
In any case, I am happy to ack either version of VERSION.
Also why drop m_version?
The field isn’t dropped. It is simply renamed from m_version to VERSION, keeping everything else the same.
If there is a need to load older versions in the future, the code can be written then. Also, writing such code seems unrelated to this pull request.
inline can also be applied to variables since C++17 👀
Re-ACK 00618e8745192d209c23e3ae873c077e58168957
It looks like C.I failure is unrelated as this diff is the last push.
New torrent (untested and seeding might take a while to kick in): magnet:?xt=urn:btih:596c26cc709e213fdfec997183ff67067241440c&dn=utxo-840000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
Update: I’m able to load the snapshot (and also checked that a version 1 snapshot is rejected)
97@@ -98,21 +98,20 @@ def expected_error(msg):
98 bogus_block_hash = "0" * 64 # Represents any unknown block hash
99 # The height is not used for anything critical currently, so we just
