Assumeutxo: Sanitize block height in metadata #30516

Fixes #30514 which has more context.

The block height in the assumeutxo metadata was implicitly cast to int which could cause it to be interpreted as signed.

The first commit shows how to reproduce the error shown in #30514 and how it could be fixed with a minimal change. The second commit adds an explicit sanitizer that will probably be the preferred approach. I can squash the commits but first I would like to hear conceptual feedback since #30514 suggested to remove the block height instead.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

For detailed information about the code coverage, see the test coverage report.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #30598 (assumeutxo: Drop block height from metadata by fjahr)

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.

From the issue:

The additional information can be helpful to us in certain failure cases, especially if we discuss issues like in #30288 seriously.

How? I could see how situations described in #30288 would require to add the block hash in a hypothetical scenario where we only saved the height - but we are in the reverse scenario where we already have the block hash, so I can't really think of a scenario where haveing the block height would help. I also didn't find any discussion about the reason for the block height in #29612 on a quick glance (but didn't expand every comment).

How?

The height in the metadata clearly tells us which height the hash is expected at. Not having the same hash at that height clearly tells us that the node is on a different chain than was expected by the metadata. That is better than just saying "we don't know this hash for whatever reason".

I also didn't find any discussion about the reason for the block height in #29612 on a quick glance (but didn't expand every comment).

It was suggested here and when I added it along with the other suggested values, all reviewers ACK-ed it.

An alternative would be to write a full file-content hash in the metadata as the second field (after the file magic), covering the whole file after the second field. This way, the file-content hash would be retrieved from the trusted chainparams, then calculated and compared. Afterwards, the code can assume that all fields in the checked file are trusted to some extent, which would allow using the block height value.

Do you mean the serialized tx hash here? That's different from the file content hash. We do have the serialized hash in the trusted assumeutxo data but not the file hash. I don't think we want to have it in there because we want to force users of the data to calculate it themselves. And if there is a mismatch it's just the same as with the block height where it seems to be an issue for you that it may mismatch. Or are you arguing that the file hash should be added to the chainparams as well? It's unclear to me, honestly.

You'll have to check that the block height corresponds to the block hash, otherwise I fail to see how it can be used without adding bugs or risks or confusion. However, if you do the check, you may as well discard the deserialized value and just use the value from the check.

The block height is intended as additional information that can help users debug what the issue is, like the error message in the suggested change above where it is removed. It doesn't need to be checked for that. If the file is corrupt or the network is found in an unexpected state, it can be helpful as additional information as described above.

In an otherwise fully valid utxo snapshot dump, with a single bit flip in the block height field in the metadata, the error message for the user will not be: "Corrupt metadata, block height corrupt", but instead "A forked headers-chain with more work than the chain with the snapshot base block header exists. Please proceed to sync without AssumeUtxo."

I don't think it's fair criticism to say "this is confusing when I introduce a magic bitflip in this exact place". There are literally infinite places all over the code base where that is true and we won't get anywhere if this is the bar we need to meet. If I can just flip any bit in your block storage or utxo set, we are completely screwed and most importantly you probably wouldn't even see an error until it's too late while in the case discussed here, there is a failure right away (if we check) or there is no issue at all because the value is not used for anything critical as outlined before.

The arguments here don't convince me yet that the height really has to be dropped necessarily but I also seem to be less passionate about this issue than @maflcko et al. So I opened the suggested approach of removing the block height in #30598 and I pinged other reviewers of the original metadata PR to weigh in shortly.

Ultimately what matters most to me is that we finally get this feature into the hands of users and I am fine to go along with either approach if it avoids further blocking of that.

The height in the metadata clearly tells us which height the hash is expected at. Not having the same hash at that height clearly tells us that the node is on a different chain than was expected by the metadata. That is better than just saying "we don't know this hash for whatever reason".

Ah, ok, my point is that the height is implied by the block hash, together with the block index db of the node. If the block tree db doesn't contain a header with that block hash, then we abort anyway. If it contains the index but we don't have a registered snapshot for this height in the chainparams, we can look the attempted height up from the block index, e.g. for a user-facing error message. But it should be impossible to construct an alternative block tree db where a block with the given hash is at a different height (unless you can break sha256...) so providing the height doesn't add any information in my opinion.

