[bundle 6/n] Prune g_chainman usage in auxiliary modules (refactoring)

https://github.com/bitcoin/bitcoin/pulls/21767

Host: jnewbery  -  PR author: dongcarl

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).

  • The end goal of issue 20049 is to remove the global g_chainman, so that the ChainstateManager object is owned by the NodeContext object.

  • PR 21767 removes the usage of g_chainman in various components.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

  2. 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?

  3. This series of PRs is part of a larger project to “modularize our consensus engine”. What are some of the benefits of doing that?

  4. Briefly, what are each of the following classes responsible for:

    • ChainstateManager
    • CChainState
    • CChain
    • BlockManager
  5. What is cs_main? Why does it feature so prominently in the changes in this PR?

  6. In commit rest: Add GetChainman function and use it, what does the EnsureAnyChainman() function do? Why is it being removed from rest.cpp?

  7. What does the following code do?

    assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman));

    Why is it added in this PR (and other PRs in the series)?

  8. 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?

Meeting Log

  119:00 <jnewbery> #startmeeting
  219:00 <levitate> hi
  319:00 <marqusat> hi
  419:00 <jnewbery> Hi folks! Welcome to Bitcoin Core Review Club.
  519:00 <willcl_ark> hi
  619:00 <michaelfolkson> hi
  719:00 <jarolrod> hi
  819:00 <larryruane__> hi
  919:00 <glozow> hi
 1019:00 <jnewbery> This week, we'll be talking about PR 21767. Notes and questions are on the website: https://bitcoincore.reviews/21767
 1119:00 <nehan_> hi
 1219:00 <jnewbery> Feel free to say hi to let everyone know you're here.
 1319:00 <dongcarl> hi
 1419:00 <jnewbery> Is anyone here for the first time?
 1519:00 <dkf> hi
 1619:01 <schmidty> hi
 1719:01 <amiti> hi
 1819:02 ℹ nehan_ is now known as nehan
 1919:02 <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.
 2019:02 <YErzhan> Hi thanks!
 2119:03 <jnewbery> Who had a chance to review the PR? (y/n)
 2219:03 <jarolrod> y
 2319:03 <levitate> n
 2419:03 <michaelfolkson> y
 2519:03 <willcl_ark> y
 2619:03 <hernanmarino> hi !
 2719:03 <nehan> y
 2819:03 <marqusat> just at high-level
 2919:03 <amiti> n
 3019:03 <glozow> n
 3119:03 <jnewbery> Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?
 3219:04 <hernanmarino> y, high level only at the notes. didn' t review de PR yet
 3319:04 <marqusat> Concept ACK
 3419:04 <ben13> Concept ACK. Reducing the use of global variables is good.
 3519:04 <michaelfolkson> I'm a Concept ACK on the 7 PRs but it takes a while to understand the Approach ACKs on the individual PRs and still not fully there
 3619:04 <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
 3719:04 <larryruane__> yes, minimal testing, concept & approach ACK
 3819:05 <willcl_ark> Definite concept ACK. I also really appriciate the way dongcarl splits the commits so meticulously to make review logical/simple where possible
 3919:05 <jnewbery> jarolrod: Yes, let's talk about CChain a bit later!
 4019:05 <dongcarl> Thanks willcl_ark!
 4119:05 <michaelfolkson> I guess we are focusing on this particular PR rather than all 7
 4219:05 <jonatack> hi
 4319:05 <jnewbery> willcl_ark: totally agree. These PRs are a joy to review
 4419:05 <jarolrod> michaelfolkson: yep
 4519:05 <glozow> there was a review club on the full one iirc
 4619:06 <glozow> https://bitcoincore.reviews/20158
 4719:06 <michaelfolkson> glozow: https://bitcoincore.reviews/20158
 4819:06 <michaelfolkson> Yeah I guess that one was for high level all 7
 4919:06 <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?
 5019:07 <ben13> Running functional testes
 5119:07 <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
 5219:07 <YErzhan> Bitcoin CI system should catch that by running unit tests
 5319:07 <marqusat> by running tests (potentially expanding tests first in the case of any gaps in coverage identified)
 5419:07 <larryruane__> running unit tests, starting up a real node (letting it sync with the network)
 5519:08 <glozow> I sometimes add a bunch of asserts for would-be-obvious things and run functional tests
 5619:08 <jarolrod> through good testing, but the assert statements give some confidence to the move
 5719:08 <jnewbery> excellent answers everyone!
 5819:09 <michaelfolkson> glozow: So too obvious asserts that shouldn't be added to the code?
 5919:09 <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?
 6019:09 <ben13> Code easier to maintain and reuse.
 6119:09 <michaelfolkson> glozow: I guess things you want to assert yourself but might be obvious to others
 6219:09 <jnewbery> ben13: I agree. Can you expand a bit?
 6319:10 <marqusat> easier to reason about the code, better configurability and testability
 6419:10 <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.
 6519:10 <dkf> decouple it from the core engine to expose as little as needed for security and maintainability
 6619:10 <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
 6719:10 <jarolrod> very cool chaincode labs podcast episode
 6819:10 <YErzhan> Easier to fork bitcoin into new altcoins for everyoneX-P
 6919:10 <ben13> "very cool chaincode labs podcast episode" where ? jarolrod
 7019:10 <jarolrod> it's also cleaner code for bitcoin core devs
 7119:11 <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
 7219:11 <glozow> michaelfolkson: yes. such as checking that 2 numbers add up to what we expect, a data member is properly filled, a lock is held, etc.
 7319:11 <jnewbery> marqusat: yes. Can you give a bit more detail on configurability and testability?
 7419:11 <jarolrod> ben13: https://podcast.chaincode.com/2020/12/15/carl-dong-2.html
 7519:11 <ben13> Thanks jarolrod
 7619:11 <michaelfolkson> ben13: Or transcript https://btctranscripts.com/chaincode-labs/chaincode-podcast/2020-11-30-carl-dong-reproducible-builds/
 7719:12 <ben13> Thanks michaelfolkson
 7819:12 <jnewbery> larryrane__: Yes, this should make testing much easier, and allow more test setups
 7919:12 <glozow> michaelfolkson: sometimes i just add many copies of the same assert before and after calls. would be too verbose to actually add
 8019:12 <michaelfolkson> glozow: Cool, makes sense
 8119:12 <dongcarl> assert-based code exploration is underrated for how powerful it is
 8219:12 <ben13> There is also some configuration in chainparams.{h,cpp}
 8319:13 <marqusat> jnewbery: launch params can be used to configure non-global data
 8419:13 <jnewbery> No-one has mentioned using debuggers to step through the code. Does anyone use that regularly?
 8519:13 <dongcarl> marqusat: yes, that one is pretty big in my opinion
 8619:14 <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
 8719:14 <levitate> jnewbery as in: removing globals helps debugger use or no one has mentioned using a debugger to audit a PR?
 8819:14 <michaelfolkson> jnewbery: I have used pdb on functional tests (Python) but haven't used debuggers on C++ code enough yet
 8919:14 <larryruane__> jnewbery: yes i actually have been stepping through this PR's code
 9019:15 <jnewbery> marqusat: right, and we can construct the components with parameters, such as here for PeerManager: https://github.com/bitcoin/bitcoin/blob/6b49d88a/src/net_processing.h#L37-L39
 9119:15 <ben13> I use gdb in VS Code to debug Bitcoin Core.
 9219:15 <jnewbery> and that gives us more control over testing those objects with different configuration
 9319:15 <larryruane__> if using debugger, important to build with `CONFIGURE_FLAGS='CXXFLAGS=-O0'`
 9419:15 <michaelfolkson> Also this enables projects like jamesob AssumeUTXO https://github.com/bitcoin/bitcoin/pull/17737
 9519:16 <jnewbery> michaelfolkson: I think those things are distinct. Maybe AssumeUTXO is easier with a better defined interface with validation though
 9619:16 <dongcarl> larryruane__: `--enable-debug` also works I think
 9719:16 <ben13> AssumeUTXO requires 2 chainstates
 9819:17 <michaelfolkson> jnewbery: Maybe this particular PR is distinct. I think the 7 PRs on the whole help enable AssumeUTXO
 9919:17 <larryruane__> dongcarl: yes I think that enables more checking code as well, good point
