refactor: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate) #6078

kwvg · 2024-06-25T10:08:31Z

Thank you, I'll say goodbye soon
Though its the end of these globals, don't blame yourself now
And if its true, I will surround you and give life to a chainstate
That's our own

Additional Information

In CDeterministicMN::ToJson(), collateralAddress is extracted by finding the scriptPubKey of a transaction output for a masternode, originally this used GetUTXOCoin but doesn't work for spent tranasction outputs (as they're not UTXOs), so in dash#5607, a fallback was introduced that looks through the general transaction set if going through the UTXO set yielded nothing.

GetUTXOCoin accesses the active chainstate to get ahold of the UTXO set, this was done through globals. The removal of chainstate globals meant that whoever was calling GetUTXOCoin should have access to the chainstate handy. This is trivial in RPC code where ToJson() is used (source) through Ensure(Any)Chainman. Not the case in Qt code (source), which is supposed to be given restricted access to information by the interface.

As the fallback seems to be capable of fetching UTXOs and spent outputs, we can remove the GetUTXOCoin method and make the fallback the only method.
In develop, as of this writing, CChainState members FlushStateToDisk and {Enforce, Invalidate, MarkConflicting}Block were accessing their internals through the global, despite having direct access to them. As the globals they were calling are going to be bid farewell, they needed to be changed to access its members instead.

The reason for going the roundabout way is unknown.
CDSNotificationInterface takes in a ChainstateManager (instead of the CChainState it actually requires) as at the time of interface initialization (source), the active chainstate hasn't been loaded in yet as that happens further down (source).

As CDSNotificationInterface::InitializeCurrentBlockTip() is called well after it is initialized, we can resolve to the active chainstate in there.
As GetCreditPoolDiffForBlock requires access to ChainstateManager as GetCreditPoolDiffForBlock > ProcessLockUnlockTransaction > CheckAssetLockUnlockTx > CheckAssetUnlockTx > ChainstateManager::m_blockman.LookupBlockIndex() and BlockAssembler only has CChainState, it had to be reworked around ChainstateManager.

CChainState is passed as a direct argument while ChainstateManager can be fetched from NodeContext. Unlike CTxMemPool, which can be passed custom instances (source, source), CChainState's argument value is taken from NodeContext::chainstate.ActiveChainstate() and since we're now accepting ChainstateManager wholesale, we can dispense with accepting CChainState as an argument.

~~Changes to that effect have been made.~~

AssumeUTXO introduces the need to be able to use different CChainStates, so this underlying assumption no longer holds true, the above described changes have been reverted. Asset locks code has been refactored to use BlockManager directly (which does come with the downside of needing to hold cs_main for longer than strictly necessary, this is why only asset locks uses BlockManager directly while other cases still benefit from having ChainstateManager as a whole).
CMNHFManager::ConnectManagers will be taking in a ChainstateManager pointer due to the GetSignalsStage > GetForBlock > ProcessBlock > extractSignals > CheckMNHFTx > ChainstateManager::m_blockman.LookupBlockIndex() chain.

The use of a bespoke NodeContext in coinselector_tests breaks tests if any interface call relies on a chainstate as testNode doesn't initialize one. For the most part, this was masked by WalletTestingSetup populating the chainstate globals from its own NodeContext even if the tests themselves preferred to use their own stripped down testNode.

Though, removing the chainstate globals meant that they can no longer rely on WalletTestingSetup's NodeContext to mask the barebones testNode global being used in the test (specifically, addCoins > listMNCollaterials > ChainActive() worked because ChainActive() accessed WalletTestingSetup's NodeContext but when ChainActive() was gone and replaced with NodeContext::chainman.ActiveChain(), it uses testNode's ChainstateManager, which doesn't exist, which causes it to crash).

To remedy this, a5595b1 and 5e54aa9 from bitcoin#23288 were adapted for the limited purpose of eliminating testNode and using WalletTestingSetup's NodeContext instead. This comes with the unfortunate effect of skipping a lot of the refactoring, cleanups and optimizations done before and adapting the ones after them non-trivial.

It is therefore best recommended that the commit be reverted and changes implemented step-by-step in a pull request at some point in the future. For now, it's kept around here for the sake of this pull request, which, if merged, should prevent more chainstate globals use from leaking into the codebase.

