[Bundle 7/7] validation: Farewell, global Chainstate! #21866

dongcarl commented at 8:51 PM on May 5, 2021: member

Based on: #21767

à la Mr. Sandman

Mr. Chainman, bring me a tip (bung, bung, bung, bung)
Make it the most work that I've ever seen (bung, bung, bung, bung)
Rewind old tip till we're at the fork point (bung, bung, bung, bung)
Then tell it that it's time to call Con-nectTip

Chainman, I'm so alone (bung, bung, bung, bung)
No local objects to call my own (bung, bung, bung, bung)
Please make sure I have a ref
Mr. Chainman, bring me a tip!

This is the last bundle in the #20158 series. Thanks everyone for their diligent review. I would like to call attention to #21766, where a few leftover improvements were collated.

Remove globals:
- ChainstateManager g_chainman
- CChainState& ChainstateActive()
- CChain& ChainActive()
Remove all review-only assertions.

DrahtBot added the label GUI on May 5, 2021

DrahtBot added the label Mempool on May 5, 2021

DrahtBot added the label Mining on May 5, 2021

DrahtBot added the label P2P on May 5, 2021

DrahtBot added the label RPC/REST/ZMQ on May 5, 2021

DrahtBot added the label UTXO Db and Indexes on May 5, 2021

DrahtBot added the label Validation on May 5, 2021

DrahtBot added the label Wallet on May 5, 2021

DrahtBot commented at 6:53 AM on May 6, 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:

#22155 (wallet test: Add test for subtract fee from recipient behavior by ryanofsky)
#21789 (refactor: Remove ::Params() global from CChainState by MarcoFalke)
#21706 (log: Mitigate disk filling attacks by globally rate limiting LogPrintf(…) by dergoegge)
#21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)
#21562 ([net processing] Various tidying up of PeerManagerImpl ctor by jnewbery)
#21526 (validation: UpdateTip/CheckBlockIndex assumeutxo support by jamesob)
#21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
#20331 (allow -loadblock blocks to be unsorted by LarryRuane)
#20295 (rpc: getblockfrompeer by Sjors)
#19438 (Introduce deploymentstatus by ajtowns)
#19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
#16981 (Improve runtime performance of --reindex by LarryRuane)
#15719 (Wallet passive startup by ryanofsky)

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.

Sjors commented at 1:40 PM on May 6, 2021: member

In the description of a839078c20f4ac3d92e14af7b287f2f0fddbc61e:

It fixes the erroneous use of Ensure*() in rest.cpp, which caused crashes in REST contexts.

Can you elaborate on this?

dongcarl commented at 3:59 PM on May 6, 2021: member

@Sjors EnsureAnyChainman uses the src/rpc way of returning errors, by doing:

https://github.com/bitcoin/bitcoin/blob/59869704c0fe0f105d9cbc193e3670917c618b83/src/rpc/blockchain.cpp#L85

However from my understanding, in src/rest, the way to return errors is to call RESTERR, which will fill the req in-out param with the right HTTP error.

In other words, throwing a JSONRPCError in the context of src/rest is meaningless, and therefore might crash the node much like an assert would.

(btw, that commit is part of Bundle 6)

Sjors commented at 6:11 PM on May 6, 2021: member

That makes sense, alright, I'll check out episode 6 first.

DrahtBot added the label Needs rebase on May 12, 2021

MarcoFalke removed the label GUI on Jun 1, 2021

MarcoFalke removed the label Mempool on Jun 1, 2021

MarcoFalke removed the label Mining on Jun 1, 2021

MarcoFalke removed the label P2P on Jun 1, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Jun 1, 2021

MarcoFalke removed the label UTXO Db and Indexes on Jun 1, 2021

MarcoFalke removed the label Wallet on Jun 1, 2021

MarcoFalke added the label Refactoring on Jun 1, 2021

dongcarl force-pushed on Jun 2, 2021

dongcarl commented at 10:31 PM on June 2, 2021: member

Pushed 8135fd659bd1f068aecf4d0ad3f3862127bb0886 -> 509264117c30704c2f048c702aa0ee9ac9657e7a

Rebased over merge of #21767

DrahtBot removed the label Needs rebase on Jun 2, 2021

laanwj added this to the "Blockers" column in a project

