assumeutxo: Drop block height from metadata #30598

fjahr · 2024-08-06T22:27:13Z

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.

DrahtBot · 2024-08-06T22:27:16Z

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	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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30214 (refactor: Improve assumeutxo state representation by ryanofsky)

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.

DrahtBot · 2024-08-06T22:50:30Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/28432306724}

Hints

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.

TheCharlatan · 2024-08-07T07:47:52Z

Concept ACK

maflcko · 2024-08-07T10:47:18Z

Well, I wrote the code, but for clarity:

review ACK 6a020f0 👌

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 6a020f019f709b595b68516bb3772b811a962e7 👌
jS2HHppPfD+tzX6e9AslIRHlJ8EUjBeobC1IScJiN3mMyocR35ATxpOKXOZ7fJWm5OHSJeknBuKINW2mw3ecBA==

theStack · 2024-08-07T11:52:57Z

As expressed in #30516 (comment), I have a slight preference for keeping the block height for informative purposes (showing a block height is just more user friendly than showing block hash, for external tooling) and just verifying it as well (see e.g. #30516 (comment)), but happy to review this PR if there is consensus that we don't need it.

Sjors · 2024-08-07T12:16:28Z

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)

BrandonOdiwuor

Concept ACK

fjahr · 2024-08-08T12:37:35Z

I have closed #30516 so we can move forward with the approach here. Looking forward to get reviews, thanks!

ismaelsadeeq

Tested ACK 6a020f0

src/node/utxo_snapshot.h

mzumsande

Concept ACK

The block height should also be removed from the SnapshotMetadata constructor and the utxo_snapshot fuzz target.

theStack · 2024-08-08T17:45:17Z

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>

fjahr · 2024-08-08T21:57:41Z

The block height should also be removed from the SnapshotMetadata constructor and the utxo_snapshot fuzz target.

Thanks, I fixed that.

maflcko · 2024-08-09T05:59:05Z

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 00618e8 🎌

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 00618e8745192d209c23e3ae873c077e58168957 🎌
mb+RYeTncQeKoOZTwq6wWc8zuxvPLDxuqiCpdNP9BdIYw2BYD3vbd9H9TvenfMARnJJ41hGFM3eLSYjeI5PtBQ==

Sjors · 2024-08-09T07:50:38Z

