[Bundle 6/n] Prune g_chainman usage in auxiliary modules

dongcarl commented at 8:13 pm on April 23, 2021: member

Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)

The first 2 commits are fixups addressing review for the last bundle: #21391

NEW note:

I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!

Note to reviewers:

This bundle may apparently introduce usage of g_chainman or ::Chain(state|)Active() globals, but these are resolved later on in the overall PR. Commits of overall PR
There may be seemingly obvious local references to ChainstateManager or other validation objects which are not being used in callers of the current function in question, this is done intentionally to keep each commit centered around one function/method to ease review and to make the overall change systematic. We don’t assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. Commits of overall PR
When changing a function/method that has many callers (e.g. LookupBlockIndex with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add new_function, make old_function a wrapper of new_function, divert all calls to old_function to new_function in the local module only
2. Scripted-diff to divert all calls to old_function to new_function in the rest of the codebase
3. Remove old_function

DrahtBot added the label Mining on Apr 23, 2021

DrahtBot added the label RPC/REST/ZMQ on Apr 23, 2021

DrahtBot added the label UTXO Db and Indexes on Apr 23, 2021

DrahtBot commented at 1:14 am on April 24, 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:

#21726 (Improve Indices on pruned nodes via prune blockers by fjahr)
#21526 (validation: UpdateTip/CheckBlockIndex assumeutxo support by jamesob)
#15606 ([experimental] UTXO snapshots by jamesob)

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.

MarcoFalke added the label Refactoring on Apr 24, 2021

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

laanwj removed the label Mining on May 5, 2021

laanwj removed the label RPC/REST/ZMQ on May 5, 2021

laanwj removed the label UTXO Db and Indexes on May 5, 2021

laanwj commented at 12:09 pm on May 5, 2021: member

Looks like this accumulated a silent merge conflict:

0…/bitcoin/src/test/coinstatsindex_tests.cpp:35:28: error: too few arguments to function call, single argument 'active_chainstate' was not specified
1    coin_stats_index.Start();
2    ~~~~~~~~~~~~~~~~~~~~~~ ^
3…/bitcoin/src/index/base.h:122:10: note: 'Start' declared here
4    void Start(CChainState& active_chainstate);
5         ^
61 error generated.

dongcarl force-pushed on May 5, 2021

in src/rest.cpp:120 in a839078c20 outdated

115+ * @returns        Pointer to the chainstatemanager or nullptr if none found.
116+ */
117+static ChainstateManager* GetChainman(const std::any& context, HTTPRequest* req)
118+{
119+    auto node_context = util::AnyPtr<NodeContext>(context);
120+    if (!node_context) {

ryanofsky commented at 0:28 am on May 7, 2021:

In commit “rest: Add GetChainman function and use it” (a839078c20f4ac3d92e14af7b287f2f0fddbc61e)

It seems like this should say if (!node_context || !node_context->chainman) otherwise the http error won’t be set in the null chainman case.

in src/rest.cpp:121 in a839078c20 outdated

116+ */
117+static ChainstateManager* GetChainman(const std::any& context, HTTPRequest* req)
118+{
119+    auto node_context = util::AnyPtr<NodeContext>(context);
120+    if (!node_context) {
121+        RESTERR(req, HTTP_NOT_FOUND, "Chainman disabled or instance not found");

ryanofsky commented at 0:30 am on May 7, 2021:

In commit “rest: Add GetChainman function and use it” (a839078c20f4ac3d92e14af7b287f2f0fddbc61e)

If this condition is never expected, then maybe this should use HTTP_INTERNAL_SERVER_ERROR to flag that this is a bug in request handling, not an invalid request.

in src/rest.cpp:674 in a839078c20 outdated

666@@ -644,7 +667,9 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
667 
668     CBlockIndex* pblockindex = nullptr;
669     {
670-        ChainstateManager& chainman = EnsureAnyChainman(context);
671+        ChainstateManager* maybe_chainman = GetChainman(context, req);

ryanofsky commented at 0:33 am on May 7, 2021:

In commit “rest: Add GetChainman function and use it” (a839078c20f4ac3d92e14af7b287f2f0fddbc61e)

re: “Someone can probably make this a bit prettier,” your current version of this seems fine to me, but if you wanted to make it more minimal you could use CHECK_NONFATAL and catch the NonFatalCheckError exception in here, I think: https://github.com/bitcoin/bitcoin/blob/06d573f053c63eb6521c62d210c5924418ae2b83/src/rest.cpp#L699

dongcarl commented at 7:07 pm on May 7, 2021:

Hmmmm, how could we differentiate between NOT_FOUND like the “no mempool”-case and INTERNAL_SERVER_ERROR like the “no Context/Chainman”-case here?

ryanofsky commented at 8:26 pm on May 7, 2021:

re: #21767 (review)

Hmmmm, how could we differentiate between NOT_FOUND like the “no mempool”-case and INTERNAL_SERVER_ERROR like the “no Context/Chainman”-case here?

I didn’t realize that was intentional and I do think the your approach is fine, but you wanted to use the NonFatalCheckError approach, you would just add:

 0class PageNotFoundError : public std::runtime_error
 1{
 2    using std::runtime_error::runtime_error;
 3};
 4
 5...
 6
 7throw PageNotFoundError{};
 8
 9...
10
11} catch (const PageNotFoundError&) {

ryanofsky commented at 8:36 pm on May 7, 2021:

Oh I see see the mempool code you’re referring to is preexisting. That would more incline towards following previous pattern like you’re doing I think. I’m sure you could just set the 404 one way and the 500 the other way, too though. Again any approach seems fine and I was just responding originally to dissatisfaction in the commit message.

in src/miner.cpp:42 in f5368a525b outdated

38@@ -39,13 +39,21 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
39     return nNewTime - nOldTime;
40 }
41 
42-void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block)
43+void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)

ryanofsky commented at 0:46 am on May 7, 2021:

In commit “miner: Pass in chainman to RegenerateCommitments” (f5368a525b3d5b93d06c83101497de5aaa6fe1c8)

It seems like this could take blockman reference instead of a chainman reference to be a little more limited. Doesn’t matter though, all version of this function seem about the same to me.

dongcarl commented at 6:57 pm on May 7, 2021:

Unfortunately if we take a blockman reference, then callers would need to take cs_main to reach into ChainMan for the blockman ref :-/

Something for another day I think! Added to: #21766

in src/index/base.cpp:68 in 0945b4117e outdated

62@@ -63,30 +63,31 @@ bool BaseIndex::Init()
63     if (locator.IsNull()) {
64         m_best_block_index = nullptr;
65     } else {
66-        m_best_block_index = g_chainman.m_blockman.FindForkInGlobalIndex(::ChainActive(), locator);
67+        m_best_block_index = m_chainstate->m_blockman.FindForkInGlobalIndex(m_chainstate->m_chain, locator);
68     }
69-    m_synced = m_best_block_index.load() == ::ChainActive().Tip();
70+    CChain& active_chain = m_chainstate->m_chain;

ryanofsky commented at 0:50 am on May 7, 2021:

In commit “index: Add chainstate member to BaseIndex” (0945b4117e0209d156a6b74c407f4d32e0cf2c1a)

Seems like active_chain could be used above too. Fine either way in case this was intentional, though.

ryanofsky approved

ryanofsky commented at 0:55 am on May 7, 2021: member

Code review ACK 0945b4117e0209d156a6b74c407f4d32e0cf2c1a. Looks good. I like cleaning up the index code. Straightforward review, too.

dongcarl force-pushed on May 7, 2021

dongcarl commented at 6:52 pm on May 7, 2021: member

Pushed 0945b4117e -> b04370dbb5

Addressed: #21767 (review), #21767 (review), #21767 (review)

ryanofsky approved

ryanofsky commented at 8:31 pm on May 7, 2021: member

Code review ACK b04370dbb55e61e168cbf0aba3d29fb882a46bec. Just http error code change and new commit tweaking variable usage since previous review

in src/rest.cpp:125 in b04370dbb5 outdated

120+    if (!node_context || !node_context->chainman) {
121+        RESTERR(req, HTTP_INTERNAL_SERVER_ERROR,
122+                strprintf("%s:%d (%s)\n"
123+                          "Internal bug detected: Chainman disabled or instance not found!\n"
124+                          "You may report this issue here: %s\n",
125+                          __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT));

jnewbery commented at 3:24 pm on May 9, 2021:

I don’t think this is necessarily a bug. Once chainman is deglobalized, it should be theoretically possible to run without a chainman. I think just having a message “Chainman disabled or instance not found” (similar to if mempool isn’t found) is sufficient.

MarcoFalke commented at 6:30 am on May 12, 2021:

What is the point of running a bitcoin full node without a blockchain?

jnewbery commented at 8:01 am on May 12, 2021:

Testing mostly, but you could also imagine using Bitcoin Core as an addr relayer without a blockchain (eg http://www.erisian.com.au/bitcoin-core-dev/log-2020-12-16.html#l-583), or serving compact block indexes.

dongcarl commented at 8:30 pm on May 19, 2021:

I’ve been making changes with the assumption that chainman is considered absolutely necessary. If we decide that running bitcoin without a chainman is a usecase we want to support, I think we can have a separate discussion about that. Added to: #21766

in src/miner.h:206 in b04370dbb5 outdated

202@@ -203,6 +203,6 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned
203 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
204 
205 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
206-void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block);
207+void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);

jnewbery commented at 3:37 pm on May 9, 2021:

Perhaps just pass in a BlockManager& here?

MarcoFalke commented at 6:30 am on May 12, 2021:

This is the 4th time the same function gets changed around. I think we should decide what to settle on, and ideally provide a rationale for it.

#21391 (review)

jnewbery commented at 8:20 am on May 12, 2021:

I think fundamentally the issue here is with generateblock(), which calls into miner.cpp to construct the block, then changes the transactions in that block, and then calls back into miner.cpp to fix up the coinbase commitment. It’d be a much simpler interface if generateblock() just passed in a const ref to the vector of transactions it wants to include in its first call to miner.cpp.

I’m going to mark this comment as resolved. The mining code is a bit of a mess, and can be cleaned up in other PRs.

in src/bench/duplicate_inputs.cpp:28 in b04370dbb5 outdated

24@@ -25,7 +25,8 @@ static void DuplicateInputs(benchmark::Bench& bench)
25     CMutableTransaction naughtyTx{};
26 
27     LOCK(cs_main);
28-    CBlockIndex* pindexPrev = ::ChainActive().Tip();
29+    assert(std::addressof(::ChainActive()) == std::addressof(testing_setup->m_node.chainman->ActiveChain()));

jnewbery commented at 3:38 pm on May 9, 2021:

Does this need to be in the cs_main scope?

dongcarl commented at 8:33 pm on May 19, 2021:

I don’t think it necessarily has to be (at least not before more of AssumeUTXO is merged), in any case Bundle 7 will remove all of these assertions!

in src/bench/duplicate_inputs.cpp:29 in b04370dbb5 outdated

24@@ -25,7 +25,8 @@ static void DuplicateInputs(benchmark::Bench& bench)
25     CMutableTransaction naughtyTx{};
26 
27     LOCK(cs_main);
28-    CBlockIndex* pindexPrev = ::ChainActive().Tip();
29+    assert(std::addressof(::ChainActive()) == std::addressof(testing_setup->m_node.chainman->ActiveChain()));
30+    CBlockIndex* pindexPrev = testing_setup->m_node.chainman->ActiveChain().Tip();

jnewbery commented at 3:38 pm on May 9, 2021:

0    CBlockIndex* pindexPrev = testing_setup->m_node.chainman->ActiveTip();

in src/index/base.cpp:63 in b04370dbb5 outdated

59@@ -60,33 +60,34 @@ bool BaseIndex::Init()
60     }
61 
62     LOCK(cs_main);
63+    CChain& active_chain = m_chainstate->m_chain;

jnewbery commented at 3:42 pm on May 9, 2021:

I really don’t like CChain becoming part of chainstate’s public interface. It’s too low level - code outside validation shouldn’t be interacting directly with that object and should be using higher level interface functions.

dongcarl commented at 8:26 pm on May 19, 2021:

Do you mean the m_chain member specifically or any kind of CChain? It seems that we were already using a CChain previously when we were caling ::ChainActive(). Happy to document and add to #21766 so nothing gets lost!

in src/index/base.h:122 in b04370dbb5 outdated

118@@ -117,7 +119,7 @@ class BaseIndex : public CValidationInterface
119 
120     /// Start initializes the sync state and registers the instance as a
121     /// ValidationInterface so that it stays in sync with blockchain updates.
122-    void Start();
123+    void Start(CChainState& active_chainstate);

jnewbery commented at 3:47 pm on May 9, 2021:

Any reason that you decided to put this in the Start() method rather than the constructor. You have access to the ::ChainstateActive() object at the point you construct the index objects.

Adding this parameter to the ctor would allow you to set m_chainstate in the initializer list and make it a const member.

dongcarl commented at 8:23 pm on May 19, 2021:

I believe I did this mostly because ctors are not inherited, whereas methods are. If I were to make m_chainstate be a const member and initialized in the ctor, I’d have to change the ctor of every subclass.

DrahtBot added the label Needs rebase on May 12, 2021

jamesob commented at 5:06 pm on May 12, 2021: member

Concept ACK, full review soon

rpc/blockchain: Use existing blockman in gettxoutsetinfo

Was missed in last bundle

fc1c282845

dongcarl force-pushed on May 19, 2021

dongcarl commented at 8:45 pm on May 19, 2021: member

Pushed b04370dbb55e61e168cbf0aba3d29fb882a46bec -> 6ef9163cc4604177fbdab5b644f7b2af7a33204b

Rebased over master

DrahtBot removed the label Needs rebase on May 19, 2021

in src/rest.cpp:117 in c834888358 outdated

112+ *
113+ * @param[in]  req The HTTP request, whose status code will be set if node
114+ *                 context chainstatemanager is not found.
115+ * @returns        Pointer to the chainstatemanager or nullptr if none found.
116+ */
117+static ChainstateManager* GetChainman(const std::any& context, HTTPRequest* req)

ariard commented at 7:22 pm on May 21, 2021:

In “rest: Add GetChainman function and use it”

Do you have pointers to inform reviewers about EnsureAnyChainman causing crashes in REST contexts ?

fjahr commented at 8:46 pm on May 23, 2021:

nit: could have used std::optional for prettiness 💄

dongcarl commented at 6:06 pm on May 27, 2021:

Would be happy to add to PR if you supply a simple enough diff!

dongcarl commented at 6:08 pm on May 27, 2021:

Added in commit message

fjahr commented at 9:25 pm on May 30, 2021:

Something like this maybe, if have to retouch: https://github.com/fjahr/bitcoin/commit/69187f82b1227cc33bc5733c742b8f8b9fcefea3

MarcoFalke commented at 10:24 am on June 1, 2021:

This creates a copy of chainman, no?

jnewbery commented at 12:28 pm on June 4, 2021:

@fjahr I think the branch is invalid because you’re trying to move from the global g_chainman.

In general, the usage of algebraic sum types like std::optional or std::variant isn’t for prettiness - it’s to entirely remove the possibility of being in an invalid program state.

fjahr commented at 9:12 am on June 6, 2021:

Ok, never mind, I guess since there is no optional reference in std this wasn’t a good idea after all.

in src/index/base.h:80 in f9222a0cb5 outdated

75@@ -75,8 +76,9 @@ class BaseIndex : public CValidationInterface
76     /// to a chain reorganization), the index must halt until Commit succeeds or else it could end up
77     /// getting corrupted.
78     bool Commit();
79-
80 protected:
81+    CChainState* m_chainstate{nullptr};

ariard commented at 8:06 pm on May 21, 2021:

nit: Add comment “Provide access to the active chainstate, containing the current most-work chain”. Maybe even as a member name m_active_chainstate

dongcarl commented at 6:08 pm on May 27, 2021:

Not sure m_active_chainstate makes too much sense here, as in the index context, this member means “the active chainstate at the time when <Index>::Start was called”

ariard commented at 6:08 pm on May 28, 2021:

Sure, it’s just with assumeutxo, we start to have multiple chainstate references through the codebase (m_ibd_chainstate, m_snapshot_chainstate, m_m_active_chainstate) and it could be confusing in the future. I don’t think it changes anything for the indexes rn.

jamesob commented at 8:06 pm on May 28, 2021:

To @ariard’s point, we will likely have to facilitate changing this pointer mid-execution when the user calls ActivateSnapshot(). I’ll have to warm my caches on the indexing stuff a little more, but IIRC we want this class instance to have a reference to the current active chainstate (which obviously will be liable to change as assumeutxo progresses).

jamesob commented at 8:08 pm on May 28, 2021:

That said, the reduction of global use is an obvious win, and it should be pretty easy to facilitate changing m_chainstate via some method that can be called by ActivateSnapshot().

jamesob commented at 8:17 pm on May 28, 2021:

Might even just make sense to pass in a ChainstateManager instance to keep with this class… but that can be done in a follow-up.

ariard commented at 8:19 pm on May 21, 2021: member

utACK f9222a0c

fjahr commented at 9:20 pm on May 23, 2021: member

light code review ACK 6ef9163cc4604177fbdab5b644f7b2af7a33204b

I did a thorough review of the changes but did not review all the previous PRs in this series so I could be missing something due to a lack of context, that’s why only “light”.

rest: Add GetChainman function and use it

This is not the cleanest change but:

1. It fixes the erroneous use of RPC's Ensure*() in rest.cpp, which
   cause crashes in REST contexts.

   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.

2. It is consistent with other functions like GetMemPool.

Someone can probably make this a bit prettier.

9ecade1425

miner: Pass in chainman to RegenerateCommitments

Pass in chainman instead of prev_block so that we can enforce the
block.hashPrevBlock refers to prev_block invariant in the function
itself.

We should probably rethink BlockAssembler's API and somehow include
commitment regeneration functionality in there. Something like a variant
of CreateNewBlock that takes in a std::vector<TxRef> and return a CBlock
instead of CBlockTemplate. That could avoid reaching for
LookupBlockIndex at all.

e6b4aa6eb5

bench: Use existing NodeContext in DuplicateInputs 91226eb917

bench: Use existing chainman in AssembleBlock f4a47a1feb

index: Add chainstate member to BaseIndex db33cde80f

index: refactor-only: Reuse CChain ref 7a799c9c2b

dongcarl force-pushed on May 27, 2021

dongcarl commented at 6:07 pm on May 27, 2021: member

Pushed 6ef9163cc4604177fbdab5b644f7b2af7a33204b -> 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a

Added REST crash explanation to commit message per: #21767 (review)

ryanofsky approved

ryanofsky commented at 9:04 pm on May 27, 2021: member

Code review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()

jarolrod commented at 1:59 am on May 28, 2021: member

ACK 7a799c9

Easy to follow what’s going on, the assert statements and tests also give assurance to there being no change in behavior. Nice to see how this gets wrapped up on the next bundle.

ariard commented at 6:09 pm on May 28, 2021: member

Code Review ACK 7a799c9

in src/miner.cpp:55 in e6b4aa6eb5 outdated

52+        // TODO: Temporary scope to check correctness of refactored code.
53+        // Should be removed manually after merge of
54+        // https://github.com/bitcoin/bitcoin/pull/20158
55+        LOCK(::cs_main);
56+        assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman));
57+        prev_block = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock);

