Fix assumeutxo crash due to invalid base_blockhash #21584
Conversation
|
Introduced in #19806 |
804156f to
06bd064
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
d370f20 to
fae0997
Compare
There was a problem hiding this comment.
Code review ACK fae0997. This all looks good, and I especially like getting rid of the magic 0 block hashes.
One thing I'm not clear on is the motivation for moving the assume utxo block hash check earlier in fa8676b. Is it just supposed to be an optimization to fail faster if an unrecognized snapshot is loaded? Or is it actually still a crash fix after #21582?
fae0997 to
fa19a40
Compare
|
Force pushed in an attempt to address feedback.
Yes. You can check by running the added test without the fix and observing the crash. |
fa19a40 to
3333a34
Compare
There was a problem hiding this comment.
Code review ACK 3333a34. Since last review main change was to restore some previous code and no longer change the MapAssumeutxo definition.
Now that I understand the crash better, I will say I broadly agree with comments from Marco like #19806 (review), and it doesn't seem great that PopulateAndValidateSnapshot feeds untrusted data to CCoins code that wasn't necessarily written to handle it. (I'm imagining an assumeutxo snapshot file that doesn't load correctly, but does launch calc.exe on windows.) Maybe the assumeutxo data should include a simpler hash along with the CCoinsStats hash, that could be checked before the data is fed to CCoins.
|
Code review ACK, haven't tested the fix but it is much better to use |
Can be reviewed with --color-moved=dimmed-zebra
This avoids accidentally mixing it up with other hashes (like block hashes).
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
3333a34 to
fa9b802
Compare
Just use std::optional
fa9b802 to
fa340b8
Compare
|
Rebased. Should be trivial to re-ACK |
There was a problem hiding this comment.
ACK fa340b8 (jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due)
Built, ran tests locally. Change looks good; not only fixes the crash
bug, but removes special-case interpretation of 0000... as null
blockhash value.
Show signature data
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK fa340b87944764ea4e8e04038fe7471fd452bc23 ([`jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due))
Built, ran tests locally. Change looks good; not only fixes the crash
bug, but removes special-case interpretation of `0000...` as null
blockhash value.
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmCcB70ACgkQepNdrbLE
TwXO6hAAlqvcS4gXI4Q702FfwOMO3Kmp2ZAbZy9yW6sIIJVkAcYxV8+CXf8UdWu5
dzQFKWWbGA9+Kbis1sva71OVWarBNQLVeUvZ8PHlfVLRLDXQ7YzOPUjidYKHNLtN
B3t4WhOLnUKVXtC1J98beaE4ZyaWtTvk7000xVEepiA73BSVCgAQA05pajBWGxdA
m9sgoaivixkNZbew+Twy6svmTBzUK6SQmODG6dHZhSKgc8DCzXSffgQFTPRoYfsL
e0YZJfrsnh/8UePIrA8FHDmEeupE9L36byg51W5E3ubHj1zMB4ZZTBhd7JX7+jPJ
gEjYU9JMkbSpZmOjmbnf/JMTpIrhjxzKGpcO+KEG4XmzYaaZ5S5VxYv/7Ym2PJKH
sDdhj9xaX79ATveyC+QN6fnk6/+/MedqQBRAFcnZubfe9+J4885lJj5AKFEZBZUu
V+hePUsSL22M/eMLVwJAzpXZ3TUU4ga3r1epdOtIpaLqg1YUH39pDV0YBZSdCIpR
27IfbnUsHGCfT8ddPIdjp/EViAAKl/9WS28GOT9GYF7ig39Voi/ropPiEbdv/IpQ
o2ilsAyS5jehtzZrLFw3/XcvM0/+KmXuE4hPreHEZoQbjpkmi+Kw4eMLtZaGEn6u
PWOUUghFNQJhR5qJqByWgkGPfCBYUB1xLsISiYJdOD8v11jvLS4=
=2I/H
-----END PGP SIGNATURE-----
Show platform data
Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28
Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_srcdir)=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2 i
Compiler version: Debian clang version 11.1.0-++20210405104510+1fdec59bffc1-1~exp1~20210405085125.161
|
Code review re-ACK fa340b8 |
Summary: Can be reviewed with --color-moved=dimmed-zebra This is a partial backport of [[bitcoin/bitcoin#21584 | core#21584]] bitcoin/bitcoin@faa921f Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11330
Summary: Values for mainnet and testnet will be specified in a follow-up PR that can be scrutinized accordingly. This structure is required for use in snapshot activation logic. This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [4/8] and [[bitcoin/bitcoin#21584 | core#21584]] [2 & 3/5] bitcoin/bitcoin@7a6c46b bitcoin/bitcoin@0000007 bitcoin/bitcoin@fa5668b (partial) Notes: - The new circular dependency was added by core in a later commit. We already need it here. - We also check the allowed hashes in a functional test reproducing the deterministic regtest chain and allowed hashes. Depends on D11328 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11238
Summary: bitcoin/bitcoin@769a1ef > test: Add tests with maleated snapshot data ---- https://github.com/bitcoin/bitcoin/pull/21681/files > validation: fix ActivateSnapshot to use hardcoded nChainTx > validation: remove nchaintx from assumeutxo metadata Only test related changes for these commits are in this revision ---- bitcoin/bitcoin@fa8fffe > refactor: Prefer clean assert over UB in coinstats ---- bitcoin/bitcoin@fa9b74f > Fix assumeutxo crash due to missing base_blockhash Only test related changes from this commit are in this revision. ---- bitcoin/bitcoin@fae33f9 > Fix assumeutxo crash due to invalid base_blockhash Only test related changes from this commit are in this revision. ---- This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [7/8] and test related changes from [[bitcoin/bitcoin#21681 | core#21681]], [[bitcoin/bitcoin#21582 | core#21582]] and [[bitcoin/bitcoin#21584 | core#21584]] Depends on D11240 Test Plan: `ninja check` With UBSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11241
Summary: Just use std::optional This concludes backport of [[bitcoin/bitcoin#21584 | core#21584]] bitcoin/bitcoin@fa340b8 Depends on D11334 and D11241 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11331
Starting with commit d6af06d, a block hash of all-zeros is invalid and will lead to a crash of the node. Can be tested by cherry-picking the test changes without the other changes.
Stack trace (copied from #21584 (comment)):