in src/node/interfaces.cpp:215 in 509264117c outdated

 221 | -            LOCK(::cs_main);
 222 | -            active_chainstate = &m_context->chainman->ActiveChainstate();
 223 | -            assert(std::addressof(::ChainstateActive()) == std::addressof(*active_chainstate));
 224 | -        }
 225 | -        return active_chainstate->IsInitialBlockDownload();
 226 | +        return m_context->chainman->ActiveChainstate().IsInitialBlockDownload();

ariard commented at 10:43 PM on June 8, 2021:

I think you can directly rely on NodeImpl::chainman() method here.

dongcarl commented at 3:03 PM on June 9, 2021:

Nice catch!

ariard commented at 10:44 PM on June 8, 2021: member

Approach ACK 5092641

dongcarl force-pushed on Jun 9, 2021

dongcarl commented at 3:03 PM on June 9, 2021: member

Pushed 509264117c...cc8cc9c4a2

Addressed #21866 (review)

jnewbery commented at 11:15 AM on June 10, 2021: member

utACK cc8cc9c4a24bb638baa733c129a87346c66821ba

jnewbery commented at 11:18 AM on June 10, 2021: member

This is a trivial rebase past #22084 (consecutive lines touched)

jnewbery commented at 11:19 AM on June 10, 2021: member

Strong concept ACK for:

Over time, we should probably move these mutable global state variables into ChainstateManager or CChainState so it's easier to reason about their lifecycles.

I'd be happy to review any of those changes. Please tag me in PRs that implement those.

DrahtBot added the label Needs rebase on Jun 10, 2021

in src/test/util/mining.cpp:82 in 1b21b6095a outdated

  79 |              .CreateNewBlock(coinbase_scriptPubKey)
  80 |              ->block);
  81 |  
  82 |      LOCK(cs_main);
  83 | -    block->nTime = ::ChainActive().Tip()->GetMedianTimePast() + 1;
  84 | +    block->nTime = Assert(node.chainman)->ActiveChain().Tip()->GetMedianTimePast() + 1;

jamesob commented at 4:34 PM on June 10, 2021:

https://github.com/bitcoin/bitcoin/pull/21866/commits/1b21b6095a4deb9b88cbee27bb9f885565793741

Obligatory nit about using ActiveTip()

in src/test/miner_tests.cpp:447 in f172c8a220 outdated

 439 | @@ -440,7 +440,11 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
 440 |      m_node.mempool->addUnchecked(entry.Fee(HIGHFEE).Time(GetTime()).SpendsCoinbase(true).FromTx(tx));
 441 |      BOOST_CHECK(CheckFinalTx(::ChainActive().Tip(), CTransaction(tx), flags)); // Locktime passes
 442 |      BOOST_CHECK(!TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks fail
 443 | -    BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(::ChainActive().Tip()->nHeight + 2))); // Sequence locks pass on 2nd block
 444 | +
 445 | +    {
 446 | +        CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
 447 | +        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 2, active_chain_tip))); // Sequence locks pass on 2nd block
 448 | +    }

jamesob commented at 4:47 PM on June 10, 2021:

https://github.com/bitcoin/bitcoin/pull/21866/commits/f172c8a220c29d1724c8c1a62cacc1ac8cbc2f17

I don't mind it but is there the temporary scope necessary? (Also ActiveTip() nits)

in src/test/miner_tests.cpp:464 in f172c8a220 outdated

 461 | +        m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i)->nTime += 512; //Trick the MedianTimePast
 462 | +
 463 | +    {
 464 | +        CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip();
 465 | +        BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip))); // Sequence locks pass 512 seconds later
 466 | +    }

jamesob commented at 4:48 PM on June 10, 2021:

https://github.com/bitcoin/bitcoin/pull/21866/commits/f172c8a220c29d1724c8c1a62cacc1ac8cbc2f17

ActiveTip() nits here if you end up rebasing.

laanwj added this to the milestone 22.0 on Jun 10, 2021

init: Use existing chainman 464c313e30

test/util: Use existing chainman in ::PrepareBlock f0dd5e6bb4

test/miner_tests: Pass in chain tip to CreateBlockIndex 4d99b61014

test: Pass in CoinsTip to ValidateCheckInputsForAllFlags e197076219

scripted-diff: test: Use existing chainman in unit tests

