-
Notifications
You must be signed in to change notification settings - Fork 36.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
coinstats, assumeutxo: fix hash_serialized2 calculation #28685
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee 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. ConflictsReviewers, 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. |
6c0b7f1
to
0e744ce
Compare
0e744ce
to
0787af1
Compare
I've written a fuzz test that catches the original bug mentioned in #28675 (comment): https://github.com/dergoegge/bitcoin/blob/2023-10-fuzz-coinstats-hash/src/test/fuzz/coinstats_hash.cpp. I think it also caught a second issue regarding the values of UTXOs, certain negative values seem to result in the same hash as their positive counter part. I have not investigated further but I assume it is realted to $ echo "CMoC9Q==" | base64 --decode > coinstats_hash.crash
$ FUZZ_PRINTF=1 FUZZ=coinstats_hash ./src/test/fuzz/fuzz coinstats_hash.crash
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3663010564
INFO: Loaded 1 modules (379542 inline 8-bit counters): 379542 [0x560080cb5d88, 0x560080d1281e),
INFO: Loaded 1 PC tables (379542 PCs): 379542 [0x560080d12820,0x5600812dd180),
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: coinstats_hash_crashes/crash-d59c557ae7f9e837716b272a05d1397a94f589d7
2023-10-19T11:34:50Z Opened LevelDB successfully
2023-10-19T11:34:50Z Using obfuscation key for : 0000000000000000
2023-10-19T11:34:50Z mutating a coin
2023-10-19T11:34:50Z coinbase=1 height=1386179375 value=-5934962493229608344 scriptPubKey=[97951910994b0a6104092fbc9b385639]
2023-10-19T11:34:50Z coinbase=1 height=1386179375 value=6856422276773357632 scriptPubKey=[97951910994b0a6104092fbc9b385639]
fuzz: test/fuzz/coinstats_hash.cpp:135: void coinstats_muhash_fuzz_target(FuzzBufferType): Assertion `mutated_result->hashSerialized != result->hashSerialized' failed. Using the varints in the serialization for the hashes seems like unnecessary complexity in the first place. Since we are changing the hash format already, maybe we just get rid of the varints entirely? (I've not polished the fuzz test, sorry if you run into bugs in the test itself) |
cd0eefd
to
c46d1d9
Compare
@dergoegge Awesome to have a fuzz test already :) I think the issue with negative amounts is good to know but not as critical. Since negative output values are not consensus valid it seems ok for the serialization to assume that they are not negative. That's at least how I understand the issue right now. So I think in practice this should at least not lead to a different hash. I am unsure about removing VARINT from this code entirely. I agree that for the hashing it is not needed but we also use it when we dump the utxo set and there we probably don't want to get rid of it since it's saving space. So we still have to reason about it anyway and I think VARINT isn't really a problem in general. It's widely used in our code and I think also pretty well understood. But I won't fight it if people agree that this is a valuable change. Let's discuss this in the IRC meeting today! |
I think this should also change I don't see any value in keeping around a broken |
That's something worth considering IMHO, had similar thoughts. I assume the primary (probably only?) reason for using VARINTs in the hashing routine is that it's supposedly faster, as it leads to less total data that has to be processed by the underlying hash function (e.g. the vast majority of UTXO amounts can probably be encoded in 2-4 bytes if encoded as VARINTs, rather than needing the full 8 bytes). Would be interesting to benchmark how much difference in performance it really is, to judge if it's worth it to keep the VARINTs in the hashing code. |
Makes sense, added |
e403dc9
to
b4713c5
Compare
Couldn't an attacker distribute utxo sets with the negative values instead of the correct positive ones without it being detected at startup? This would then later lead to a consensus split when these outputs are spent. |
On that note, may not be a bad idea to add a negative-amount sanity check to |
Yeah, I thought somehow that this was enforced by |
b861737
to
9454f66
Compare
Concept ACK and approach ACK My only reservation here would be completely displacing Not to create more work, but I wonder if it's worth polling the mailinglist to see if there are existing consumers of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the fuzzer and it found one more bug (see comment inline): https://github.com/dergoegge/bitcoin/tree/2023-10-fuzz-coinstats-hash
The version of ApplyHash
on that branch no longer uses VARINT and looks much simpler imo.
Imo, we should fix all bugs that cause two different versions of a snapshot file to result in the same hash. Otherwise the assumeutxo check becomes "check that the hashes match and check for negative output values and check ..." instead of "check that the hashes match".
Here are some benchmark results of different branches that has been proposed so far.
The |
I have similar results as @theStack . Involvement of
|
Seems that you are right, I have tested this with the code here and I get similar performance as with master (m17.516s, 1m20.996s, 1m22.714s). |
I have just pushed another commit that breaks all the tests and chainparams that were previously fixed but I would like someone else to confirm my findings in terms of performance before I finish everything up, fixing the tests, squashing etc. This latest change aligns the serialization for muhash and hash_serialized which eliminates the @dergoegge could you also point your merciless fuzzer at this? |
Approach ACK - re-using the muhash serialization makes a lot of sense.
Done! Didn't find any more bugs. |
src/kernel/coinstats.cpp
Outdated
{ | ||
DataStream ss{}; | ||
ss << outpoint; | ||
ss << static_cast<uint32_t>(coin.nHeight * 2 + coin.fCoinBase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ss << static_cast<uint32_t>(coin.nHeight * 2 + coin.fCoinBase); | |
ss << static_cast<uint32_t>((coin.nHeight << 1) + coin.fCoinBase); |
This should avoid UBSan screaming at us in the future, e.g. kernel/coinstats.cpp:55:46: runtime error: signed integer overflow: 2147483636 * 2 cannot be represented in type 'int'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UBSan screaming
Wouldn't it scream equally about left shift of a signed integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it isn't complaining. nHeight
is also not signed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, I mixed it up with CBlockIndex::nHeight
. In that case, the potentially-narrowing cast isn't needed and a non-narrowing cast on the bool to unsigned should be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have adopted @dergoegge 's suggestion for now, let me know if you would prefer a different change though @maflcko
Re-did the benchmarks the current state of the PR (using MuHash serialization without the temporary
That's still a bit slower than on master (and alternative implementations), but IMHO it's not too bad to be a show-stopper for using the MuHash serialization. |
921db52
to
942f058
Compare
Cleaned everything up while using the MuHash serialization as discussed. The remaining tests are fixed and chainparams for testnet and signet updated. This currently does not preserve the old
I will write a post to the mailing list today and also look into how much pain it would be to preserve the Personally, I agree we shouldn't break anything without warning but I am also pretty convinced that there is no serious usage of I think this should not block review of this change here though. |
Thanks, everyone for the review and feedback so far! A few more updates:
EDIT: Here is another, slightly altered approach to the re-introduction of |
The legacy serialization was vulnerable to maleation and is fixed by adopting the same serialization procedure as was already in use for MuHash. This also includes necessary test fixes where the hash_serialized2 was hardcoded as well as correction of the regtest chainparams. Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
-BEGIN VERIFY SCRIPT- sed -i 's/hash_serialized_2/hash_serialized_3/g' $( git grep -l 'hash_serialized_2' ./src ./contrib ./test ) -END VERIFY SCRIPT-
Review hint: You can use devtools/utxo_snapshot.sh to validate this. ./contrib/devtools/utxo_snapshot.sh 2500000 testnet-utxo.dat ./src/bitcoin-cli
Review hint: You can use devtools/utxo_snapshot.sh to validate this. ./contrib/devtools/utxo_snapshot.sh 160000 signet-utxo.dat ./src/bitcoin-cli
942f058
to
4bfaad4
Compare
Rebased since #28669 was merged 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK f621392
I did not verify the new chainparams hashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK 4bfaad4
Verified the adapted testnet and signet txoutset hashes via
$ ./src/bitcoind -testnet &
$ ./contrib/devtools/utxo_snapshot.sh 2500000 testnet-utxo.dat ./src/bitcoin-cli -testnet
Rewinding chain back to height 2500000 (by invalidating 000000000000016ffe5c7617d0ac05a8bad83aed050241d91a1c2f67c3320bfd); this may take a while
Generating UTXO snapshot...
{
"coins_written": 29324631,
"base_hash": "0000000000000093bcb68c03a9a168ae252572d348a2eaeba2cdf9231d73206f",
"base_height": 2500000,
"path": "/home/thestack/.bitcoin/testnet3/testnet-utxo.dat",
"txoutset_hash": "f841584909f68e47897952345234e37fcd9128cd818f41ee6c3ca68db8071be7",
"nchaintx": 66484552
}
Restoring chain to original height; this may take a while
and
$ ./src/bitcoind -signet &
$ ./contrib/devtools/utxo_snapshot.sh 160000 signet-utxo.dat ./src/bitcoin-cli -signet
Rewinding chain back to height 160000 (by invalidating 000000a724dba71c14821ecf2670048cc99de44c206ea2c84411b97dd1ab14a4); this may take a while
Generating UTXO snapshot...
{
"coins_written": 1278002,
"base_hash": "0000003ca3c99aff040f2563c2ad8f8ec88bd0fd6b8f0895cfaf1ef90353a62c",
"base_height": 160000,
"path": "/home/thestack/.bitcoin/signet/signet-utxo.dat",
"txoutset_hash": fe0a44309b74d6b5883d246cb419c6221bcccf0b308c9b59b7d70783dbdf928a",
"nchaintx": 2289496
}
Restoring chain to original height; this may take a while
RPC | ||
--- | ||
|
||
The `hash_serialized_2` value has been removed from `gettxoutsetinfo` since the value it calculated contained a bug and did not take all data into account. It is superseded by `hash_serialized_3` which provides the same functionality but serves the correctly calculated hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc-nit: could add PR reference
The `hash_serialized_2` value has been removed from `gettxoutsetinfo` since the value it calculated contained a bug and did not take all data into account. It is superseded by `hash_serialized_3` which provides the same functionality but serves the correctly calculated hash. | |
The `hash_serialized_2` value has been removed from `gettxoutsetinfo` since the value it calculated contained a bug and did not take all data into account. It is superseded by `hash_serialized_3` which provides the same functionality but serves the correctly calculated hash. (#28685) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 4bfaad4
I only reviewed the code and did not confirm the hardcoded hash values, but code changes seem very good. At first I was confused why the first commit was making so many changes, instead of just doing a minimal bugfix:
-ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u);
+ss << VARINT(it->second.nHeight * 2 + (it->second.fCoinBase ? 1u : 0u));
and updating the hashes.
But I guess the idea is that as long as the hash values need to change anyway, might as well make other changes to the hash format and simplify the code around it. Making these bigger changes to the way this hash is computed instead of just fixing the bug and restoring the original hash_serialized_2
introduced in #10195 and broken in #12737 does seem reasonable.
@@ -48,15 +48,35 @@ uint64_t GetBogoSize(const CScript& script_pub_key) | |||
script_pub_key.size() /* scriptPubKey */; | |||
} | |||
|
|||
DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin) | |||
template <typename T> | |||
static void TxOutSer(T& ss, const COutPoint& outpoint, const Coin& coin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "coinstats: Fix hash_serialized2 calculation" (351370a)
Name of this function is a little confusing since its signature is changing and it is no longer returning a serialization. Would suggest calling it SerializeTxOut or similar.
ss << VARINT_MODE(it->second.out.nValue, VarIntMode::NONNEGATIVE_SIGNED); | ||
|
||
if (it == std::prev(outputs.end())) { | ||
ss << VARINT(0u); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "coinstats: Fix hash_serialized2 calculation" (351370a)
It seems reasonable to to get rid of ss << VARINT(0)
added to the hash between transactions. It seems to date back to the original hashing code added in #7756 and I guess along with the nonzero ss << VARINT(it->first + 1)
above was intended to delimit the boundary between transactions. None of this is necessary with the new hash format that repeats the transaction id before each coin.
ACK 4bfaad4 Also verified that the updated chainparams are correct. |
Thanks a lot to the reviewers! FYI, nobody has reached out concerning the usage of |
post-merge tACK da8e397 tested on signet output
tested on testnet output
@Sjors's commit for the |
I'll look into updating the hash in #28553 |
Closes #28675
The last commit demonstrates that theStack's analysis here seems to be correct. There will be more changes needed for the rest of the test suite but the
feature_assumeutxo.py
with my additional tests pass.