jamesob commented at 7:34 pm on May 28, 2021:

Hm, shouldn’t we be asserting here (instead of assigning) to preserve behavior? Edit: not sure what practical difference that would have if any.

dongcarl commented at 10:29 pm on June 2, 2021:

So ::RegenerateCommitments has been changed a few times now:

In 2c3ba00693, where we passed in a BlockManager, instead of reaching for g_chainman.m_blockman
- Because we don’t want to reach for globals anymore (the overall point of this PR series)
In cced0f46c9, where we passed in the specific CBlockIndex instead of a BlockManager
- Because it is a more specific validation-related object than BlockManager
In e6b4aa6eb5, where we pass in a ChainstateManger instead of the specific CBlockIndex
- Because then we don’t have to worry about callers violating the invariant checked here: https://github.com/bitcoin/bitcoin/blob/cced0f46c9133e0fc6211e987421ad1d9be1a399/src/miner.cpp#L48

Just to be clear, the original behaviour does not do any checking/assertions: https://github.com/bitcoin/bitcoin/blob/2afcf24408b4453e4418ebfb326b141f6ea8647c/src/miner.cpp#L42-L51

So overall, I don’t think an behaviour has changed, and I think we should just go with the current one since the few versions we’ve tried are all functionally equivalent. Thanks for the careful review tho! :-)