Not sure if this is a convincing argument for anyone, but I can at least see a point of keeping the block height for purely informative purposes, in case the snapshot is processed by external tools which don't have access to the block index, e.g. #27432 (or, at some point even the https://linux.die.net/man/1/file utility, which can not only determine a file type, but also emit a short meta-data summary, if properly implemented in the magic file). Wouldn't it be an alternative to simply keep the format as-is, but use the block height only for an immediate sanity check in Bitcoin Core, i.e. throw if it doesn't match the included block hash?

The arguments here don't convince me yet that the height really has to be dropped necessarily but I also seem to be less passionate about this issue than @maflcko et al.

Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed. I haven't tried it, but #30516 (review) (the error message for the user will not be: "Corrupt metadata, block height corrupt", but instead "A forked headers-chain with more work than the chain with the snapshot base block header exists. Please proceed to sync without AssumeUtxo.") should still be reproducible on top of this branch currently.

If you keep the metadata, it should be renamed to m_base_blockheight_untrusted, so that new places don't use it by accident, like #30516 (review).

Other than that, I agree this is mostly a bike-shedding discussions whether or not external tools (or error messages) display the height and hash, or just the hash.

@maflcko: I think the simplest (and IMHO most obvious) solution would be to just verify the height as well, then all of the problems you described should be solved? E.g. with something like

