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

dongcarl · 2021-04-23T20:13:09Z

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 followups: Various potential improvements arising out of chainman-deglobalizing review #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 · 2021-04-24T01:14:19Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Improve Indices on pruned nodes via prune blockers #21726 (Improve Indices on pruned nodes via prune blockers by fjahr)
validation: UpdateTip/CheckBlockIndex assumeutxo support #21526 (validation: UpdateTip/CheckBlockIndex assumeutxo support by jamesob)
assumeutxo #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.

laanwj · 2021-05-05T12:09:13Z

Looks like this accumulated a silent merge conflict:

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

ryanofsky

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

src/rest.cpp

ryanofsky · 2021-05-07T00:33:15Z

src/rest.cpp

@@ -644,7 +667,9 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,

    CBlockIndex* pblockindex = nullptr;
    {
-        ChainstateManager& chainman = EnsureAnyChainman(context);
+        ChainstateManager* maybe_chainman = GetChainman(context, req);


In commit "rest: Add GetChainman function and use it" (a839078)

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:

bitcoin/src/rest.cpp

Line 699 in 06d573f

auto handler = [context, up](HTTPRequest* req, const std::string& prefix) { return up.handler(context, req, prefix); };

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

re: #21767 (comment)

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:

class PageNotFoundError : public std::runtime_error { using std::runtime_error::runtime_error; }; ... throw PageNotFoundError{}; ... } catch (const PageNotFoundError&) {

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.

ryanofsky · 2021-05-07T00:46:16Z

src/miner.cpp

@@ -39,13 +39,21 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
    return nNewTime - nOldTime;
 }

-void RegenerateCommitments(CBlock& block, CBlockIndex* prev_block)
+void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)


In commit "miner: Pass in chainman to RegenerateCommitments" (f5368a5)

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.

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

src/index/base.cpp

dongcarl · 2021-05-07T18:52:24Z

Pushed 0945b41 -> b04370d

Addressed: [Bundle 6/n] Prune g_chainman usage in auxiliary modules #21767 (comment), [Bundle 6/n] Prune g_chainman usage in auxiliary modules #21767 (comment), [Bundle 6/n] Prune g_chainman usage in auxiliary modules #21767 (comment)

ryanofsky

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

jnewbery · 2021-05-09T15:24:37Z

src/rest.cpp

+                          "Internal bug detected: Chainman disabled or instance not found!\n"
+                          "You may report this issue here: %s\n",
+                          __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT));


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.

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

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.

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

src/miner.h

jnewbery · 2021-05-09T15:38:19Z

src/bench/duplicate_inputs.cpp

@@ -25,7 +25,8 @@ static void DuplicateInputs(benchmark::Bench& bench)
    CMutableTransaction naughtyTx{};

    LOCK(cs_main);
-    CBlockIndex* pindexPrev = ::ChainActive().Tip();
+    assert(std::addressof(::ChainActive()) == std::addressof(testing_setup->m_node.chainman->ActiveChain()));


Does this need to be in the cs_main scope?

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!

jnewbery · 2021-05-09T15:38:28Z

src/bench/duplicate_inputs.cpp

@@ -25,7 +25,8 @@ static void DuplicateInputs(benchmark::Bench& bench)
    CMutableTransaction naughtyTx{};

    LOCK(cs_main);
-    CBlockIndex* pindexPrev = ::ChainActive().Tip();
+    assert(std::addressof(::ChainActive()) == std::addressof(testing_setup->m_node.chainman->ActiveChain()));
+    CBlockIndex* pindexPrev = testing_setup->m_node.chainman->ActiveChain().Tip();


Suggested change

CBlockIndex* pindexPrev = testing_setup->m_node.chainman->ActiveChain().Tip();

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

jnewbery · 2021-05-09T15:42:56Z

src/index/base.cpp

@@ -60,33 +60,34 @@ bool BaseIndex::Init()
    }

    LOCK(cs_main);
+    CChain& active_chain = m_chainstate->m_chain;


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.

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!

jnewbery · 2021-05-09T15:47:37Z

