Fix assumeutxo crash due to invalid base

MarcoFalke commented at 2:49 pm on April 3, 2021: member

Starting with commit d6af06d68aa, 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 (review)):

 0[#0](/bitcoin-bitcoin/0/)  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
 1[#1](/bitcoin-bitcoin/1/)  0x00007ffff583c8b1 in __GI_abort () at abort.c:79
 2[#2](/bitcoin-bitcoin/2/)  0x00007ffff582c42a in __assert_fail_base (fmt=0x7ffff59b3a38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
 3    assertion=assertion@entry=0x555556c8b450 "!hashBlock.IsNull()", file=file@entry=0x555556c8b464 "txdb.cpp", line=line@entry=89, 
 4    function=function@entry=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:92
 5[#3](/bitcoin-bitcoin/3/)  0x00007ffff582c4a2 in __GI___assert_fail (assertion=0x555556c8b450 "!hashBlock.IsNull()", file=0x555556c8b464 "txdb.cpp", line=89, 
 6    function=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:101
 7[#4](/bitcoin-bitcoin/4/)  0x000055555636738b in CCoinsViewDB::BatchWrite (this=0x5555577975c0, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at txdb.cpp:89
 8[#5](/bitcoin-bitcoin/5/)  0x00005555564a2e80 in CCoinsViewBacked::BatchWrite (this=0x5555577975f8, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at coins.cpp:30
 9[#6](/bitcoin-bitcoin/6/)  0x00005555564a43de in CCoinsViewCache::Flush (this=0x55555778eaf0) at coins.cpp:223
10[#7](/bitcoin-bitcoin/7/)  0x00005555563fc11d in ChainstateManager::PopulateAndValidateSnapshot (this=0x55555740b038 <g_chainman>, snapshot_chainstate=..., coins_file=..., metadata=...)
11    at validation.cpp:5422
12[#8](/bitcoin-bitcoin/8/)  0x00005555563fab3d in ChainstateManager::ActivateSnapshot (this=0x55555740b038 <g_chainman>, coins_file=..., metadata=..., in_memory=true) at validation.cpp:5299
13[#9](/bitcoin-bitcoin/9/)  0x0000555555e8c893 in validation_chainstatemanager_tests::CreateAndActivateUTXOSnapshot<validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12>(NodeContext&, boost::filesystem::path, validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12) (node=..., 
14    root=..., malleation=...) at test/validation_chainstatemanager_tests.cpp:199
15[#10](/bitcoin-bitcoin/10/) 0x0000555555e8877a in validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method (this=0x7fffffffc8d0)
16    at test/validation_chainstatemanager_tests.cpp:262

MarcoFalke commented at 2:52 pm on April 3, 2021: member

Introduced in #19806

DrahtBot added the label Validation on Apr 3, 2021

MarcoFalke force-pushed on Apr 4, 2021

DrahtBot commented at 6:43 am on April 4, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21620 (ChainstateManager locking improvements by jamesob)

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.

MarcoFalke force-pushed on Apr 7, 2021

MarcoFalke force-pushed on Apr 9, 2021

MarcoFalke marked this as ready for review on Apr 9, 2021

in src/chainparams.cpp:443 in fa3b1cd38e outdated

439@@ -440,12 +440,12 @@ class CRegTestParams : public CChainParams {
440 
441         m_assumeutxo_data = MapAssumeutxo{
442             {
443-                110,
444-                {uint256S("0x1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618"), 110},
445+                BlockHash{uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c")}, // height 110

ryanofsky commented at 10:13 pm on April 12, 2021:

In commit “refactor: Assumeutxo lookup by blockhash” (fa3b1cd38eb98dd90be097bd8f0734a802de38f0)

Seems like you could drop // height = 110 to not repeat 110 next line. Could maybe write do /* height = */ there to be clearer the value is a height.

MarcoFalke commented at 6:25 am on April 13, 2021:

;)

The value is not height but nChainTx

in src/chainparams.h:55 in fa3b1cd38e outdated

51@@ -43,7 +52,7 @@ struct AssumeutxoData {
52 
53 std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud);
54 
55-using MapAssumeutxo = std::map<int, const AssumeutxoData>;
56+using MapAssumeutxo = std::map<BlockHash, const AssumeutxoData>;

ryanofsky commented at 10:18 pm on April 12, 2021:

In commit “refactor: Assumeutxo lookup by blockhash” (fa3b1cd38eb98dd90be097bd8f0734a802de38f0)

Interesting this adds block hash information to the MapAssumeutxo map. Previously there was discussion about doing the opposite and removing block height information #19806 (review).

It seems to me like maybe it could be better not to add extra hardcoded information that needs more runtime code and reviewer effort to check. An alternative to hardcoding block hashes here could just be to look up the block hashes as they are needed using the heights (i.e. when loading a new snapshot, iterate over m_assumeutxo_data entries and check if snapshot base hash matches block hash of any entry’s height).

This is just a thought. No strong opinion about what’s hardcoded or not hardcoded.

MarcoFalke commented at 8:53 am on April 13, 2021:

There should be no difference in using block height or block hash, but I reverted to height, since that is used before.

in src/validation.cpp:5255 in fa8676b668 outdated

5251@@ -5252,6 +5252,14 @@ bool ChainstateManager::ActivateSnapshot(
5252         return false;
5253     }
5254 
5255+    auto maybe_au_data = ExpectedAssumeutxo(BlockHash{base_blockhash}, ::Params());

ryanofsky commented at 10:42 pm on April 12, 2021:

In commit “Fix assumeutxo crash due to invalid base_blockhash” (fa8676b668cfd71eb299bd7e59cdec374c5d5407)

After #21582, should this commit description be updated? This commit seems not to avoid a crash, but just be an optimization that makes the check for a known assumeutxo hash happen a little earlier than it used to.

MarcoFalke commented at 8:51 am on April 13, 2021:

huh? This absolutely avoids a crash. Do you have steps to not-reproduce the crash I added in that commit?

ryanofsky commented at 6:12 pm on April 13, 2021:

re: #21584 (review)

huh? This absolutely avoids a crash. Do you have steps to not-reproduce the crash I added in that commit?

Marco. You’re going to make me actually test a PR? What about my tried and true method of just asking stupid questions about PRs and hoping that it sometimes makes things better?

But just in case anyone else is curious where the crash happens and doesn’t want to endure the indignities Marco is trying to subject me to, the crash doesn’t happen where I was looking for it in ActivateSnapshot, but actually in an assert inside the coins_cache.Flush() call in PopulateAndValidateSnapshot:

https://github.com/bitcoin/bitcoin/blob/a1f0b8b62eb851c837a3618583b7c2fd4d12006c/src/validation.cpp#L5275

The stack trace is:

 0[#0](/bitcoin-bitcoin/0/)  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
 1[#1](/bitcoin-bitcoin/1/)  0x00007ffff583c8b1 in __GI_abort () at abort.c:79
 2[#2](/bitcoin-bitcoin/2/)  0x00007ffff582c42a in __assert_fail_base (fmt=0x7ffff59b3a38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
 3    assertion=assertion@entry=0x555556c8b450 "!hashBlock.IsNull()", file=file@entry=0x555556c8b464 "txdb.cpp", line=line@entry=89, 
 4    function=function@entry=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:92
 5[#3](/bitcoin-bitcoin/3/)  0x00007ffff582c4a2 in __GI___assert_fail (assertion=0x555556c8b450 "!hashBlock.IsNull()", file=0x555556c8b464 "txdb.cpp", line=89, 
 6    function=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:101
 7[#4](/bitcoin-bitcoin/4/)  0x000055555636738b in CCoinsViewDB::BatchWrite (this=0x5555577975c0, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at txdb.cpp:89
 8[#5](/bitcoin-bitcoin/5/)  0x00005555564a2e80 in CCoinsViewBacked::BatchWrite (this=0x5555577975f8, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at coins.cpp:30
 9[#6](/bitcoin-bitcoin/6/)  0x00005555564a43de in CCoinsViewCache::Flush (this=0x55555778eaf0) at coins.cpp:223
10[#7](/bitcoin-bitcoin/7/)  0x00005555563fc11d in ChainstateManager::PopulateAndValidateSnapshot (this=0x55555740b038 <g_chainman>, snapshot_chainstate=..., coins_file=..., metadata=...)
11    at validation.cpp:5422
12[#8](/bitcoin-bitcoin/8/)  0x00005555563fab3d in ChainstateManager::ActivateSnapshot (this=0x55555740b038 <g_chainman>, coins_file=..., metadata=..., in_memory=true) at validation.cpp:5299
13[#9](/bitcoin-bitcoin/9/)  0x0000555555e8c893 in validation_chainstatemanager_tests::CreateAndActivateUTXOSnapshot<validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12>(NodeContext&, boost::filesystem::path, validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12) (node=..., 
14    root=..., malleation=...) at test/validation_chainstatemanager_tests.cpp:199
15[#10](/bitcoin-bitcoin/10/) 0x0000555555e8877a in validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method (this=0x7fffffffc8d0)
16    at test/validation_chainstatemanager_tests.cpp:262
17```

MarcoFalke commented at 6:21 pm on April 13, 2021:

Thanks Ryan, added your stack trace to #21584#issue-608396741

MarcoFalke commented at 6:22 pm on April 13, 2021:

You’re going to make me actually test a PR

The commit that added the assert is mentioned in OP, so in theory it would be possible to review without testing ;)

ryanofsky commented at 6:31 pm on April 13, 2021:

The commit that added the assert is mentioned in OP, so in theory it would be possible to review without testing ;)

Now I get it. Thanks! Another connection I didn’t make the first time around.

in src/test/validation_chainstatemanager_tests.cpp:272 in fae0997688 outdated

270@@ -273,9 +271,8 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
271     BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root));
272 
273     // Ensure our active chain is the snapshot chainstate.
274-    BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash.IsNull());

ryanofsky commented at 10:44 pm on April 12, 2021:

In commit “refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash” (fae09976880051232a4082248e30cc7a3b801644)

Seems fine to drop this line, but strictly speaking this check could still work and could be kept? (Just want to make sure I’m not missing something and it had to be removed for another reason)

MarcoFalke commented at 8:50 am on April 13, 2021:

Restored check

ryanofsky approved

ryanofsky commented at 11:13 pm on April 12, 2021: member

Code review ACK fae09976880051232a4082248e30cc7a3b801644. 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 fa8676b668cfd71eb299bd7e59cdec374c5d5407. 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?

MarcoFalke force-pushed on Apr 13, 2021

MarcoFalke commented at 8:54 am on April 13, 2021: member

Force pushed in an attempt to address feedback.

is it actually still a crash fix

Yes. You can check by running the added test without the fix and observing the crash.

MarcoFalke force-pushed on Apr 13, 2021

ryanofsky approved

ryanofsky commented at 6:23 pm on April 13, 2021: member

Code review ACK 3333a34d440e55908eaa411c1b62e50aa52e58ad. 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#pullrequestreview-634216112, 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.

jamesob approved

jamesob commented at 4:40 pm on April 14, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/21584/commits/3333a34d440e55908eaa411c1b62e50aa52e58ad

Reviewed code on Github; haven’t fetched/built locally.

Changes all look good, thanks for these. Using optional vs. IsNull() for the hashes is a lot better.

DrahtBot added the label Needs rebase on Apr 30, 2021

DrahtBot commented at 9:32 am on May 3, 2021: member

🕵️ @jamesob @sipa have been requested to review this pull request as specified in the REVIEWERS file.

laanwj commented at 1:08 pm on May 5, 2021: member

Code review ACK, haven’t tested the fix but it is much better to use std::optional instead of special-casing a hash value. Needs rebase, though.

move-only: Add util/hash_type

Can be reviewed with --color-moved=dimmed-zebra

faa921f787

refactor: Remove unused code 0000007709

refactor: Use type-safe assumeutxo hash

This avoids accidentally mixing it up with other hashes (like block
hashes).

fa5668bfb3

Fix assumeutxo crash due to invalid base_blockhash

Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

fae33f98e6

MarcoFalke force-pushed on May 11, 2021

DrahtBot removed the label Needs rebase on May 11, 2021

refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash

Just use std::optional

fa340b8794

MarcoFalke force-pushed on May 11, 2021

MarcoFalke commented at 9:54 am on May 11, 2021: member

Rebased. Should be trivial to re-ACK

jamesob approved

jamesob commented at 4:52 pm on May 12, 2021: member

ACK fa340b87944764ea4e8e04038fe7471fd452bc23 (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.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK fa340b87944764ea4e8e04038fe7471fd452bc23 ([`jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due))
 4
 5Built, ran tests locally. Change looks good; not only fixes the crash
 6bug, but removes special-case interpretation of `0000...` as null
 7blockhash value.
 8-----BEGIN PGP SIGNATURE-----
 9
10iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmCcB70ACgkQepNdrbLE
11TwXO6hAAlqvcS4gXI4Q702FfwOMO3Kmp2ZAbZy9yW6sIIJVkAcYxV8+CXf8UdWu5
12dzQFKWWbGA9+Kbis1sva71OVWarBNQLVeUvZ8PHlfVLRLDXQ7YzOPUjidYKHNLtN
13B3t4WhOLnUKVXtC1J98beaE4ZyaWtTvk7000xVEepiA73BSVCgAQA05pajBWGxdA
14m9sgoaivixkNZbew+Twy6svmTBzUK6SQmODG6dHZhSKgc8DCzXSffgQFTPRoYfsL
15e0YZJfrsnh/8UePIrA8FHDmEeupE9L36byg51W5E3ubHj1zMB4ZZTBhd7JX7+jPJ
16gEjYU9JMkbSpZmOjmbnf/JMTpIrhjxzKGpcO+KEG4XmzYaaZ5S5VxYv/7Ym2PJKH
17sDdhj9xaX79ATveyC+QN6fnk6/+/MedqQBRAFcnZubfe9+J4885lJj5AKFEZBZUu
18V+hePUsSL22M/eMLVwJAzpXZ3TUU4ga3r1epdOtIpaLqg1YUH39pDV0YBZSdCIpR
1927IfbnUsHGCfT8ddPIdjp/EViAAKl/9WS28GOT9GYF7ig39Voi/ropPiEbdv/IpQ
20o2ilsAyS5jehtzZrLFw3/XcvM0/+KmXuE4hPreHEZoQbjpkmi+Kw4eMLtZaGEn6u
21PWOUUghFNQJhR5qJqByWgkGPfCBYUB1xLsISiYJdOD8v11jvLS4=
22=2I/H
23-----END PGP SIGNATURE-----

0Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28
1
2Configured 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
3
4Compiled 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
5
6Compiler version: Debian clang version 11.1.0-++20210405104510+1fdec59bffc1-1~exp1~20210405085125.161

laanwj commented at 6:59 pm on May 12, 2021: member

Code review re-ACK fa340b87944764ea4e8e04038fe7471fd452bc23

laanwj merged this on May 12, 2021

laanwj closed this on May 12, 2021

MarcoFalke deleted the branch on May 12, 2021

sidhujag referenced this in commit 86b8f4d7b0 on May 13, 2021

Fabcien referenced this in commit af6a2dfd8d on Apr 12, 2022

Fabcien referenced this in commit a5dc7a8acf on Apr 12, 2022

Fabcien referenced this in commit 83ee112814 on Apr 13, 2022

Fabcien referenced this in commit 1501283bac on Apr 13, 2022

gwillen referenced this in commit 1fd44e5dfc on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

Fix assumeutxo crash due to invalid base_blockhash #21584

Conflicts