The PR branch HEAD was b04370d at the time of this review club meeting.
Notes
This PR is part of a project to de-globalize g_chainman. Issue
20049 describes the high-level
goal and motivations for that project. This topic was also discussed in a
previous review club on PR 20158, “De-globalize ChainstateManager.”
g_chainman
is currently a global variable, or in other words, a non-local static
storage duration variable. Global variables are initialized during
startup,
before the main() function is invoked.
Usage of global variables makes it difficult to modularize the code base:
Since the variable is initialized before main(), it can’t be constructed
with parameters that are only known at runtime (e.g. configuration
options).
It’s difficult to test global objects, since the test can’t
construct/destruct new instances of the object, and other components may
make changes to the global object during the test.
There’s no concept of ownership of a global object. We have no control over
when the memory for the object is allocated or freed, or the order in which
different global objects are constructed/destructed.
The use of globals leads to tight coupling between components. Other
components will have undocumented dependencies on the global object, which
makes it difficult to enforce a well-defined interface to the object.
The NodeContext
object
was added in PR 16839. The PR description
and code comment clearly describe the rationale and benefits of managing the subcomponents
in this way.
Over time, other subcomponents have been moved to the NodeContext object,
for example CTxMemPool (PR
19556) and
CBlockPolicyEstimator (PR
18766).
Why is it added in this PR (and other PRs in the series)?
Why does the last commit in PR
21866 change
NodeContext.chainman from a raw pointer to a std::unique_ptr? What
ownership semantics does a unique_ptr connote? What are the benefits of
using a smart pointer (unique_ptr or shared_ptr) over a raw pointer?
<jnewbery> I'll be asking questions from https://bitcoincore.reviews/21767 to guide the conversation, but feel free to jump in with questions at any time.
<jarolrod> concept ack, don't like that we are exposing CChain in b04370d, but this can be addressed in a follow-up making changes to how we interact with that
<willcl_ark> Definite concept ACK. I also really appriciate the way dongcarl splits the commits so meticulously to make review logical/simple where possible
<jnewbery> ok, next question: This PR is a refactor and is not supposed to change any functional behaviour. What are some ways we can verify that behaviour doesn’t change?
<willcl_ark> I was thinking you could *not* apply commits which touch the functional tests and check that all (the old) tests still pass with the new code
<jnewbery> ok, let's move on to the next question: This series of PRs is part of a larger project to “modularize our consensus engine”. What are some of the benefits of doing that?
<willcl_ark> A single modular "consensus engine" is easier to reason about from a bug perspective than consensus codepaths which spread over multiple modules. The initalisation benefits for testing also seem pretty appetising.
<jarolrod> as to the greater bitcoin ecosystem, plug and play consensus modules for developers. Confidence that your bitcoin application won't fork you off the chain because you are just using the core consensus engine
<larryruane__> makes testing easier; global variables are bad because a test may change them, and then the next test is messed up ... so each test must remember to restore their initial states
<larryruane__> an important thing in unit testing is that each test should run independently -- if a test passes (or fails) as part of a group of tests, the same result should happen if run individually
<jnewbery> Right. Satoshi used hungarian notation (https://en.wikipedia.org/wiki/Hungarian_notation) where the type of the variable is reflected in the name, eg CChainState is a Class, fDiscover is a flag (bool), nBestScore is an int, etc
<jnewbery> the current style is not to use hungarian notation. We use m_ to represent member variables, g_ to represent globals, and no prefix to represent local variables/parameters.
<jnewbery> funnctions/methods use CamelCase. The interface methods in src/interface/* are the same except the first character is lower case (eg acceptToMemoryPool instead of AcceptToMemoryPool)
<jnewbery> I think the various classes (ChainstateManager/CChainstate/CChain/BlockManager) that are used as an interface into validation are too low-level and expose too much of validation's implementation to the rest of the program, but that can be cleaned up once we have a well-defined interface
<jnewbery> jamesob_: interesting. Wikipedia claims that camel case refers to both: https://en.wikipedia.org/wiki/Camel_case, but PascalCase specifically means with leading uppercase
<jnewbery> YErzhan: we discourage changes that are aesthetic refactors only. We'll change naming as that code is touched, otherwise it's unnecessary churn
<jamesob_> YErzhan: refactoring core code is tricky. Too much code churn at once creates review burden, potential for error, and can hide vulnerabilities, so refactoring has to be done sparingly and usually in the service of a very concrete goal
<larryruane__> jamesob_: Yes, and another thing is, we should always be aware of how many projects have forked Core, and made their own changes! Nightmare for them to merge changes
<jnewbery> michaelfolkson: churn as in changes to the codebase. Every time that you rename something in a header file, everyone needs to recompile any file that includes that header.
<jarolrod> looks like cs_main locks access to CChainstate's, as so doesn't it make sense to refactor cs_main to be more self contained inside of the ChainstateManager interface
<larryruane__> jnewbery: I think it figures prominently in this PR because it protects the global `g_chainman` and reducing use of that variable means reducing use of `cs_main` (great!)
<levitate> When the word "snapshot" is used in. CChainStates, what does that word mean in that context? is it: an externally submitted chainstate the node will consider potentially valid?
<jnewbery> a long-term goal is to separate out those different bits of data that are protected by cs_main, so that eventually cs_main only protects the data inside validation
<jnewbery> levitate: snapshot in this instance refers to a chainstate that is based on a UTXO set that is provided to your node (and doesn't start from the genesis block). It's part of the AssumeUTXO project. There's excellent documentation here: https://github.com/bitcoin/bitcoin/issues/15605
<jnewbery> larryruane: conceptually validation stores and maintains our best view of the blockchain and associated UTXO set. It also includes an interface to submit unconfirmed transactions to the mempool
<michaelfolkson> levitate: It can be used to skip a lot of verification of the blockchain (which would be frowned upon if you are introducing trust in a snapshot). But it also could be used (in concept) to verify different parts of the blockchain at the same time
<jnewbery> Next question: In commit rest: Add GetChainman function and use it, what does the EnsureAnyChainman() function do? Why is it being removed from rest.cpp?
<marqusat> extracts ChainstateManager reference or throws exception. it’s being replaced with GetChainman which returns nullptr instead of throwing exception. nullptr is then handled not crashing the execution
<jnewbery> right, the Ensure* functions are used in the RPC code and throw exceptions. Those exceptions are caught and returned to the client as errors.
<dongcarl> michaelfolkson: As jnewbery said: RPC code wraps all calls in a try/except, REST code does not. Ensure*, being part of RPC, expects that its throw's will get caught by a try/except. But if you use Ensure* in REST code, since it doesn't have a try/except wrap, a crash will happen.
<larryruane__> just wondering for testing, is there a way to start a local bitcoind and hit those rest endpoints? (I'm also curious how the rest interface works)
<jnewbery> I think these temporary asserts are a good idea to give us confidence in this refactor. The first PRs in the series were merged several weeks/months ago, and the asserts haven't been hit, so we can be quite confident that the objects really are the same objects!