src/index/base.h

@@ -117,7 +119,7 @@ class BaseIndex : public CValidationInterface

    /// Start initializes the sync state and registers the instance as a
    /// ValidationInterface so that it stays in sync with blockchain updates.
-    void Start();
+    void Start(CChainState& active_chainstate);


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.

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.

jamesob · 2021-05-12T17:06:07Z

Concept ACK, full review soon

Was missed in last bundle

dongcarl · 2021-05-19T20:45:13Z

Pushed b04370d -> 6ef9163

Rebased over master

ariard

utACK f9222a0

src/rest.cpp

ariard · 2021-05-21T20:06:24Z

src/index/base.h

 protected:
+    CChainState* m_chainstate{nullptr};


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

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"

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.

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

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

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.

fjahr

light code review ACK 6ef9163

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

fjahr · 2021-05-23T20:46:09Z

src/rest.cpp

+ *                 context chainstatemanager is not found.
+ * @returns        Pointer to the chainstatemanager or nullptr if none found.
+ */
+static ChainstateManager* GetChainman(const std::any& context, HTTPRequest* req)


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

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

Something like this maybe, if have to retouch: fjahr@69187f8

This creates a copy of chainman, no?

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

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

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.

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.

dongcarl · 2021-05-27T18:07:51Z

Pushed 6ef9163 -> 7a799c9

Added REST crash explanation to commit message per: [Bundle 6/n] Prune g_chainman usage in auxiliary modules #21767 (comment)

ryanofsky

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

jarolrod · 2021-05-28T01:59:03Z

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 · 2021-05-28T18:09:07Z

Code Review ACK 7a799c9

jamesob · 2021-05-28T19:34:31Z

src/miner.cpp

+        // https://github.com/bitcoin/bitcoin/pull/20158
+        LOCK(::cs_main);
+        assert(std::addressof(g_chainman.m_blockman) == std::addressof(chainman.m_blockman));
+        prev_block = chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock);


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

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

In 2c3ba00, 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 cced0f4, where we passed in the specific CBlockIndex instead of a BlockManager

Because it is a more specific validation-related object than BlockManager

In e6b4aa6, 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:

bitcoin/src/miner.cpp

Line 48 in cced0f4

WITH_LOCK(::cs_main, assert(g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock) == prev_block));

Just to be clear, the original behaviour does not do any checking/assertions:

bitcoin/src/miner.cpp

Lines 42 to 51 in 2afcf24

void RegenerateCommitments(CBlock& block)

{

CMutableTransaction tx{*block.vtx.at(0)};

tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));

block.vtx.at(0) = MakeTransactionRef(tx);

GenerateCoinbaseCommitment(block, WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());

block.hashMerkleRoot = BlockMerkleRoot(block);

}

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