jamesob commented at 8:26 pm on May 28, 2021: member

conditional ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a (jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai)

Only hung up on the question about change in miner.cpp behavior. Everything else is informational. Good change!

fc1c28284 rpc/blockchain: Use existing blockman in gettxoutsetinfo

Oneline fix.
9ecade142 rest: Add GetChainman function and use it

I don’t know the REST code well but all changes seem consistent with existing conventions.
e6b4aa6eb miner: Pass in chainman to RegenerateCommitments

Question about assertion -> assignment change.
91226eb91 bench: Use existing NodeContext in DuplicateInputs

ActiveTip() nit as already noted by John.
f4a47a1fe bench: Use existing chainman in AssembleBlock
db33cde80 index: Add chainstate member to BaseIndex

See questions around active chainstate management in the context of ActivateSnapshot() use. The indexer probably needs to be made aware of the new active chainstate, or it should just hold a reference to chainman and not have to worry about being notified.
7a799c9c2 index: refactor-only: Reuse CChain ref

Could probably be fixed-up into the previous commit.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))
 4
 5-----BEGIN PGP SIGNATURE-----
 6
 7iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmCxUQAACgkQepNdrbLE
 8TwViSw/+IpN1mVXXNgnNcNJ4dv+u9QjAqslyD7xq5IpJrDSI7dPIU8XF1ypOlV2I
 9AiUd4sOheAF/fkj+61f8uRvWO55DdFPhuwarAxwUna4YvL8E4ui1uplavDGrsgMa
