[Bundle 7/7] validation: Farewell, global Chainstate! #21866
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
This was referenced May 6, 2021
Merged
|
In the description of a839078:
Can you elaborate on this? |
This was referenced May 6, 2021
|
@Sjors bitcoin/src/rpc/blockchain.cpp Line 85 in 5986970 However from my understanding, in In other words, throwing a (btw, that commit is part of Bundle 6) |
8000
|
That makes sense, alright, I'll check out episode 6 first. |
This was referenced May 7, 2021
12 tasks
jamesob
approved these changes
Jun 10, 2021
There was a problem hiding this comment.
ACK cc8cc9c (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.
Show signature data
-----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-----
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
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
| .CreateNewBlock(coinbase_scriptPubKey) | ||
| ->block); | ||
|
|
||
| LOCK(cs_main); | ||
| block->nTime = ::ChainActive().Tip()->GetMedianTimePast() + 1; | ||
| block->nTime = Assert(node.chainman)->ActiveChain().Tip()->GetMedianTimePast() + 1; |
There was a problem hiding this comment.
Obligatory nit about using ActiveTip()
| { | ||
| CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip(); | ||
| BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 2, active_chain_tip))); // Sequence locks pass on 2nd block | ||
| } |
There was a problem hiding this comment.
I don't mind it but is there the temporary scope necessary? (Also ActiveTip() nits)
| { | ||
| CBlockIndex* active_chain_tip = m_node.chainman->ActiveChain().Tip(); | ||
| BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(active_chain_tip->nHeight + 1, active_chain_tip))); // Sequence locks pass 512 seconds later | ||
| } |
There was a problem hiding this comment.
ActiveTip() nits here if you end up rebasing.
|
reACK 6f99488 based on the contents of |
|
Code Review ACK 6f99488. Diff since last review is using |
|
utACK 6f99488 |
|
Code Review ACK 6f99488 |
ryanofsky
approved these changes
Jun 11, 2021
There was a problem hiding this comment.
Code review ACK 6f99488.
Just want to note so it isn't forgotten that 972c516 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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 14, 2021
|
Just noting that commit 972c516 causes a segmentation fault because the commits are in the wrong order: |
|
Otherwise looks good |
This was referenced Oct 12, 2021
fanquake
added a commit
that referenced
this pull request
Oct 20, 2021
a0efe52 Fix outdated comments referring to ::ChainActive() (Samuel Dobson) Pull request description: After #21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`. ACKs for top commit: jamesob: ACK a0efe52 Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 20, 2021
…ctive() a0efe52 Fix outdated comments referring to ::ChainActive() (Samuel Dobson) Pull request description: After bitcoin#21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`. ACKs for top commit: jamesob: ACK bitcoin@a0efe52 Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
gwillen
pushed a commit
to ElementsProject/elements
that referenced
this pull request
Jun 1, 2022
Paying the piper for some lazy use of globals, including ones I added in 8e770d3 Had to add another couple methods to inter 9E88 faces::Chain
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 22, 2022
Summary: This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [1/12] bitcoin/bitcoin@464c313 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11646
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 22, 2022
Summary: This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [2/12] bitcoin/bitcoin@f0dd5e6 Depends on D11646 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11647
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 22, 2022
Summary: This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [3/12] bitcoin/bitcoin@4d99b61 Depends on D11647 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11648
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 22, 2022
Summary: This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [4/12] bitcoin/bitcoin@e197076 Depends on D11648 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11649
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 29, 2022
Summary:
Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways:
1. Calling InitializeFuzzingContext, which implicitly constructs a static
const BasicTestingSetup
2. Directly constructing a static const BasicTestingSetup in the initialize_*
function
3. Directly constructing a static TestingSetup and reproducing the
initialization arguments (I'm assuming because
InitializeFuzzingContext only initializes a BasicTestingSetup)
The new, relatively-simple MakeFuzzingContext function allows us to
consolidate these methods of initialization by being flexible enough to
be used in all situations. It:
1. Is templated so that we can choose to initialize any of
the *TestingSetup classes
2. Has sane defaults which are often used in fuzzers but are also
easily overridable
3. Returns a unique_ptr, explicitly transferring ownership to the caller
to deal with according to its situation
This is a backport of [[bitcoin/bitcoin#20946 | core#20946]] [1/2]
bitcoin/bitcoin@713314a
Note: this backport is a dependency for [[bitcoin/bitcoin#21866 | core#21866]] (removal of global Chainstate)
Test Plan: `ninja bitcoin-fuzzers`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D11678
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 30, 2022
Summary: For fuzz tests that need it. This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [6/12] bitcoin/bitcoin@ee0ab1e Note: src/test/fuzz/validation_load_mempool.cpp has not been created yet in Bitcoin ABC. If/When it will eventually be backported, it will be obvious that this change is needed, as `g_chainman` will no longer exist by then. Test Plan: `ninja bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11682
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 30, 2022
Summary:
```
-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' \
-e 's@ActiveChain\(\)\.Tip\(\)@ActiveTip()@g' \
-e 's@ActiveChain\(\)\.Height\(\)@ActiveHeight()@g'
arc lint
-END VERIFY SCRIPT-
```
This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [7/12]
bitcoin/bitcoin@6c15de1
Test Plan: `ninja check`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D11683
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 30, 2022
Summary: This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [8/12] bitcoin/bitcoin@f323248 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11684
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 30, 2022
Summary: Unfortunately, these assertion don't fit the regex in the scripted-diff. Therefore, we remove it manually This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [9/12] bitcoin/bitcoin@3e82abb Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11685
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 30, 2022
Summary:
```
-BEGIN VERIFY SCRIPT-
find_regex='(assert|CHECK_NONFATAL)\(std::addressof'
git grep -l -E "$find_regex" -- . | xargs sed -i -E "/${find_regex}[^;]+;/d"
git grep -l -E "$find_regex" -- . | xargs sed -i -E "/${find_regex}/,/;/d"
git grep -l -E "TODO: REVIEW-ONLY" -- . | xargs sed -i -E "/TODO: REVIEW-ONLY/,/;/d"
-END VERIFY SCRIPT-
```
Note: the script first removes assertions that are on a single line, then removes the multiline assertions by using the ";" character as the end delimiter.
This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [10/12]
bitcoin/bitcoin@6c3b5dc
Depends on D11685
Test Plan: `ninja all check-all bitcoin-bench`
Reviewers: #bitcoin_abc, Fabien
Reviewed By: #bitcoin_abc, Fabien
Differential Revision: https://reviews.bitcoinabc.org/D11686
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jul 15, 2022
Summary: This was the last use of global chainstate in the unit tests, except for the actual test fixture setup code itself (which will be removed in the two final commits of [[bitcoin/bitcoin#21866 | core#21866]] that remain to be backported). Depends on D11739 Test Plan: `ninja check-avalanche` Check for other use of the global chainstate in unit tests: ``` grep -r "g_chainman" src/test/ src/*/test grep -r "::Chain\w*Active" src/test/ src/*/test ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11742
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jul 15, 2022
Summary: There are some mutable, global state variables that are currently reseti 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 invalid0 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. This is a backport of [[bitcoin/bitcoin#21866 | core#21866]] [11/12] bitcoin/bitcoin@972c516 Depends on D11751 and D11744 Backport note: it is not possible to drop the chainman reset code in apptests.cpp at this point, as the unit tests still use g_chainman. This has to be removed in the next commit in D11753, when we actually make the unit tests use their own chain state manager instead of the global one. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11752
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jul 15, 2022
Summary: This concludes backport of [[bitcoin/bitcoin#21866 | core#21866]] [12/12] bitcoin/bitcoin@6f99488 Depends on D11752 Backport note: the changes in qt/test/apptests.cpp are done in a different commit in the source material, but this is incorrect and belongs here (as explained in D11752) Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11753
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on: #21767
à la Mr. Sandman
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.
ChainstateManager g_chainmanCChainState& ChainstateActive()CChain& ChainActive()