10019:17 <ben13> Is there any difference between `--enable-debug` and `CONFIGURE_FLAGS='CXXFLAGS=-O0'` ?
10119:17 <jnewbery> I think it might be useful to do a PR review club just on using gdb/lldb. They're really powerful tools
10219:17 <jarolrod> jnewbery: that would be nice
10319:17 <larryruane__> i'd be willing to contribute (to leading)
10419:17 <jnewbery> if anyone feels like hosting that, let me know!
10519:17 <ben13> Great. I would be great.
10619:18 <glozow> i nominate larryruane__
10719:18 <willcl_ark> I would certainly be interested at becoming more proficient in them
10819:18 <michaelfolkson> +1
10919:18 <jnewbery> larryruane__: thank you!
11019:18 <biteskola> +1
11119:18 <larryruane__> except I don't know VSCode so maybe have someone else for that
11219:18 <jnewbery> Next question: Briefly, what are each of the following classes responsible for:
11319:18 <jnewbery> - ChainstateManager
11419:18 <jnewbery> - CChainState
11519:19 <jnewbery> - CChain
11619:19 <jnewbery> - BlockManager
11719:19 <ben13> ChainstateManager -> manages 2 differents chainstats (IBD and snapshot)
11819:19 <ben13> CChainState -> represents the current UTXO (cache and disk - the chainstate/* LevelDB database)
11919:19 <jnewbery> (and bonus question: why do CChainState and CChain have a leading 'C', but ChainstateManager and BlockManager don't?)
12019:19 <ben13> CChain -> an in-memory indexed chain of blocks.
12119:19 <ben13> BlockManager -> Maintains a tree of blocks (stored in `m_block_index`)
12219:20 <YErzhan> Satoshi made a typo?
12319:20 <glozow> c = class, hungarian style naming?
12419:20 <larryruane__> jnewbery: because not having the C is the new convention (the old is C for Class)
12519:20 <jnewbery> ben13: very good. CChainState contains more than just the UTXO set though.
12619:21 <ben13> hungarian style. Isn't this used in Bitcoin Core code anymore ?
12719:21 <willcl_ark> CChainState is an general API into the details of our current most work chain.
12819:22 <larryruane__> ben13: no we're getting away from it as code is added / changed
12919:22 <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
13019:22 <jamesob_> willcl_ark: that's less true now that CChainState can correspond to a background validation chainstate
13119:22 <michaelfolkson> Global ChainstateManager = g_chainman
13219:22 <YErzhan> If CChainState contains UTXO, why not name it UTXOState for simplicity?
13319:22 <jamesob_> YErzhan: it contains more than just the UTXO set
13419:23 <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.
13519:23 <larryruane__> may be useful? https://doxygen.bitcoincore.org/class_chainstate_manager.html https://doxygen.bitcoincore.org/class_c_chain_state.html
13619:23 <YErzhan> So, why prefix with C then?
13719:23 <jamesob_> (and ryanofsky has been using camelCaseForMethods recently)
13819:24 <michaelfolkson> I looked up when CChain was introduced. sipa in 2013 removing other globals :) https://github.com/bitcoin/bitcoin/pull/3077
13919:24 <ben13> YErzhan C means Class
14019:24 <willcl_ark> interesting thanks jamesob_
14119:24 <ben13> I think
14219:24 <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)
14319:25 <YErzhan> ben13 BlockManager is also a class I think
14419:26 <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
14519:26 <jamesob_> jnewbery: dumb nit, but it's actually called PascalCase (thisIsCamelCase)
14619:27 <ben13> YErzhan It is a recent implementation, It does not use Hungarian style.
14719:27 <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
14819:27 <dongcarl> jnewbery: Agree that the interfaces can be tidied!
14919:27 <ben13> ChainstateManager and BlockManager are more recent. CChainstate and CChain is legacy. I think.
15019:28 <YErzhan> So, why not refactor for consistency?
15119:28 <jnewbery> and dromedaryCase is with leading lowercase
15219:28 <jnewbery> ben13: the naming is legacy, the classes are very much still used as part of the interface
15319:28 <ben13> Yes. The naming is legacy.
15419:29 <jnewbery> YErzhan: we discourage changes that are aesthetic refactors only. We'll change naming as that code is touched, otherwise it's unnecessary churn
15519:29 <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
15619:29 <michaelfolkson> jnewbery: Churn as in rebasing of open PRs and docs? Anything else that is being churned?
15719:30 <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
15819:30 <jnewbery> dongcarl: that wasn't a criticism of this PR, by the way. Clarifying the interface is a prerequisite to cleaning it up later
15919:30 <glozow> increases depth for git blames
16019:30 <dongcarl> :-)
16119:30 <michaelfolkson> glozow: Haha
16219:31 <dongcarl> at some point we should maintain a --ignore-revs-file
16319:31 <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.
16419:31 <dongcarl> (and put all the chainman refactor commits there)
16519:31 <michaelfolkson> jnewbery: Gotcha, thanks
16619:31 <jnewbery> and if you're switching branches often and rebuilding, that becomes very tiresome
16719:32 <jnewbery> ok, any more questions about the various classes we're dealing with, or is everyone happy to move on?
16819:32 <willcl_ark> is CChain only headers, or headers and blocks? Curious as it says "in-memory"
16919:33 <larryruane__> I'm sorry we were talking about naming a lot, what is the substantive differences among these 4 classes?
17019:33 <michaelfolkson> jnewbery: What is your best short definitions of those four? Or how you think of them?
17119:34 <dongcarl> ChainstateManager manages multiple CChainState's, which each manages a single CChain
17219:34 <jnewbery> the doxygen comments are good summaries:
17319:34 <jnewbery> ChainstateManager: Provides an interface for creating and interacting with one or two chainstates
17419:34 <jnewbery> CChainState: Stores and provides an API to update our local knowledge of the current best chain.
17519:34 <jnewbery> CChain: An in-memory indexed chain of blocks.
17619:34 <jnewbery> BlockManager: Maintains a tree of blocks (stored in `m_block_index`) which is consulted to determine where the most-work tip is.
17719:34 <jamesob_> willcl_ark: it's only CBlockIndex instances, which provide information about how to retrieve the block data from disk
17819:34 <willcl_ark> ah
17919:34 <michaelfolkson> Ah I didn't see the doxygen comments, cool
18019:35 <ben13> The 2 CChainStates are the IBD one and the snapshot one.
18119:35 <dkf> ben13: what does IBD mean?
18219:35 <ben13> Initial Block Download
18319:36 <jnewbery> larryruane__: does that answer your question, or do you have specific questions?
18419:36 <larryruane__> jnewbery: that was perfect, thanks
18519:36 <jnewbery> dkf: IBD is the initial sync where your node downloads and validates the complete historic block chain
18619:36 <YErzhan> In what scenarios we might need 2 ChainStates?
18719:36 <dkf> thanks ben13, it has to do with this then re: syncing node https://bitcoin.org/en/full-node#special-cases
18819:37 <jarolrod> YErzhan: you have an IBD chainstate and you could have a snapshot chainstate from passing in a assumeutxo snapshot
18919:37 <ben13> 2 chainstates are required for AssumeUTXO. One cs is the default, from block sync. And the other, you download the UTXO Set from your peers.
19019:38 <jarolrod> YErzhan: only one of these will be the active chainstate though
19119:38 <jnewbery> dkf: did you mean to link to https://bitcoin.org/en/full-node#initial-block-downloadibd ?
19219:38 <dkf> jnewberry: yes
19319:38 <ben13> Yes, just one. There is a `m_active_chainstate` member, something like that
19419:39 <jnewbery> I'll ask the next question, but if there's anything that's still unclear about the classes, please ask
19519:39 <jnewbery> What is cs_main? Why does it feature so prominently in the changes in this PR?
19619:39 <larryruane__> important thing to help you understand `CChain` is it contains a `std::vector` (of block index pointers)
19719:39 <larryruane__> indexed by height (i believe)
19819:39 <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
19919:39 <ben13> It is a lock to synchronize changes in the Chain State.
20019:39 <michaelfolkson> This AssumeUTXO use case could be verifying different portions of the blockchain in parallel....?
20119:40 <willcl_ark> cs_main is a recursive mutex which is used to ensure that validation is carried out in an atomic way.
20219:40 <marqusat> Mutex to guard access to validation specific variables (such as chainstate) or updating mempool
20319:40 <michaelfolkson> 0-25% of the blockchain being verified at the same time as 25-50% of the blockchain
20419:40 <ben13> Multiple threads can access the chain state at the same time. cs_main syncs this.
20519:41 <jnewbery> ben13 willcl_ark marqusat: very good. Yes, it's a mutex that protects validation-specific data.
20619:41 <jnewbery> it also protects a whole load of other stuff, mostly in net_processing
20719:41 <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!)
20819:41 <jamesob_> michaelfolkson: in concept, yes - the n-chainstate approach would be abstracted under the chainstatemanager
20919:41 <jnewbery> any idea why it's called cs_main?
21019:42 <larryruane__> `main.cpp` ?
21119:42 <jnewbery> larryruane__: bingo!
21219:42 <ben13> Was ChainState originally in main.cpp ?
21319:42 <jnewbery> the code that is now in validation.cpp and net_processing.cpp used to be in one file called main.cpp
21419:42 <larryruane__> ben13: i think everything was originally there :)
21519:42 <michaelfolkson> jamesob_: So you get that n high enough and you are looking at a *very* quick verification time. Any limit on that n?
21619:42 <jnewbery> and cs_main was the mutex ("critical section") that protected data in main.cpp
21719:43 <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?
21819:43 <ben13> larryruane__ it is true
21919:43 <larryruane__> levitate: excellent question, wondering the same thing
22019:43 <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
22119:44 <larryruane__> if this isn't too much of a sidetrack, jnewbery can you give us a one-line summary of what "validation" includes?
22219:45 <larryruane__> i know there's `validation.{h,cpp}` but conceptually?
22319:45 <larryruane__> (it's ok if we want to move on)
22419:45 <michaelfolkson> jamesob_: And you do say "in concept" too :) So that wouldn't currently be possible if all the assumeutxo PRs were merged?
22519:46 <ben13> levitate I think so. There will be the hash of UTXO Set hardcoded for checkpoint, like `assumevalid` I think.
22619:46 <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
22719:47 ⚡ levitate mentally maps the word snapshot -> UTXO_proposal
22819:47 <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
22919:48 <larryruane__> jnewbery: thank you, that's very helpful
23019:48 <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
23119:48 <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?
23219:49 <willcl_ark> I think EnsureAnyChainman is determining whether we have a synced chain which we can use to answer RPC calls
23319:50 <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
23419:50 <willcl_ark> we are now delegating this
23519:51 <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.
23619:52 <jnewbery> The REST code isn't wrapped in a try/except, so if that function throws then the program aborts
23719:52 <jnewbery> so this PR replaces a call to Ensure* with code that won't throw if there isn't a chainman
23819:53 <larryruane__> jnewbery: oh so this PR includes a bug fix?
23919:53 <dongcarl> larryruane__: A bug introduced by me in the last bundle unfortunately :-(
24019:53 <larryruane__> :) oh i see
24119:53 <willcl_ark> hehe
24219:54 <michaelfolkson> dongcarl: Can you explain the bug?
24319:54 <jnewbery> if you introduce bug in a PR in a series it really motivates people to review the next one :)
24419:54 <michaelfolkson> Haha deliberate strategy
24519:54 <jnewbery> next question:
24619:54 <jnewbery> What does the following code do?
24719:54 <jnewbery> assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman));
24819:54 <jnewbery> Why is it added in this PR (and other PRs in the series)?
24919:55 <larryruane__> is it temporary to ensure correct transition (to eliminating g_chainman completely)?
25019:56 <larryruane__> baby steps :)
25119:56 <jnewbery> larryruane__: yes!
25219:56 <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.
25319:56 <willcl_ark> checks that the global is using the same object as the new reference?
25419:56 <willcl_ark> well, asserts!
25519:57 <jnewbery> willcl_ark: exactly
25619:57 <jnewbery> https://github.com/bitcoin/bitcoin/pull/21866 finally removes g_chainman, and removes all of these temporary asserts too
25719:57 <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)
25819:57 <michaelfolkson> dongcarl: So using the RPC resulted in crashing after the PR was merged?
25919:58 <larryruane__> michaelfolkson: i think only rest, not RPC
26019:58 <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!
26119:58 <michaelfolkson> larryruane__: Oh sorry I misread, right
26219:59 <dongcarl> The asserts helped me find things that resulted in: #20323
26319:59 <michaelfolkson> larryruane__: Using REST after that PR was merged resulted in crashes
26419:59 <jnewbery> There's one final question about raw pointers and smart pointers, but we'll leave that as an exercise for the reader
26519:59 <jnewbery> any final questions in the last minute?
26619:59 <larryruane__> could the assert compare g_chainman to chainman?
26720:00 <jnewbery> dongcarl: very cool
26820:00 <willcl_ark> larryruane__: without using std::addressof()?
26920:00 <dongcarl> larryruane__: Sure! That would have worked too
27020:01 <jnewbery> ok, that's time. Thanks dongcarl for giving us a very nice PR to review and talk about :)
27120:01 <jnewbery> #endmeeting