Pre-fix crash stacktrace:

dash@71aecd6afb45:/src/dash$ lldb-16 ./src/test/test_dash
(lldb) target create "./src/test/test_dash"
Current executable set to '/src/dash/src/test/test_dash' (x86_64).
(lldb) r -t coinselector_tests
Process 395006 launched: '/src/dash/src/test/test_dash' (x86_64)
Running 4 test cases...
node/interfaces.cpp:711 chainman: Assertion `m_node.chainman' failed.
Process 395006 stopped
* thread #1, name = 'd-test', stop reason = signal SIGABRT
    frame #0: 0x00007ffff7a7300b libc.so.6`__GI_raise(sig=<unavailable>) at raise.c:51:1
(lldb) bt
* thread #1, name = 'd-test', stop reason = signal SIGABRT
* frame #0: 0x00007ffff7a7300b libc.so.6`__GI_raise(sig=<unavailable>) at raise.c:51:1
    frame #1: 0x00007ffff7a52859 libc.so.6`__GI_abort at abort.c:79:7
    frame #2: 0x00005555563cba33 test_dash`assertion_fail(file="node/interfaces.cpp", line=711, func="chainman", assertion="m_node.chainman") at check.cpp:13:5
    frame #3: 0x0000555555fb47aa test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(std::vector<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>, std::allocator<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>>> const&) [inlined] std::unique_ptr<ChainstateManager, std::default_delete<ChainstateManager>>& inline_assertion_check<true, std::unique_ptr<ChainstateManager, std::default_delete<ChainstateManager>>&>(val=nullptr, file=<unavailable>, line=711, func=<unavailable>, assertion=<unavailable>) at check.h:62:13
    frame #4: 0x0000555555fb4781 test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(std::vector<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>, std::allocator<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>>> const&) [inlined] node::(anonymous namespace)::ChainImpl::chainman(this=0x000055555723e830)at interfaces.cpp:711:45
    frame #5: 0x0000555555fb477d test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(std::vector<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>, std::allocator<std::pair<std::shared_ptr<CTransaction const> const&, unsigned int>>> const&) [inlined] node::(anonymous namespace)::ChainImpl::listMNCollaterials(this=<unavailable>)::'lambda'()::operator()() const at interfaces.cpp:788:34
    frame #6: 0x0000555555fb474f test_dash`node::(anonymous namespace)::ChainImpl::listMNCollaterials(this=0x000055555723e830, outputs=size=0) at interfaces.cpp:788:34
    frame #7: 0x00005555565bcd07 test_dash`CWallet::AddToWallet(this=0x00005555571701e0, tx=<unavailable>, confirm=<unavailable>, update_wtx=<unavailable>, fFlushOnClose=<unavailable>) at wallet.cpp:886:46
    frame #8: 0x0000555555bed3ef test_dash`coinselector_tests::add_coin(wallet=0x00005555571701e0, nValue=0x00007fffffffc7c0, nAge=144, fIsFromMe=false, nInput=0, spendable=<unavailable>) at coinselector_tests.cpp:77:29
    frame #9: 0x0000555555bead3e test_dash`coinselector_tests::bnb_search_test::test_method() [inlined] coinselector_tests::add_coin(nValue=0x00007fffffffc7c0, nAge=144, fIsFromMe=false, nInput=0, spendable=false) at coinselector_tests.cpp:88:5
    frame #10: 0x0000555555bead20 test_dash`coinselector_tests::bnb_search_test::test_method(this=0x00007fffffffcad0) at coinselector_tests.cpp:278:5
    frame #11: 0x0000555555be6607 test_dash`coinselector_tests::bnb_search_test_invoker() at coinselector_tests.cpp:138:1

Breaking Changes

Backporting coinselector_tests changes are now much more annoying.
The following RPCs, protx list, protx listdiff, protx info will no longer report collateralAddress if the transaction index has been disabled (txindex=0).

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas (note: N/A)
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation (note: N/A)
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

github-actions · 2024-06-25T13:46:36Z

This pull request has conflicts, please rebase.

PastaPastaPasta

general utACK; a few problems: 8e32493

PastaPastaPasta · 2024-06-25T17:25:50Z

src/evo/deterministicmns.cpp