conditional ACK 7a799c9 (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!

fc1c282 rpc/blockchain: Use existing blockman in gettxoutsetinfo

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

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

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

ActiveTip() nit as already noted by John.
f4a47a1 bench: Use existing chainman in AssembleBlock
db33cde 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.
7a799c9 index: refactor-only: Reuse CChain ref

Could probably be fixed-up into the previous commit.

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmCxUQAACgkQepNdrbLE
TwViSw/+IpN1mVXXNgnNcNJ4dv+u9QjAqslyD7xq5IpJrDSI7dPIU8XF1ypOlV2I
AiUd4sOheAF/fkj+61f8uRvWO55DdFPhuwarAxwUna4YvL8E4ui1uplavDGrsgMa
mhZzx3PoDBm2RotKXF/zPdsbZ18mXeGPEbePiH1+r1+wbIYp/7Mj4z5Snp+OK3EE
tuWVf4efTYvymL1ZosI/4gIcc8FbpcxCMHVD5RJaN/pH5KickFzs7oDiaBNuzIB1
lgC837X5a4cjEN68LBXVGBVax2RkS+N09p162+6dK8mlcc9ob6JtBZxcW4B3YAwP
k8AIbNOqpK1SI5ayvaz/GLpHbdwup1tjZxabhzYIJYYx3feslcg2h45H444xBqn3
01YrnJeG5LxjSKEdYAjYNagE7zsRY2kykhMEodBj8kAU+whtmM5SIYAP40rHHe78
4vGqo0hN4ekvdoA+sE4paqccNJkHDlN0ben5l2phS3FVCG4fBxM2nZ3OduOklAWH
l8kZQSO2b2IwDn/2a7iTBVk+2oTZ9rO4MK1Qk3cBumSAPQQvF0O9ZckYcIO6agvU
XM0xghbsEpbT2UexxEoDWu2u0DbQYXu4fOBnX+s8XDEjowsoPFVylcKrz/6YVkrP
jpmZEoKL6VAfmAv7BlvOocjpJq/sNZgIQtV63a3Ekikew2K9Nvc=
=BpU9
-----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 --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

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 -fsanitize=address,fuzzer,undefined -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: Debian clang version 11.1.0-++20210405104510+1fdec59bffc1-1~exp1~20210405085125.161

fjahr · 2021-05-30T21:27:40Z

re-ACK 7a799c9

maflcko · 2021-06-01T11:18:42Z

review ACK 7a799c9 🌠

Show signature and timestamp

Signature:

Timestamp of file with hash e8caa0bbe435d430361ec68dd841345069e815f61d2cf90bf463dd41e3290516 -

… modules

6f99488 validation: Farewell, global Chainstate! (Carl Dong) 972c516 qt/test: Reset chainman in ~ChainstateManager instead (Carl Dong) 6c3b5dc scripted-diff: tree-wide: Remove all review-only assertions (Carl Dong) 3e82abb tree-wide: Remove stray review-only assertion (Carl Dong) f323248 qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) (Carl Dong) 6c15de1 scripted-diff: wallet/test: Use existing chainman (Carl Dong) ee0ab1e fuzz: Initialize a TestingSetup for test_one_input (Carl Dong) 0d61634 scripted-diff: test: Use existing chainman in unit tests (Carl Dong) e197076 test: Pass in CoinsTip to ValidateCheckInputsForAllFlags (Carl Dong) 4d99b61 test/miner_tests: Pass in chain tip to CreateBlockIndex (Carl Dong) f0dd5e6 test/util: Use existing chainman in ::PrepareBlock (Carl Dong) 464c313 init: Use existing chainman (Carl Dong) Pull request description: Based on: #21767 à la Mr. Sandman ``` Mr. Chainman, bring me a tip (bung, bung, bung, bung) Make it the most work that I've ever seen (bung, bung, bung, bung) Rewind old tip till we're at the fork point (bung, bung, bung, bung) Then tell it that it's time to call Con-nectTip Chainman, I'm so alone (bung, bung, bung, bung) No local objects to call my own (bung, bung, bung, bung) Please make sure I have a ref Mr. Chainman, bring me a tip! ``` 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. - Remove globals: - `ChainstateManager g_chainman` - `CChainState& ChainstateActive()` - `CChain& ChainActive()` - Remove all review-only assertions. ACKs for top commit: jamesob: reACK 6f99488 based on the contents of ariard: Code Review ACK 6f99488. jnewbery: utACK 6f99488 achow101: Code Review ACK 6f99488 ryanofsky: Code review ACK 6f99488. Tree-SHA512: 4052ea79360cf0efd81ad0ee3f982e1d93aab1837dcec75f875a56ceda085de078bb3099a2137935d7cc2222004ad88da94b605ef5efef35cb6bc733725debe6