10mhZzx3PoDBm2RotKXF/zPdsbZ18mXeGPEbePiH1+r1+wbIYp/7Mj4z5Snp+OK3EE
11tuWVf4efTYvymL1ZosI/4gIcc8FbpcxCMHVD5RJaN/pH5KickFzs7oDiaBNuzIB1
12lgC837X5a4cjEN68LBXVGBVax2RkS+N09p162+6dK8mlcc9ob6JtBZxcW4B3YAwP
13k8AIbNOqpK1SI5ayvaz/GLpHbdwup1tjZxabhzYIJYYx3feslcg2h45H444xBqn3
1401YrnJeG5LxjSKEdYAjYNagE7zsRY2kykhMEodBj8kAU+whtmM5SIYAP40rHHe78
154vGqo0hN4ekvdoA+sE4paqccNJkHDlN0ben5l2phS3FVCG4fBxM2nZ3OduOklAWH
16l8kZQSO2b2IwDn/2a7iTBVk+2oTZ9rO4MK1Qk3cBumSAPQQvF0O9ZckYcIO6agvU
17XM0xghbsEpbT2UexxEoDWu2u0DbQYXu4fOBnX+s8XDEjowsoPFVylcKrz/6YVkrP
18jpmZEoKL6VAfmAv7BlvOocjpJq/sNZgIQtV63a3Ekikew2K9Nvc=
19=BpU9
20-----END PGP SIGNATURE-----

0Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28
1
2Configured 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 --enable-fuzz --with-sanitizers=address,fuzzer,undefined CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
3
4Compiled 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 -fsanitize=address,fuzzer,undefined -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: Debian clang version 11.1.0-++20210405104510+1fdec59bffc1-1~exp1~20210405085125.161

fjahr commented at 9:27 pm on May 30, 2021: member

re-ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a

MarcoFalke commented at 11:18 am on June 1, 2021: member

review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a 🌠

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a 🌠
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgnkAv/eYSF8/6fmyM3n1fjSVmbmXT74brAbRsg0tMEuWb0rtk603x8E8QSAAot
 8PCSICg/oAsOXVIzdxOWNYpqhEiceILJNlQTjuNfswOu0cgJNNqCp/sYX3nWrlmaW
 99dqsK0zMMLjnC/U2xzSzvvU6id++ipSQlyWkbAjt+n7UorVd2A8wCdQMrMCQejpz
10pFB1K25WAH7KYiDIwLxy1ZWO/x094gQaVSk6DfIu+KhUSQTL03QunHCFjN3Fyoey
118drI7oGaL0GtA6mcZHjApXhJG4ts7kYI39WJ8zD9wIHotQ1e9earLar9L28WwCRE
12fhdCBtjHGSTf1V6WKOZFkvYVbMyxQhyUVqdSwsZzlcNB7szkUJfmLZTMNSfKKYBS
13MPuNUM5MCB9/atp2+YVEvCw3TdaXHxNJ1JDLvuFMAMOvCD5BuGrZklYKW2FhP4Ar
14MsMO6AoCfzHJUVvdWlsxWYneMlN37Eg428rZOw9lUH9rdWavMMskwpcUe8MaYY5w
15e3hP/g/H
16=P/4c
17-----END PGP SIGNATURE-----

Timestamp of file with hash e8caa0bbe435d430361ec68dd841345069e815f61d2cf90bf463dd41e3290516 -

MarcoFalke merged this on Jun 1, 2021

MarcoFalke closed this on Jun 1, 2021

sidhujag referenced this in commit 8934617ada on Jun 1, 2021

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

fanquake referenced this in commit a55904a80c on Jun 12, 2021

gwillen referenced this in commit b654bb7f5e on Jun 1, 2022

Fabcien referenced this in commit bc55fd397a on Jun 20, 2022

Fabcien referenced this in commit eb17f26abb on Jun 20, 2022

Fabcien referenced this in commit cbaf5494b4 on Jun 21, 2022

DrahtBot locked this on Aug 18, 2022

[Bundle 6/n] Prune g_chainman usage in auxiliary modules #21767

Conflicts