+    uint256 tmpHashBlock;
+    CTransactionRef collateralTx = GetTransaction(/* block_index */ nullptr,  /* mempool */ nullptr, collateralOutpoint.hash, Params().GetConsensus(), tmpHashBlock);
+    if (collateralTx) {
+        CTxDestination dest;
+        if (ExtractDestination(collateralTx->vout[collateralOutpoint.n].scriptPubKey, dest)) {
+            obj.pushKV("collateralAddress", EncodeDestination(dest));
+        }


needs release notes documenting breaking change when txindex is disabled

The following RPC calls are affected:

protx info (source)

protx list (source, source)

protx listdiff (source)

Resolved in latest push.

src/governance/classes.h

knst

LGTM overall, but see comments

src/miner.h

knst · 2024-06-25T19:55:47Z

src/spork.cpp

@@ -149,8 +150,8 @@ PeerMsgRet CSporkManager::ProcessSpork(const CNode& peer, PeerManager& peerman,
    {
        LOCK(cs_main);
        EraseObjectRequest(peer.GetId(), CInv(MSG_SPORK, hash));
-        if (!::ChainActive().Tip()) return {};
-        strLogMsg = strprintf("SPORK -- hash: %s id: %d value: %10d bestHeight: %d peer=%d", hash.ToString(), spork.nSporkID, spork.nValue, ::ChainActive().Height(), peer.GetId());
+        if (!active_chain.Tip()) return {};


why do we need this conditition? whatis' if Tip is nullptr ?

Examining that condition is outside the scope of this PR. If that condition can be done away with (in a different PR before or after this one), the only remaining usage would be to print height, which can be trimmed away by omitting bestHeight, which will remove the need for passing active_chain.

EDIT: Udjin has clarified the earlier check was due to earlier usage of ::ChainActive().Tip()->nHeight (source), seems safe to remove. Done in latest push.

Yeah, this was introduced back then when we used chainActive.Tip()->nHeight in logs and was never reviewed/removed when we switched to chainActive.Height() later. Should be safe to remove heigh/active_chain from spork logs imo. Could be done in a separate PR but technically speaking 68e677c wasn't a refactor either so either way is fine imo.

src/spork.h

src/bench/coin_selection.cpp

src/validation.h

UdjinM6 · 2024-06-26T10:23:17Z

src/spork.cpp

@@ -149,8 +150,8 @@ PeerMsgRet CSporkManager::ProcessSpork(const CNode& peer, PeerManager& peerman,
    {
        LOCK(cs_main);
        EraseObjectRequest(peer.GetId(), CInv(MSG_SPORK, hash));
-        if (!::ChainActive().Tip()) return {};
-        strLogMsg = strprintf("SPORK -- hash: %s id: %d value: %10d bestHeight: %d peer=%d", hash.ToString(), spork.nSporkID, spork.nValue, ::ChainActive().Height(), peer.GetId());
+        if (!active_chain.Tip()) return {};


Yeah, this was introduced back then when we used chainActive.Tip()->nHeight in logs and was never reviewed/removed when we switched to chainActive.Height() later. Should be safe to remove heigh/active_chain from spork logs imo. Could be done in a separate PR but technically speaking 68e677c wasn't a refactor either so either way is fine imo.

kwvg · 2024-06-26T11:07:06Z

The latest diff of changes got buried due to a force push to fix a typo, the diff is available here.

The fallback introduced in dash#5607 for spent UTXOs also works for unspent UTXOs, no reason to leave it as fallback when it can be made primary. Also, if we did want to keep GetUTXOCoin around, it would need access to CChainState due to the refactoring due in the next commit, which is possible in its RPC invocations but isn't so readily available in Qt code, where it is also called. The fallback code has the benefit of not relying on CChainState.

FlushStateToDisk and {Enforce, Invalidate, MarkConflicting}Block are all CChainState functions, no need to access our own members through chainstate globals when we can access them directly.

adapted from a5595b1 and 5e54aa9 in bitcoin#23288. changes to coinselector_tests must be reverted before backports related to it are done as these changes are primarily motivated by bitcoin#21866 taking away the global chainstate, which breaks coinselector_tests and these changes skips over a lot of backporting, making them incompatible with backporting efforts (or even the commits its adapted from). the existing behaviour was that it creates its own testNode but doesn't populate it with a ChainstateManager (or much at all) while the rest of the client uses WalletTestingSetup's values. for the longest time this was fine because addCoins > listMNCollaterials > ChainActive() meant that it was using the fallback, but when chainstate globals are removed, WalletTestingSetup's values can't serve as a fallback anymore as it'd now be looking for NodeContext::chainman::m_chain, and chainman wasn't setup with testNode and the tests crash.

UdjinM6

utACK 0213fbe

knst

utACK 0213fbe

force-pushes looks good for me

PastaPastaPasta

utACK 0213fbe

, bitcoin#23174, bitcoin#23785, bitcoin#23581, bitcoin#23974, bitcoin#22932, bitcoin#24050, bitcoin#24515 (blockstorage backports) 1bf0bf4 merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh) 5c1eb67 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh) c440304 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh) e303a4e merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh) 301163c merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh) 732e871 merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh) b402fd5 merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh) a08f2f4 merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh) 472caa0 merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh) d69ca83 merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh) 6df927f chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6078 * Dependent on #6074 * Dependent on #6083 * Dependent on #6119 * Dependency for #6138 * In [bitcoin#24050](bitcoin#24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic. * Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/spork.cpp#L44)) and upstream's usage of it to process translatable strings ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/util/translation.h#L55-L62)). Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore. ## Breaking Changes None expected ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1bf0bf4 (with one nit) knst: utACK 1bf0bf4 PastaPastaPasta: utACK 1bf0bf4 Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4

knst · 2024-10-14T10:34:06Z

src/wallet/test/coinselector_tests.cpp

-CoinEligibilityFilter filter_confirmed(1, 1, 0);
-CoinEligibilityFilter filter_standard_extra(6, 6, 0);
-CoinSelectionParams coin_selection_params(false, 0, 0, CFeeRate(0), 0, false);
+CoinSelectionParams coin_selection_params(/* use_bnb = */ false, /* change_output_size = */ 0,


unrelated refactoring!

, bitcoin#22742, bitcoin#19101, bitcoin#22183, bitcoin#22009, bitcoin#22938, bitcoin#23288, bitcoin#24592 (wallet backports: part 2) e2eea71 merge bitcoin#24592: Delete old line of code that was commented out (Kittywhiskers Van Gogh) cf8d5a3 merge bitcoin#23288: remove usage of LegacyScriptPubKeyMan from some wallet tests (Kittywhiskers Van Gogh) e99d065 merge bitcoin#22938: Add remaining scenarios of 0 waste, in wallet waste_test (Kittywhiskers Van Gogh) 31f8f9e merge bitcoin#22009: Decide which coin selection solution to use based on waste metric (Kittywhiskers Van Gogh) 24695e1 merge bitcoin#22183: Remove `gArgs` from `wallet.h` and `wallet.cpp` (Kittywhiskers Van Gogh) 14aa05d merge bitcoin#19101: remove ::vpwallets and related global variables (Kittywhiskers Van Gogh) 0d64290 refactor: separate out Dash-specific RPCs that rely on wallet logic (Kittywhiskers Van Gogh) 1548058 partial bitcoin#19101: remove ::vpwallets and related global variables (Kittywhiskers Van Gogh) 1fcdc96 test: remove unneeded code from some `wallet_tests` (Kittywhiskers Van Gogh) 5dc5090 refactor: move tests to match upstream order, Dash tests at the tail (Kittywhiskers Van Gogh) 2c24c63 merge bitcoin#22742: Use proper target in do_fund_send (Kittywhiskers Van Gogh) 2f3ceba merge bitcoin#22686: Use GetSelectionAmount in ApproximateBestSubset (Kittywhiskers Van Gogh) f94993c merge bitcoin#22008: Cleanup and refactor CreateTransactionInternal (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6517 * Dependent on #6594 * [bitcoin#19101](bitcoin#19101) has been split into two due to it performing two refactors in one commit, the first half deals with utilizing interfaces and managers as defined in `WalletContext` (as opposed to either passing them in separately as arguments or accessing them through globals) and the second half deals with deglobalizing wallet variables like `::vpwallets` and `cs_wallets`. * `WalletContext` still needs to be initialized with a constructor as it stores a const-ref `std::unique_ptr` of `interfaces::CoinJoin::Loader`. This requirement hasn't been done away with. Tests that don't have access to `coinjoin_loader` will still need to pass `nullptr`. * Bitcoin registers wallet RPCs through `WalletClient::registerRpcs()` ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/interfaces.cpp#L512-L522)), which a part of the `ChainClient` interface ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/interfaces/chain.h#L292-L293)). They are enumerated ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/rpcwallet.cpp#L4702-L4776)) differently from non-wallet RPCs ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/rpc/blockchain.cpp#L2638-L2682)). Wallet RPCs aren't supposed to have knowledge of `NodeContext` and likewise non-wallet RPCs aren't supposed to have knowledge of `WalletContext`. So far, Bitcoin has reworked their RPCs to maintain this separation and upstream RPCs are a part of the `libbitcoin_wallet` library. This isn't the case for some Dash RPCs, many of which reside in `libbitcoin_server`, some of which fit into two categories. * Requires knowledge of both `NodeContext` and `WalletContext` * Knowledge of `WalletContext` wasn't mandatory before deglobalization as wallet information could be accessed through the global context courtesy of `::vpwallets` and friends but now that it is, many RPCs need to be able to access `WalletContext` when otherwise they wouldn't need to. * Due to the mutual exclusivity mentioned above, RPCs that access `WalletContext` _cannot_ access `NodeContext` as their RPC registration logic doesn't give them access to `NodeContext` ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/interfaces.cpp#L516-L517), `m_context` here is `WalletContext`) but in order to give those RPCs `WalletContext`, we need to register them as wallet RPCs, which prevent us from accessing `NodeContext`. * This has been **tentatively** worked around by including a pointer to `NodeContext` in `WalletContext` ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/wallet/context.h#L47-L52)) and modifying `EnsureAnyNodeContext()` to fetch from `WalletContext` if regular behavior doesn't yield a `NodeContext` ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/rpc/server_util.cpp#L28-L37)). * Are RPCs that possess both wallet and non-wallet functionality (i.e. have "reduced" capabilities if wallet support isn't compiled in as opposed to being absent altogether). * To enable wallet functionality, the RPCs need to be _registered_ as wallet RPCs. If wallet functionality is disabled, those RPCs are not registered. Simple enough, unless you have an RPC that can _partially_ work if wallet functionality as disabled, as the lack of registration would mean those RPCs are _entirely_ inaccessible. * This is also **tentatively** worked around by registering them as non-wallet RPCs in the cases where wallet RPCs aren't registered (i.e. if wallet support isn't compiled in **or** if wallet support is disabled at runtime). * Bitcoin doesn't use `#ifndef ENABLE_WALLET` for checking if support is absent, we're expected to use `!g_wallet_init_interface.HasWalletSupport()` (which will always evaluate `false` if support isn't compiled in, [source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/dummywallet.cpp#L19), as the stub wallet would be utilized to initialize the global in those cases, [source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/dummywallet.cpp#L57)). This has been done in the PR. * A notable change in this PR is that a lot of behavior that would be enabled if support was compiled in but disabled at runtime would now be disabled if support was disabled at runtime as `wallet_loader` won't be initialized if runtime disablement occurs ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/init.cpp#L128-L131)), which in turns means that anything that relies on `wallet_loader` wouldn't work either, such as `coinjoin_loader`. This means that we also need to register the RPC as a non-wallet RPC if support is compiled in but disabled at runtime ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/rpc/evo.cpp#L1841-L1843)). * To register RPCs as wallet RPCs, generally they're done through `WalletClient::registerRpcs()`, which iterates through `GetWalletRPCCommands()`. This is perfectly fine as both reside in `libbitcoin_wallet`. Unfortunately, our Dash RPCs reside in `libbitcoin_server` and `registerRpcs()` cannot be extended to enumerate through RPCs not included in its library. We cannot simply move the Dash RPCs either (at least in its current state) as they rely on non-wallet logic, so moving the source files for those RPCs into `libbitcoin_wallet` would pull more or less, all of `libbitcoin_server` along with it. * To **tentatively** get around this, a new method has been defined, `WalletLoader::registerOtherRpcs()`, which will accept any set of RPCs for registration ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/wallet/interfaces.cpp#L603-L606)) and we use it in RPC init logic ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/init.cpp#L1513-L1528)). * Some usage of `CoreContext` had to be removed ([source](b27d2dc#diff-157f588a09ff1da05b8c74acfcfb4da21917b6a97ed5d2e702228d621d23e66dL1024)) as without those removals, the test suite would crash. <details> <summary>Crash:</summary> ``` dash@01fd4f6cfa52:/src/dash$ lldb ./src/test/test_dash (lldb) target create "./src/test/test_dash" Current executable set to '/src/dash/src/test/test_dash' (x86_64). (lldb) r -t wallet_tests Process 653232 launched: '/src/dash/src/test/test_dash' (x86_64) Running 17 test cases... unknown location(0): fatal error: in "wallet_tests/CreateTransactionTest": unknown type wallet/test/wallet_tests.cpp(1052): last checkpoint *** 1 failure is detected in the test module "Dash Core Test Suite" Process 653232 exited with status = 201 (0x000000c9) ``` </details> * The assertion introduced in [bitcoin#22686](bitcoin#22686) ([source](https://github.com/achow101/bitcoin/blob/92885c4f69a5e6fc4989677d6e5be8a666fbff0d/src/wallet/spend.cpp#L781-L783)) has not been backported as this assertion is downgraded to an error in [bitcoin#26611](bitcoin#26611) and then further modified in [bitcoin#26643](bitcoin#26643) ([source](bitcoin@c1a84f1#diff-6e06b309cd494ef5da4e78aa0929a980767edd12342137f268b9219167064d13R1016-R1020)), portions of which are already included in [dash#6517](#6517) ([source](https://github.com/dashpay/dash/blob/012298acb071a45fa943653197ccc4f2c48a75a4/src/wallet/wallet.cpp#L3832-L3835)). * [bitcoin#23288](bitcoin#23288) finished what e3687f7 ([dash#6078](#6078)) started and `coinselection_tests` has now been realigned with upstream. * Dash-specific tests in `wallet_tests` (as introduced in [dash#3367](#3667)) have not been moved over to descriptor wallets and are still based on legacy wallets (to this effect, the old `AddKey` logic has been retained as `AddLegacyKey`, [source](https://github.com/dashpay/dash/blob/d4e8d1f1628933d9d9159fc261b22fc4427a1c7a/src/wallet/test/wallet_tests.cpp#L873-L879)). * To prevent merge conflicts, Dash-specific tests have been moved to the end of the source file and demarcated by a comment ([source](https://github.com/dashpay/dash/blob/d4e8d1f1628933d9d9159fc261b22fc4427a1c7a/src/wallet/test/wallet_tests.cpp#L868)). ## How Has This Been Tested? 6813863 was tested on Debian 12 (`bookworm`) mixing ~1 tDASH on default settings. ![CoinJoin run](https://github.com/user-attachments/assets/bdc6a20b-a2f5-4826-96b2-a1954e579e00) ## Breaking Changes * The RPCs `coinjoin`, `coinjoinsalt`, `getpoolinfo`, `gobject list-prepared`, `gobject prepare`, `gobject vote-alias`, `gobject vote-many`, `masternode outputs`, `protx update*`, `protx register*` and `protx revoke` will no longer be available if wallet support is disabled at runtime. This is not a change in **functionality** as they were already inoperative with disabled wallets but is a change in **reporting** as they would not be available at all. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: LGTM, utACK e2eea71 PastaPastaPasta: utACK e2eea71 Tree-SHA512: b3237670c2c861a353ce2486a1f0932b0bfcc6df488592cff28b85a49ed96c4612fc88866d0808e6d2cf0d409e4b8e8120e605acd7367b4bf554009672d3e9ea

kwvg changed the title ~~refactor, backport: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate)~~ refactor: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate) Jun 25, 2024

kwvg added this to the 21 milestone Jun 25, 2024

kwvg marked this pull request as ready for review June 25, 2024 11:28

kwvg requested review from knst, UdjinM6 and PastaPastaPasta June 25, 2024 11:28

github-actions bot added the needs rebase label Jun 25, 2024

kwvg force-pushed the bye_gchainman branch from 853f5dc to 8e32493 Compare June 25, 2024 14:10

github-actions bot removed the needs rebase label Jun 25, 2024

PastaPastaPasta reviewed Jun 25, 2024

View reviewed changes

knst previously approved these changes Jun 25, 2024

View reviewed changes

UdjinM6 reviewed Jun 26, 2024

View reviewed changes

kwvg dismissed knst’s stale review via 99e7953 June 26, 2024 10:54

kwvg force-pushed the bye_gchainman branch 2 times, most recently from 99e7953 to 4646eb7 Compare June 26, 2024 10:58

kwvg requested review from knst, UdjinM6 and PastaPastaPasta June 26, 2024 11:06

kwvg added 6 commits June 26, 2024 11:25

refactor: stop using ::Chain{state}Active() in GetUTXO*

6abf7f8

refactor: stop using ::ChainstateActive() in GetBlockHash

c48c0e7

refactor: don't use globals to access members we can directly access

ed56dbd

FlushStateToDisk and {Enforce, Invalidate, MarkConflicting}Block are all CChainState functions, no need to access our own members through chainstate globals when we can access them directly.

refactor: drop usage of chainstate globals in coinjoin logic

a475f5f

refactor: drop usage of chainstate globals in governance logic

21cc12c

kwvg force-pushed the bye_gchainman branch from 4646eb7 to 0213fbe Compare June 26, 2024 13:00

kwvg added 3 commits June 26, 2024 13:50

refactor: drop usage of chainstate globals in asset locks logic

fa20718

refactor: drop usage of chainstate globals in llmq logic

303c6bb

refactor: drop usage of chainstate globals in masternode logic

208b1c0

kwvg added 3 commits June 26, 2024 13:50

refactor: drop usage of chainstate globals in spork logic

0f4184c

merge bitcoin#21866: Farewell, global Chainstate!

0213fbe

UdjinM6 approved these changes Jun 27, 2024

View reviewed changes

knst approved these changes Jun 27, 2024

View reviewed changes

PastaPastaPasta approved these changes Jun 27, 2024

View reviewed changes

PastaPastaPasta merged commit b316be7 into dashpay:develop Jun 27, 2024
5 of 11 checks passed

kwvg mentioned this pull request Jun 28, 2024

backport: merge bitcoin#21727, #22371, #21526, #23174, #23785, #23581, #23974, #22932, #24050, #24515 (blockstorage backports) #6085

Merged

5 tasks

knst reviewed Oct 14, 2024

View reviewed changes

kwvg mentioned this pull request Jan 14, 2025

backport: merge bitcoin#21207, #22008, #22686, #22742, #19101, #22183, #22009, #22938, #23288, #24592 (wallet backports: part 2) #6529

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate) #6078

refactor: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate) #6078

kwvg commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024

PastaPastaPasta left a comment

PastaPastaPasta Jun 25, 2024

kwvg Jun 26, 2024

knst left a comment

knst Jun 25, 2024

kwvg Jun 26, 2024 •

edited

Loading

UdjinM6 Jun 26, 2024

UdjinM6 Jun 26, 2024

kwvg commented Jun 26, 2024

UdjinM6 left a comment •

edited

Loading

knst left a comment

PastaPastaPasta left a comment

knst Oct 14, 2024

refactor: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate) #6078

refactor: drop usage of chainstate globals in Dash-specific code, merge bitcoin#21866 (goodbye to a global chainstate) #6078

Conversation

kwvg commented Jun 25, 2024 • edited Loading

Additional Information

Breaking Changes

Checklist:

github-actions bot commented Jun 25, 2024

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta Jun 25, 2024

Choose a reason for hiding this comment

kwvg Jun 26, 2024

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

knst Jun 25, 2024

Choose a reason for hiding this comment

kwvg Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

UdjinM6 Jun 26, 2024

Choose a reason for hiding this comment

UdjinM6 Jun 26, 2024

Choose a reason for hiding this comment

kwvg commented Jun 26, 2024

UdjinM6 left a comment • edited Loading

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst Oct 14, 2024

Choose a reason for hiding this comment

kwvg commented Jun 25, 2024 •

edited

Loading

kwvg Jun 26, 2024 •

edited

Loading

UdjinM6 left a comment •

edited

Loading