src/node/utxo_snapshot.h

 class SnapshotMetadata
 {
-    const uint16_t m_version{1};
-    const std::set<uint16_t> m_supported_versions{1};
+    inline static const uint16_t VERSION{2};


I would be fine with keeping the version at 1 since this was never released, the old torrent will probably disappear, and the change here doesn't have backwards compatible support.

Also why drop 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.

I will leave this unaddressed for now since either way seems ok and we have 3 acks.

theStack

Code-review ACK 00618e8

theStack · 2024-08-09T08:39:57Z

src/node/utxo_snapshot.h

 class SnapshotMetadata
 {
-    const uint16_t m_version{1};
-    const std::set<uint16_t> m_supported_versions{1};
+    inline static const uint16_t VERSION{2};


TIL that inline can also be applied to variables since C++17 👀

Yeah, this fixed https://github.com/bitcoin/bitcoin/runs/28432306724

ismaelsadeeq

Re-ACK 00618e8

It looks like C.I failure is unrelated as this diff is the last push.

Sjors · 2024-08-09T13:01:30Z

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)

mzumsande

ACK 00618e8

mzumsande · 2024-08-09T18:43:29Z

test/functional/feature_assumeutxo.py

@@ -98,21 +98,20 @@ def expected_error(msg):
        bogus_block_hash = "0" * 64  # Represents any unknown block hash
        # The height is not used for anything critical currently, so we just


nit: could remove comment

Done as part of #30624

achow101 · 2024-08-09T20:03:42Z

ACK 00618e8

This was fixed in commit b50554b. Also, address the typo nits from: * bitcoin#29370 (comment) * bitcoin#30598 (comment)

fa04511 doc: Remove outdated nTx faking comment (MarcoFalke) Pull request description: This problematic `nTx` "faking" was removed in commit b50554b. So fix the wrong comment. Also, address the typo nits from: * #29370 (comment) * #30598 (comment) ACKs for top commit: fjahr: ACK fa04511 Tree-SHA512: c918f0b9274be9c347a37d6221915688977a858fb8d2146035718bf17df0bd3b51d67ef12b971556851c0f69f46d26f557c35a5461abeb0683b538b9dc48f5b6

This was fixed in commit b50554babdddf452acaa51bac757736766c70e81. Also, address the typo nits from: * bitcoin/bitcoin#29370 (comment) * bitcoin/bitcoin#30598 (comment)

Summary: Removing the block height from the assumeutxo snapshot file prevents potential UB (implicit-integer-sign-change) when loading the file. The snapshot file should be considered untrusted input, and its fields should be sanitized or removed if unnecessary. The Snapshot format version is updated to 2 to indicate this change. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> This is a backport of [[bitcoin/bitcoin#30598 | core#30598]] Depends on D18059 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18060

fjahr mentioned this pull request Aug 6, 2024

Assumeutxo: Sanitize block height in metadata #30516

Closed

fjahr force-pushed the 2024-08-au-drop-height branch from f211e8f to 6a020f0 Compare August 6, 2024 22:50

DrahtBot added the CI failed label Aug 6, 2024

DrahtBot removed the CI failed label Aug 7, 2024

DrahtBot mentioned this pull request Aug 7, 2024

assumeUTXO: snapshot load, update VERSION msg height #30418

Closed

DrahtBot requested a review from TheCharlatan August 7, 2024 10:47

Sjors mentioned this pull request Aug 7, 2024

implicit-integer-sign-change in ActivateSnapshot #30514

Closed

BrandonOdiwuor reviewed Aug 7, 2024

View reviewed changes

This was referenced Aug 8, 2024

refactor: Improve assumeutxo state representation #30214

Open

kernel, refactor: return error status on all fatal errors #29700

Draft

fjahr mentioned this pull request Aug 8, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Closed

32 tasks

ismaelsadeeq approved these changes Aug 8, 2024

View reviewed changes

src/node/utxo_snapshot.h Show resolved Hide resolved

DrahtBot requested a review from BrandonOdiwuor August 8, 2024 13:21

achow101 added this to the 28.0 milestone Aug 8, 2024

mzumsande reviewed Aug 8, 2024

View reviewed changes

Sjors mentioned this pull request Aug 8, 2024

validation: assumeutxo params mainnet #28553

Merged

assumeutxo: Drop block height from metadata

00618e8

The Snapshot format version is updated to 2 to indicate this change. Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

fjahr force-pushed the 2024-08-au-drop-height branch from 6a020f0 to 00618e8 Compare August 8, 2024 21:57

DrahtBot requested review from ismaelsadeeq and mzumsande August 9, 2024 05:59

DrahtBot requested a review from theStack August 9, 2024 05:59

Sjors reviewed Aug 9, 2024

View reviewed changes

theStack approved these changes Aug 9, 2024

View reviewed changes

ismaelsadeeq reviewed Aug 9, 2024

View reviewed changes

mzumsande reviewed Aug 9, 2024

View reviewed changes

achow101 merged commit 9a69639 into bitcoin:master Aug 9, 2024
15 of 16 checks passed

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 10, 2024

doc: Remove outdated nTx faking comment

fa04511

This was fixed in commit b50554b. Also, address the typo nits from: * bitcoin#29370 (comment) * bitcoin#30598 (comment)

maflcko mentioned this pull request Aug 10, 2024

doc: Remove outdated nTx faking comment #30624

Merged

theStack mentioned this pull request Aug 10, 2024

contrib: add tool to convert compact-serialized UTXO set to SQLite database #27432

Merged

Sjors mentioned this pull request Aug 13, 2024

Mining interface: getCoinbaseMerklePath() and submitSolution() Sjors/bitcoin#53

Closed

bitcoin locked and limited conversation to collaborators Aug 10, 2025

		@@ -98,21 +98,20 @@ def expected_error(msg):
		bogus_block_hash = "0" * 64 # Represents any unknown block hash
		# The height is not used for anything critical currently, so we just

assumeutxo: Drop block height from metadata #30598

assumeutxo: Drop block height from metadata #30598

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Aug 6, 2024

Uh oh!

TheCharlatan commented Aug 7, 2024

Uh oh!

maflcko commented Aug 7, 2024

Uh oh!

theStack commented Aug 7, 2024

Uh oh!

Sjors commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Aug 8, 2024

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande 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!

theStack commented Aug 8, 2024

Uh oh!

fjahr commented Aug 8, 2024

Uh oh!

maflcko commented Aug 9, 2024

Uh oh!

Sjors Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 9, 2024 • 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!

maflcko Aug 9, 2024 • 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!

maflcko Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

fjahr Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

fjahr Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Aug 9, 2024

Choose a reason for hiding this comment

fjahr commented Aug 6, 2024 •

edited

Loading

DrahtBot commented Aug 6, 2024 •

edited

Loading

Sjors commented Aug 7, 2024 •

edited

Loading

mzumsande left a comment •

edited

Loading

Sjors Aug 9, 2024 •

edited

Loading

maflcko Aug 9, 2024 •

edited

Loading

Sjors commented Aug 9, 2024 •

edited

Loading