Summary: This is a backport of [[bitcoin/bitcoin#21767 | core#21767]] [1/5] bitcoin/bitcoin@fc1c282 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D11632

Summary: This is a backport of [[bitcoin/bitcoin#21767 | core#21767]] [2 & 3 / 5] bitcoin/bitcoin@91226eb bitcoin/bitcoin@f4a47a1 Test Plan: `ninja bench-bitcoin` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D11633

Summary: This concludes backport of [[bitcoin/bitcoin#21767 | core#21767]] [4 & 5/5] bitcoin/bitcoin@db33cde bitcoin/bitcoin@7a799c9 Note: the second commit is a trivial improvement of the first one. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11642

DrahtBot added Mining RPC/REST/ZMQ UTXO Db and Indexes labels Apr 23, 2021

This was referenced Apr 24, 2021

Improve Indices on pruned nodes via prune blockers #21726

Merged

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

Merged

maflcko added the Refactoring label Apr 24, 2021

DrahtBot mentioned this pull request Apr 28, 2021

index: Avoid async shutdown on init error #21796

Merged

laanwj removed Mining RPC/REST/ZMQ UTXO Db and Indexes labels May 5, 2021

dongcarl force-pushed the 2020-10-libbitcoinruntime-v8 branch 2 times, most recently from 4e5773e to 0945b41 Compare May 5, 2021 18:17

dongcarl mentioned this pull request May 5, 2021

[Bundle 7/7] validation: Farewell, global Chainstate! #21866

Merged

ryanofsky approved these changes May 7, 2021

View reviewed changes

dongcarl force-pushed the 2020-10-libbitcoinruntime-v8 branch from 93d5efe to b04370d Compare May 7, 2021 18:51

ryanofsky approved these changes May 7, 2021

View reviewed changes

jnewbery mentioned this pull request May 9, 2021

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

jnewbery reviewed May 9, 2021

View reviewed changes

DrahtBot added the Needs rebase label May 12, 2021

dongcarl mentioned this pull request May 19, 2021

followups: Various potential improvements arising out of chainman-deglobalizing review #21766

Closed

rpc/blockchain: Use existing blockman in gettxoutsetinfo

fc1c282

Was missed in last bundle

dongcarl force-pushed the 2020-10-libbitcoinruntime-v8 branch from b04370d to 6ef9163 Compare May 19, 2021 20:44

DrahtBot removed the Needs rebase label May 19, 2021

ariard reviewed May 21, 2021

View reviewed changes

fjahr reviewed May 23, 2021

View reviewed changes

dongcarl added 6 commits May 27, 2021 13:49

bench: Use existing NodeContext in DuplicateInputs

91226eb

bench: Use existing chainman in AssembleBlock

f4a47a1

index: Add chainstate member to BaseIndex

db33cde

index: refactor-only: Reuse CChain ref

7a799c9

dongcarl force-pushed the 2020-10-libbitcoinruntime-v8 branch from 6ef9163 to 7a799c9 Compare May 27, 2021 18:06

ryanofsky approved these changes May 27, 2021

View reviewed changes

DrahtBot mentioned this pull request May 28, 2021

assumeutxo #15606

Closed

18 tasks

jamesob reviewed May 28, 2021

View reviewed changes

maflcko merged commit f63fc53 into bitcoin:master Jun 1, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 1, 2021

Merge bitcoin#21767: [Bundle 6/n] Prune g_chainman usage in auxiliary…

8934617

… modules

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge f63fc53 into merged_master (Bitcoin PR bitcoin/bitcoin#21767)

b654bb7

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

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

	void RegenerateCommitments(CBlock& block)
	{
	CMutableTransaction tx{*block.vtx.at(0)};
	tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));
	block.vtx.at(0) = MakeTransactionRef(tx);

	GenerateCoinbaseCommitment(block, WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock)), Params().GetConsensus());

	block.hashMerkleRoot = BlockMerkleRoot(block);
	}

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

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

Uh oh!

Conversation

dongcarl commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented May 5, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongcarl commented May 7, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented May 12, 2021

Uh oh!

dongcarl commented May 19, 2021

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongcarl May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dongcarl commented Apr 23, 2021 •

edited

Loading

DrahtBot commented Apr 24, 2021 •

edited

Loading

ryanofsky May 7, 2021 •

edited

Loading

dongcarl May 27, 2021 •

edited

Loading

jamesob May 28, 2021 •

edited

Loading

dongcarl Jun 2, 2021 •

edited

Loading