This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes. The fix:
Assert to transform the UB into a clean crash, even when sanitizers are disabled
I couldn’t find a code path that needs this behavior, so it seemed odd to keep the dead code. Note that in your parent PR the RPC does the waiting, so it doesn’t have to be done again in validation.
Edit: See max_secs_to_wait_for_headers in commit 1cf452104cd1c06ddba4001714f01d0516b864e9
Note that in your parent PR the RPC does the waiting, so it doesn’t have to be done again in validation.
Oh good point. I guess if we add some flag that allows specifying a snapshot to load (absent RPC) we’ll have to add some similar code to init, but we can cross that bridge when we come to it.
In any case, concept ACK. Will test later today.
we can cross that bridge when we come to it
Indeed, I was never a fan of the “idle-loop” inside validation (See #19806 (review)), so moving it to the caller (if needed at all) seems the way to go.
Code review ACK fafcddcd5311064c2e98679dd08be059ef25b318. All changes look good, but I don’t know when the bug happens in practice or how it was discovered.
Also it would be nice to have just the minimal fix and test together in same commit so it is more clear how the bug can be triggered and how it is fixed. Then it would be good to have the other cleanups in separate commits. IIUC:
if (!snapshot_start_block) check in PopulateAndValidateSnapshot, triggered by the new test passing base hash uint256::ONE.
separate commits
Thanks, force pushed so that the Assert is a separate commit, no code changes.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
ACK fa9b74f5ea89624e052934c48391b5076a87ffef (jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due)
Looks good, thanks.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK fa9b74f5ea89624e052934c48391b5076a87ffef ([`jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due))
4
5Looks good, thanks.
6-----BEGIN PGP SIGNATURE-----
7
8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBrT/QACgkQepNdrbLE
9TwULqw//SIwBkIK7OsgTB1ycHuwVJnJqnbO67v0oG/KI/2483t7jBJvMCznX2ecT
10at4FcfJ7HmyrWve4EKz5mpa7851s565ftUNMy+K6nGj53tr+ejXyioDmEamol/AT
11KzYah/LQjtwdS/WbNs1JyXE3NQ0briscUhi6053tdYGDwmtc+bdh4wdkcySRWHFR
12a7SLoASpQ4rj3c2VlSyDzXoUCLNtpvylO9aRxgINC/IOU8UKS/F51LjMIGmeNHHP
13oDDJ8cZwQURfSqg8bIQWpVDFwGBhPdXAFrQdl2kBdqJa48yp4PEg7KIHkh+H3xKV
14W4sn2xMghzwB+MtFOCUDTY6ggy7yuh/6w/ZLqBuwrQaZyepMu3OUJ8PnEH8mk4OV
15ddVL43/iGoEf9lH+X3f6WykZFIDTeMy1Np0YGjrDcFzc3qxDpQip++dvcm3rblDP
16BlaUUm1UucSU8ZafZZJpv5Jg0EpbPaNlAMb8qp1N73AD94ngYq/LIlgahu280obM
17v7lAxApGzUpufGl0Fg5ARqvKPWpwEuS8FoM0EUkv8wNaJeNxZAuUgIJ3R8PmoffH
18ffAgxlNS5Q6o3aAPsTin9hSR7Bfkzit4/Tm9xMHiH+WCHJPiML4mlO4h5DbSUjYn
19dyxgaI1e32gJBrY/xroaiu8v+EmyJeHeuDlcbN+lP7AzhDN4XfU=
20=Pv7n
21-----END PGP SIGNATURE-----
0Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ -L/home/james/.local/lib CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ -I/home/james/.local/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 --enable-multiprocess PKG_CONFIG_PATH=/home/james/.local/lib/pkgconfig CXX=/usr/bin/clang++ CC=/usr/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
3
4Compiled with /usr/bin/ccache /usr/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 -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2 i
5
6Compiler version: clang version 7.0.1-8 (tags/RELEASE_701/final)
Code review ACK fa9b74f5ea89624e052934c48391b5076a87ffef with no code changes since last review, just splitting up combocommit a little.
This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes.
Look good, though I’m curious if the crash was happening before the new test. Was the bug causing a different crash in another test? Or was it just discovered reading code?
was it just discovered reading code?
This particular one was discovered by a fuzz target, which I wrote after I discovered a crash with code review: #21585
Post merge cr ACK fa9b74f5ea89624e052934c48391b5076a87ffef
was it just discovered reading code?
This particular one was discovered by a fuzz target, which I wrote after I discovered a crash with code review: #21585
Nice fuzzing find! Fuzzing makes us: harder better faster^Wresilient stronger! 🎶
Don’t forget to add your find to the growing list of trophies over at https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Fuzz-Trophies :)