Make g_chainman internal to validation #18698
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
Contributor
|
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. |
Contributor
|
Concept ACK: very welcome from a fuzzing perspective! I know that I am repeating myself but nevertheless: thanks for doing these coupling cleanups :) |
This was referenced Apr 18, 2020
This was referenced Apr 20, 2020
Member
Author
|
Rebased due to conflict in tests |
This was referenced Apr 28, 2020
ryanofsky
approved these changes
May 1, 2020
src/validation.h
Outdated
Contributor
There was a problem hiding this comment.
In commit "validation: Mark g_chainman DEPRECATED" (fa24ea2e67d9812a7b57f9820996a8695a8fef37)
Could enforce this by deleting extern declaration from validation.h, and just declaring it an extern variable right where it's used in init.cpp.
Member
Author
There was a problem hiding this comment.
I might remove it in a later pull altogether, so I opted for the smallest diff for now
This was referenced May 6, 2020
Contributor
Maybe I just liked Ariard's commit so much I just wanted to comment about it on your PR? |
This was referenced May 16, 2020
jamesob
approved these changes
May 19, 2020
Contributor
jamesob
left a comment
jamesob
left a comment
There was a problem hiding this comment.
Re-ACK faa438b (jamesob/ackr/18698.2.MarcoFalke.make_g_chainman_internal)
Verified with diff-of-diffs that the only change other than rebasing is the &g_chainman
simplification. Built locally.
Show platform data
Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic
Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion
Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2 i
Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)
Show signature data
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Re-ACK faa438bd09cb8c940092cc9d8151524e45fa1321 ([`jamesob/ackr/18698.2.MarcoFalke.make_g_chainman_internal`](https://github.com/jamesob/bitcoin/tree/ackr/18698.2.MarcoFalke.make_g_chainman_internal))
Verified with diff-of-diffs that the only change other than rebasing is the `&g_chainman`
simplification. Build locally.
<details><summary>Show platform data</summary>
<p>
Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic
Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion
Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2 i
Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)
</p></details>
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEEpm2+LMqPsWj2rAwWyrywS1A+5mgFAl7EQ90ACgkQyrywS1A+
5mhnuA/9EQOujOUD26ZnjbYkG7erMCOtdg6g9KPvk3tNtTeMnbckCDj61oxTGYAq
kkpJgrnqrr3Jcer7Znz423BNKqWruakfdNN+gZyZY5WHLsB3A4e5j5bGbu4c1G8f
kXU02uP+YcNj9hqd1zpz6Op1HEaju9LWEeDwnLR7ER6enxC3+tAGhyUY37IQ90eD
xR7WaES5IWnQL2ghvxXeTC1ZI97kwRcGfyrNso/VjR8se8IumLq2pToSMFYC7P0b
ksgZ+uMNHkAQvamvKS5JXhMrIdO0XXGwnmJgb7ZifB8q4S1j2VNBamwZb2wDrGX9
Skro52HsVxuC+Y4fTMcDZO8fYUGBzSJKN0r33qwzFH0/p38wHplr4bFKVRmXdtM/
H0Xnmf6fJNil2wRCz5ZQsjz7t/N0jFdPa1ffLxygCevS3+vR01gR1RknriIppoSh
wAB4OfFvKw6UHAEQoJ3SpTkEI/Px9PcfJ0GbJgugH+3bgqme/eGRU2lowVkPvG4s
uRMKQsi8pscCA65L45hL0I2lC5RJx3fX5LrJxbXPD6NW85ALx0y3Bb3Nsi30fQh/
wWfvvuXetUDcMyvzW5cOxQIdt8anWsu0X046W3U+74KgtEzhZiwJiTZjIHL6I4Z/
CbYR9nX3qhXiO7/h7yiJRJSV6TDQI5rVgZUrvGYNZriyQo3s8/s=
=hqDd
-----END PGP SIGNATURE-----
added 6 commits
May 21, 2020 09:55
Member
Author
|
The rebases are trivial and I don't mind them, but letting this sit in the rebase-ping-pong doesn't make the changes any better over time. Is there anything left to be done here before merge? |
Contributor
Not that I can think of, this looks good to me |
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 1, 2020
Summary: Partial backport (1/6) of core [[bitcoin/bitcoin#18698 | PR18698]]: bitcoin/bitcoin@fa7b626#diff-d3c243938494b10666b44404a27af7d84b44a72b85a27431e0c89e181462ca6e Depends on D8553. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8560
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 1, 2020
Summary: Partial backport (2/6) of core [[bitcoin/bitcoin#18698 | PR18698]]: bitcoin/bitcoin@fa05fdf Depends on D8560. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8561
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 1, 2020
Summary: Partial backport (3/6) of core [[bitcoin/bitcoin#18698 | PR18698]]: bitcoin/bitcoin@fa84b1c Depends on D8561. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8562
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 1, 2020
Summary: Partial backport (4/6) of core [[bitcoin/bitcoin#18698 | PR18698]]: bitcoin/bitcoin@fa24d49 Depends on D8562. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8563
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 1, 2020
Summary: Partial backport (5/6) of core [[bitcoin/bitcoin#18698 | PR18698]]: bitcoin/bitcoin@fa1d97b Depends on D8563. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8564
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 1, 2020
Summary: Completes backport (6/6) of core [[bitcoin/bitcoin#18698 | PR18698]]: bitcoin/bitcoin@fab6b9d Depends on D8564. Test Plan: ninja Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8565
maflcko
pushed a commit
that referenced
this pull request
Nov 10, 2021
0fdb619 [validation] Always call mempool.check() after processing a new transaction (John Newbery) 2c64270 [refactor] Don't call AcceptToMemoryPool() from outside validation.cpp (John Newbery) 92a3aee [validation] Add CChainState::ProcessTransaction() (John Newbery) 36167fa [logging/documentation] Remove reference to AcceptToMemoryPool from error string (John Newbery) 4c24142 [validation] Remove comment about AcceptToMemoryPool() (John Newbery) 5759fd1 [test] Don't set bypass_limits to true in txvalidation_tests.cpp (John Newbery) 497c9e2 [test] Don't set bypass_limits to true in txvalidationcache_tests.cpp (John Newbery) Pull request description: Similarly to how #18698 added `ProcessNewBlock()` and `ProcessNewBlockHeaders()` methods to the `ChainstateManager` class, this PR adds a new `ProcessTransaction()` method. Code outside validation no longer calls `AcceptToMemoryPool()` directly, but calls through the higher-level `ProcessTransaction()` method. Advantages: - The interface is simplified. Calling code no longer needs to know about the active chainstate or mempool object, since `AcceptToMemoryPool()` can only ever be called for the active chainstate, and that chainstate knows which mempool it's using. We can also remove the `bypass_limits` argument, since that can only be used internally in validation. - responsibility for calling `CTxMemPool::check()` is removed from the callers, and run automatically by `ChainstateManager` every time `ProcessTransaction()` is called. ACKs for top commit: lsilva01: tACK 0fdb619 on Ubuntu 20.04 theStack: Code-review ACK 0fdb619 ryanofsky: Code review ACK 0fdb619. Only changes since last review: splitting & joining commits, adding more explanations to commit messages, tweaking MEMPOOL_ERROR string, fixing up argument name comments. Tree-SHA512: 0b395c2e3ef242f0d41d47174b1646b0a73aeece38f1fe29349837e6fb832f4bf8d57e1a1eaed82a97c635cfd59015a7e07f824e0d7c00b2bee4144e80608172
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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.
The global
g_chainmanhas recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.
I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.