-BEGIN VERIFY SCRIPT-
git ls-files -- src/test \
    | grep -v '^src/test/fuzz' \
    | xargs sed -i -E \
            -e 's@g_chainman\.m_blockman@m_node.chainman->m_blockman@g' \
            -e 's@([^:])(Chain(state|)Active)@\1::\2@g' \
            -e 's@::Chain(state|)Active\(\)@m_node.chainman->ActiveChain\1()@g'
-END VERIFY SCRIPT-

0d61634c06

fuzz: Initialize a TestingSetup for test_one_input

For fuzz tests that need it.

ee0ab1e959

scripted-diff: wallet/test: Use existing chainman

-BEGIN VERIFY SCRIPT-
git ls-files -- src/wallet/test \
    | xargs sed -i -E \
            -e 's@g_chainman\.m_blockman@m_node.chainman->m_blockman@g' \
            -e 's@([^:])(Chain(state|)Active)@\1::\2@g' \
            -e 's@::Chain(state|)Active\(\)@m_node.chainman->ActiveChain\1()@g'
-END VERIFY SCRIPT-

6c15de129c

qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) f323248aba

tree-wide: Remove stray review-only assertion

Unfortunately, these assertion don't fit the regex in the scripted-diff.
Therefore, we remove it manually.

3e82abb8dd

scripted-diff: tree-wide: Remove all review-only assertions

-BEGIN VERIFY SCRIPT-
find_regex='((assert|CHECK_NONFATAL)\(std::addressof|TODO: REVIEW-ONLY)' \
    && git grep -l -E "$find_regex" -- . \
        | xargs sed -i -E "/${find_regex}/d"
-END VERIFY SCRIPT-

6c3b5dc0c1

qt/test: Reset chainman in ~ChainstateManager instead

There are some mutable, global state variables that are currently reset
by UnloadBlockIndex such as pindexBestHeader which should be cleaned up
whenever the ChainstateManager is unloaded/reset/destructed/etc.

Not cleaning them up leads to bugs like a use-after-free that happens
like so:

1. At the end of a test, ChainstateManager is destructed, which also
   destructs BlockManager, which calls BlockManager::Unload to free all
   CBlockIndexes in its BlockMap
2. Since pindexBestHeader is not cleaned up, it now points to an invalid
   location
3. Another test starts to init, and calls LoadGenesisBlock, which calls
   AddToBlockIndex, which compares the genesis block with an invalid
   location
4. Cute puppies perish by the hundreds

Previously, for normal codepaths (e.g. bitcoind), we relied on the fact
that our program will be unloaded by the operating system which
effectively resets these variables. The one exception is in QT tests,
where these variables had to be manually reset.

Since now ChainstateManager is no longer a global, we can just put this
logic in its destructor to make sure that callers are always correct.

Over time, we should probably move these mutable global state variables
into ChainstateManager or CChainState so it's easier to reason about
their lifecycles.

972c5166ee

validation: Farewell, global Chainstate! 6f994882de

dongcarl force-pushed on Jun 10, 2021

dongcarl commented at 7:12 PM on June 10, 2021: member

Pushed cc8cc9c4a24bb638baa733c129a87346c66821ba...6f994882deafe62e97f0a889d8bdb8c96dcf913d

Rebased on top of master

I'd be happy to review any of those changes. Please tag me in PRs that implement those. @jnewbery will do! 😁 I'm cleaning up my old branch right now which conflicts with Marco's node/blockstorage changes...

DrahtBot removed the label Needs rebase on Jun 10, 2021

jamesob approved

jamesob commented at 11:39 PM on June 10, 2021: member

ACK cc8cc9c4a24bb638baa733c129a87346c66821ba (jamesob/ackr/21866.1.dongcarl.bundle_7_7_validation_f)

Many scripted-diffs make this an easy, mechanical change. All the ActiveTip() nits can be probably be addressed by doing a single scripted-diff cleanup that seds 'ActiveChain\(\).Tip\(\)'.

Nice job with all the scripted-diffs; they make review much easier. Man oh man that's a lot of asserts! I like your move of UnloadBlockIndex() into the ChainstateManager destructor.

Nice work on this! Who ever thought we'd have chainstate deglobalized?? What a world.