diff --git a/src/validation.cpp b/src/validation.cpp
index d8c1c27aae..612bb960e5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5660,10 +5660,11 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
     {
         LOCK(::cs_main);
 
-        if (!GetParams().AssumeutxoForBlockhash(base_blockhash).has_value()) {
+        auto maybe_assumeutxo_data = GetParams().AssumeutxoForBlockhash(base_blockhash);
+        if (!maybe_assumeutxo_data.has_value() || maybe_assumeutxo_data->height != base_blockheight) {
             auto available_heights = GetParams().GetAvailableSnapshotHeights();
             std::string heights_formatted = util::Join(available_heights, ", ", [&](const auto& i) { return util::ToString(i); });
-            return util::Error{strprintf(Untranslated("assumeutxo block hash in snapshot metadata not recognized (hash: %s, height: %s). The following snapshot heights are available: %s"),
+            return util::Error{strprintf(Untranslated("assumeutxo block hash/height in snapshot metadata not recognized (hash: %s, height: %s). The following snapshot heights are available: %s"),
                 base_blockhash.ToString(),
                 base_blockheight,
                 heights_formatted)};

I haven't tried to compile that suggestion, but it'll probably fail -Wsign-compare. Also, the problem remains that future code may accidentally use it before it is checked (or after the check failed). So I'd say that a rename to m_base_blockheight_untrusted could still make sense. An alternative may be to move the metadata parsing into the ActivateSnapshot function itself (instead of parsing it outside and passing it to the function, exposing it to a greater scope than needed). Any metadata values that are validated, can then be returned in a struct from ActivateSnapshot, and hopefully it will be clearer to code readers that any values in the metadata need to be checked before they can be used?

I haven't tried to compile that suggestion, but it'll probably fail -Wsign-compare.

Indeed compiling this emits -Wsign-compare warning

<details> <summary>warning</summary>

Making all in src
  CXX      libbitcoin_node_a-validation.o
validation.cpp:5664:81: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare]
        if (!maybe_assumeutxo_data.has_value() || maybe_assumeutxo_data->height != base_blockheight){
                                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~
1 warning generated.
  AR       libbitcoin_node.a
  GEN      obj/build.h
  CXXLD    bitcoind
</details>

Tested this on master

Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed

Thanks, that was not clear to me indeed.

I haven't tried it, but #30516 (review) (the error message for the user will not be: "Corrupt metadata, block height corrupt", but instead "A forked headers-chain with more work than the chain with the snapshot base block header exists. Please proceed to sync without AssumeUtxo.") should still be reproducible on top of this branch currently.

I don't understand how we can fix that issue to work perfectly for all cases. IMO it is unfixable because we can not know if a value that doesn't match is corrupt or not a match. If there is a bitflip in the version field we will show an error message that the version doesn't match and not that the version field is corrupt. If there is a bitflip in the network magic we will show an error that the network doesn't match and not that the network magic is corrupt and so on...

A little recap so far: While I still don’t find the arguments against having the height included convincing I am currently leaning towards going with the approach in #30598 and removing it for now. My reasoning is as follows:

• While the arguments against may not be strong I conceive that it would be better if we had a clear plan for how we use the height which would give a stronger argument for keeping it in. We can re-add the height in a new version together with a clear plan of how it is going to be used.
• Removing the height is a much cleaner change. Due to the lack of a clear plan and the arguments exchanged here in the last ~8 hours it became evident to me that we might get stuck in discussions about how to handle all eventual cases with the approach of keeping the height, while in the removal PR there is not really anything to discuss because the contentious part is, well, removed.
• Looking at the engagement there seem to be more proponents for removal: Pro removal 3 explicit ACKs in the removal PR (@maflcko , @BrandonOdiwuor , @TheCharlatan ) and I am implying @mzumsande is also for removal based on his comments here (please confirm!). Pro keeping height are @theStack and @Sjors . (@ismaelsadeeq since you have started reviewing this change, could you please indicate if this is because you prefer this change?)

The primary downside to removing the height seems to be that we need to produce a new torrent but I hope @Sjors would be fine with that and I am currently the only reviewer of #28553 as well.

I will keep this PR here open for discussion for at least a few more hours at least since there may be some contributors in other timezones that were not able to comment yet. But if no new findings arise I will move forward with #30598 for reasons outlined above.

I don't mind making a new torrent.

I'm still puzzled why all of a sudden a single integer in a serialized file is such a big deal that removing it is simpler than fixing it. But I haven't reviewed this PR yet.

It's true that we can add it back later, but it seems wasteful if we first have to review #30598 only to revert it later and still having to review the changes here.

Even if we never want it back, it seems very likely we'll add an integer to some other serialized thing in the future. We should just be able to do that. But again, maybe there's something really special about the integer here that I missed.

Approach wise, it makes sense to me to have two commits.

Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed

This PR fixes the undefined behavior that the sanitizer picked up in #30514. I think that's sufficient for now. We can improve handling of corrupt and/or malicious files later, which (unless I missed something) is what @maflcko's critique is about.

Afaik any corrupt (accidentally or otherwise) snapshot file will ultimately be rejected by the node because height, block_hash, nChainTx or hash_serialized doesn't match the hardcode chainparams. That all happens before we activate a snapshot and start syncing to a potentially malicious tip. So worst case you're wasting ten minutes of user time, plus whatever time they need to deal with the confusing error message.

Assuming I didn't miss anything in my reasoning above:

ACK 51f197bd84916c494e9250926776b9efc3225100

Using 98e119da35c4f7e74f0c423974d497e350c8e9fa I was able to reproduce the test failure by undoing the change in validation.cpp - without the help of a sanitiser, so that's nice. I think it's useful to keep this commit.

For the second commit maybe add to the commit message:

Review hint: `--color-moved=dimmed-zebra`

Though it's easier if you change the first commit in a way that avoids this code churn in the second commit, i.e. by copy-pasting rather than expanding the loop.

This PR fixes the undefined behavior that the sanitizer picked up in #30514.

I don't think this is UB. Personally I couldn't care less about a implicit fuzz integer issue, and I don't think any production user cares either, because fuzz tests aren't run in production.

I don't see the value in only silencing the fuzz target, but leaving the other reported bugs unfixed. If silencing the fuzz target was a goal, it could be done with a trivial temporary one-line patch to the suppressions file. Once there is a real fix, the temporary suppression can be removed. I just don't see the point of changing real code temporarily, and then revisit it in a "follow-up". Either the reported bugs are worthy to fix, in which case they should be fixed, or they are not, in which case they shouldn't be fixed and this can just be replaced with a sanitizer suppression.

It doesn't seem like we can come to an agreement quickly here, so I am closing this in favor of #30598 so we can move forward with the mainnet params. I would appreciate your reviews there, thank you!