<details><summary>Show signature data</summary> <p>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK cc8cc9c4a24bb638baa733c129a87346c66821ba ([`jamesob/ackr/21866.1.dongcarl.bundle_7_7_validation_f`](https://github.com/jamesob/bitcoin/tree/ackr/21866.1.dongcarl.bundle_7_7_validation_f))

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmDCobMACgkQepNdrbLE
TwX7NhAAgxLYZY+ecZjwI2y5OmqMrOjtmX+SplLm8B+iPpJCfYRkMfqZPG5qaVJQ
7eHCPBP6/V2Rx0CYgMFqX6G+3tJsW6k/ErahtuZXcp8nUEtyaOnt9jbDhdWRqAT9
MS0sEmvw/HNqz3qBpDW0aZs2OO6D01HUi2SkuCbze7zWiZYiAeeInUu1Du7tO0IQ
c3VqzJqz5XRVi64VF0fR5NeDgzgNYMTDWXEnwDK40JWB1gpM9uht5KuRGPLLfPl+
p7gDafWxoAbu9csPmKY9YIgPcQK2yKvQ5DSyWJVm57a0wHxlLFjGMCoprg7kbVn1
jaVyZ4EgghKT/NME6yhAeLBg8HAyaXNYaXISAlVyUM6FbvJ3p3sNpkgCbldz2xTH
nhiibFpwJbQVO/EoWw7Oy8TTe9FJGqlrhpXTR3vdsMy7g6gLwIzRQ71h43UVENs4
x4HnXfMEFPnIvt0W5xsSv6Ti+gWIB1e/nF/EdEwkmXD6s/6llqE7Bs1dGfDqb3jG
S9+2XsPy8KLgn+r3zSH66xKII70NdpUyjvQ9uNki8tVcTa5Jy905RgLxuldT2wrv
CGoazisooMRUfC44+a4Zn/w9IsxPzkUXP6RjmI9HfxF2f2vHWmv1IePwYJs0lD8U
M8x2upnRDRQe5QQn0+geIjsar4uRiEBphx6xBpSkFsWoDMmcP5Q=
=XPto
-----END PGP SIGNATURE-----

</p></details>

<details><summary>Show platform data</summary> <p>

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

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

</p></details>

jamesob commented at 12:02 AM on June 11, 2021: member

reACK https://github.com/bitcoin/bitcoin/pull/21866/commits/6f994882deafe62e97f0a889d8bdb8c96dcf913d based on the contents of git range-diff master ackr/21866.1.dongcarl.bundle_7_7_validation_f ackr/21866.2.dongcarl.bundle_7_7_validation_f and local build/test run.

ariard commented at 4:01 PM on June 11, 2021: member

Code Review ACK 6f99488.

Diff since last review is using NodeImpl::chainman method in isInitialBlockDownload() and rebase changes from #22084.

jnewbery commented at 4:07 PM on June 11, 2021: member

utACK 6f994882deafe62e97f0a889d8bdb8c96dcf913d

achow101 commented at 6:52 PM on June 11, 2021: member

Code Review ACK 6f994882deafe62e97f0a889d8bdb8c96dcf913d

ryanofsky approved

ryanofsky commented at 8:44 PM on June 11, 2021: member

Code review ACK 6f994882deafe62e97f0a889d8bdb8c96dcf913d.

Just want to note so it isn't forgotten that 972c5166ee685447a6d4bf5e501b07a0871fba85 commit message suggests some good future followup "Since now ChainstateManager is no longer a global, we can just put this [other variable cleanup] logic in its destructor to make sure that callers are always correct" which would make shutdown more comprehensible, and clean up the Qt tests

fanquake merged this on Jun 12, 2021

fanquake closed this on Jun 12, 2021

fanquake removed this from the "Blockers" column in a project

sidhujag referenced this in commit 9ac96c8db0 on Jun 14, 2021

MarcoFalke commented at 8:19 AM on June 14, 2021: member

Just noting that commit 972c5166ee685447a6d4bf5e501b07a0871fba85 causes a segmentation fault because the commits are in the wrong order:

$ valgrind ./src/qt/test/test_bitcoin-qt 
==1844015== Memcheck, a memory error detector
==1844015== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1844015== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==1844015== Command: ./src/qt/test/test_bitcoin-qt
...
********* Finished testing of RPCNestedTests *********
terminate called after throwing an instance of 'std::logic_error'
  what():  should not be overwriting a chainstate
==1844015== 
==1844015== Process terminating with default action of signal 6 (SIGABRT): dumping core
==1844015==    at 0x651A2A2: raise (in /usr/lib64/libc-2.33.so)
==1844015==    by 0x65038A3: abort (in /usr/lib64/libc-2.33.so)
==1844015==    by 0x61FEA45: ??? (in /usr/lib64/libstdc++.so.6.0.29)
==1844015==    by 0x620A29B: ??? (in /usr/lib64/libstdc++.so.6.0.29)
==1844015==    by 0x620A306: std::terminate() (in /usr/lib64/libstdc++.so.6.0.29)
==1844015==    by 0x620A5FC: __cxa_rethrow (in /usr/lib64/libstdc++.so.6.0.29)
==1844015==    by 0x4F392CD: ??? (in /usr/lib64/libQt5Test.so.5.15.2)
==1844015==    by 0x4F43B3F: QTest::qExec(QObject*, int, char**) (in /usr/lib64/libQt5Test.so.5.15.2)
==1844015==    by 0x196413: main (test_main.cpp:91)
==1844015== 
==1844015== Process terminating with default action of signal 11 (SIGSEGV)
==1844015==  General Protection Fault
==1844015==    at 0x4877C82: ??? (in /usr/lib64/libpthread-2.33.so)
==1844015==    by 0x660D03E: ??? (in /usr/lib64/libc-2.33.so)
==1844015==    by 0x664F60C: ??? (in /usr/lib64/libc-2.33.so)
==1844015==    by 0x664F261: __libc_freeres (in /usr/lib64/libc-2.33.so)
==1844015==    by 0x48351E7: _vgnU_freeres (vg_preloaded.c:74)
==1844015== 
==1844015== HEAP SUMMARY:
==1844015==     in use at exit: 45,331,308 bytes in 21,971 blocks
==1844015==   total heap usage: 128,095 allocs, 106,124 frees, 87,543,385 bytes allocated
==1844015== 
==1844015== LEAK SUMMARY:
==1844015==    definitely lost: 0 bytes in 0 blocks
==1844015==    indirectly lost: 0 bytes in 0 blocks
==1844015==      possibly lost: 22,704 bytes in 95 blocks
==1844015==    still reachable: 45,308,604 bytes in 21,876 blocks
==1844015==                       of which reachable via heuristic:
==1844015==                         stdstring          : 63 bytes in 1 blocks
==1844015==                         newarray           : 424 bytes in 11 blocks
==1844015==                         multipleinheritance: 71,160 bytes in 87 blocks
==1844015==         suppressed: 0 bytes in 0 blocks
==1844015== Rerun with --leak-check=full to see details of leaked memory
==1844015== 
==1844015== Use --track-origins=yes to see where uninitialised values come from
==1844015== For lists of detected and suppressed errors, rerun with: -s
==1844015== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)

MarcoFalke commented at 8:19 AM on June 14, 2021: member

Otherwise looks good

fanquake referenced this in commit 0ccf9b2e55 on Oct 20, 2021

sidhujag referenced this in commit f3d19d61cf on Oct 20, 2021

gwillen referenced this in commit 34da23bd1a on Jun 1, 2022

Fabcien referenced this in commit 2b80693062 on Jun 22, 2022

Fabcien referenced this in commit 121c842f62 on Jun 22, 2022

Fabcien referenced this in commit aeb423f1a1 on Jun 22, 2022

Fabcien referenced this in commit 819bf80975 on Jun 22, 2022

Fabcien referenced this in commit c5004dcda2 on Jun 29, 2022

Fabcien referenced this in commit 88f0dc11af on Jun 30, 2022

Fabcien referenced this in commit 0655265d8d on Jun 30, 2022

Fabcien referenced this in commit c392b9db21 on Jun 30, 2022

Fabcien referenced this in commit 7e8275e964 on Jun 30, 2022

Fabcien referenced this in commit 7e0e3d2c3b on Jun 30, 2022

Fabcien referenced this in commit c9189fd7a3 on Jul 15, 2022

Fabcien referenced this in commit 3151d3637e on Jul 15, 2022

Fabcien referenced this in commit f78286b329 on Jul 15, 2022

DrahtBot locked this on Aug 16, 2022