0213fbebe6 merge bitcoin#21866: Farewell, global Chainstate! (Kittywhiskers Van Gogh)
e3687f790a test, bench: remove globals vCoins and testWallet from test and bench (Kittywhiskers Van Gogh)
0f4184cd70 refactor: drop usage of chainstate globals in spork logic (Kittywhiskers Van Gogh)
208b1c079b refactor: drop usage of chainstate globals in masternode logic (Kittywhiskers Van Gogh)
303c6bb4db refactor: drop usage of chainstate globals in llmq logic (Kittywhiskers Van Gogh)
fa20718b4f refactor: drop usage of chainstate globals in asset locks logic (Kittywhiskers Van Gogh)
21cc12c62a refactor: drop usage of chainstate globals in governance logic (Kittywhiskers Van Gogh)
a475f5f4e5 refactor: drop usage of chainstate globals in coinjoin logic (Kittywhiskers Van Gogh)
ed56dbdbc4 refactor: don't use globals to access members we can directly access (Kittywhiskers Van Gogh)
c48c0e79f3 refactor: stop using `::ChainstateActive()` in `GetBlockHash` (Kittywhiskers Van Gogh)
6abf7f8b63 refactor: stop using `::Chain`{`state`}`Active()` in `GetUTXO*` (Kittywhiskers Van Gogh)
f6f7df3731 rpc: don't use GetUTXOCoin in CDeterministicMN::ToJson() (Kittywhiskers Van Gogh)
Pull request description:
```
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](https://github.com/dashpay/dash/pull/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](5baa522225/src/rpc/evo.cpp (L1286))) through `Ensure`(`Any`)`Chainman`. Not the case in Qt code ([source](5baa522225/src/qt/masternodelist.cpp (L369))), 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](5baa522225/src/init.cpp (L1915-L1918))), the active chainstate hasn't been loaded in yet as that happens further down ([source](5baa522225/src/init.cpp (L1988-L1991))).
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](5baa522225/src/rpc/mining.cpp (L381-L382)), [source](5baa522225/src/test/util/setup_common.cpp (L391-L392))), `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 `CChainState`s, 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, a5595b13 and 5e54aa9b from [bitcoin#23288](https://github.com/bitcoin/bitcoin/pull/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.
<details>
<summary>Pre-fix crash stacktrace: </summary>
```
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
```
</details>
## 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:
- [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 0213fbebe6
knst:
utACK 0213fbebe6
PastaPastaPasta:
utACK 0213fbebe6
Tree-SHA512: 839f3f5b2af018520f330c4f4727622471d6225640c98853f28c3d88c4b6c728091b5e0c35b320e82979e5cd1357902fa1212afa4b6977967f05c636a25cc3b0
adapted from a5595b13 and 5e54aa9b 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.
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.
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.
c9a600e0fa fix: linkage error - message signer better to be common code rather than libconsensus (Konstantin Akimov)
8299b3b369 feat: protxregistar implementation for descriptor wallets (Konstantin Akimov)
6f45432f76 refactor: removed unused SignSpecialTxPayloadByString (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
RPC `protx updateregistar` uses forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as
```
test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
```
## What was done?
New method `SignSpecialTxPayloadByHash` is implemented in interface instead exporting raw private key for some address.
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## How Has This Been Tested?
Functional test `feature_dip3_deterministicmns.py` to run by both ways - legacy and descriptor wallets.
Run unit and functional tests.
Extra test done locally:
```diff
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
if self.options.descriptors is None:
# Prefer BDB unless it isn't available
- if self.is_bdb_compiled():
- self.options.descriptors = False
- elif self.is_sqlite_compiled():
+ if self.is_sqlite_compiled():
self.options.descriptors = True
+ elif self.is_bdb_compiled():
+ self.options.descriptors = False
```
to flip flag descriptor wallets/legacy wallets for all functional tests.
## Breaking Changes
N/A
## 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
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK c9a600e0fa
Tree-SHA512: 00aea1cd9db3537b9a9dcdee096d47ea48337edeea3f52ad54aea91781678b8641ac2dd86b67f61f87e3912945bcb5361a42a3279b6c08bb8d9f096bed8fe842
3971613285 feat: functional tests for RPC getgovernanceinfo (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/63
## What was done?
It adds functional test for `getgovernanceinfo`
## How Has This Been Tested?
Run unit/functional tests
Check output of `test/functional/test_runner.py -j20 --previous-releases --coverage --extended`
```
Uncovered RPC commands:
- cleardiscouraged
- debug
- getblockheaders
- getmerkleblocks
- getpoolinfo
- voteraw
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 3971613285
Tree-SHA512: f2d93db9068a707f2b33dd841b14f266db226fc5e907f19a49cf7a856b840f0c2c1f521c462996888933bb0ed636c288db12b1d36978c124cb536ea1ab9f3892
dc15420470 refactor: use EnsureConnman and EnsurePeerman when possible (Konstantin Akimov)
dc01f07f74 fix: add missing checks for not nullptr in rpc for dash specific code (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
There're some usages of unique pointers in RPC code that are not guarded by non-nullptr checks
## What was done?
Added missing `CHECK_NON_FATAL` and refactored some of them to `EnsureConnman` and `EnsurePeerman`
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK dc15420470
Tree-SHA512: 0483d9208f38648cad55804791415b2facfd165514651463f9b48c44d1963be2888471bb6b1f51015bb9ddd160f272b86f42ee67aea66e640c410175412f5f75
0365b06fab fix: reset rounds to 0 when tx fee is not 0 (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
> a coinjoin send with say 10x 1 + 0.001 denom -> 10 denom, and this doesn’t reset rounds to 0
reported by @PastaPastaPasta 👍
## What was done?
only real coinjoin tx can be a 0 fee tx with denoms, use that fact to avoid the edge case described above
## How Has This Been Tested?
reproduced the issue
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
ACK 0365b06fab
Tree-SHA512: b72ac5c01cc77c81945c3ae00b80fbdc4884acf3cfadc3fbcf4b78ff5655cce3b2ece4885478e5bf7476d351411df88b967d2baa3925155cffdc6054dd3bb502
These cleanups include:
- Removing a leftover protobuf reference
- Removing instructions on how to build for 32-bit Windows
- As backports assumed that there are only instructions for 64-bit
Windows persist, their diff only made one change instead of two.
But as the 32-bit instructions precede 64-bit, only they received
changes. The following were affected.
- Removal of tarball comment
- Addition of WSL-specific instructions
- Removing references to MSVC as we do not support it
- Adding instructions for setting the default compiler for mingw32 to
POSIX
- Adding instructions for escaping %PATH%s with spaces
- Adding instructions for cloning Dash Core into the mounted filesystem
if using WSL
- Adding reference to `dependencies.md`
- Adding suggestion to use `-j`
- Realigning some language with upstream
- Add git as a dependency
241f073932 feat: rpc external users are comma separated list (Konstantin Akimov)
68def970ad refactor: re-order arguments options alphabetically (Konstantin Akimov)
c7efd56a07 feat: rpc external users: use 2 queues but no extra threads (Konstantin Akimov)
c575a5808a feat: change handler to '/' for external users, use only rpc user name to choose queue (Konstantin Akimov)
f1c1fd873e feat: implementation for /external handler for RPC (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
To avoid struggling to response to critical rpc requests, and split them from 3rd parties who uses a node as an external service, there are introduced one more queue of requests that will be served without throttling for instance consensus important rpcs
## What was done?
new command line arguments:
- `rpcexternaluser` - List of comma-separated usernames for JSON-RPC external connections. If not specified, there's no special queue is created, all requests in one queue
- `rpcexternalworkqueue=<n>` - Set the depth of the work queue to service external RPC calls
## How Has This Been Tested?
Functional test `rpc_platform_filter.py` is updated to test new functionality
## Breaking Changes
NA
## 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
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 241f073932
PastaPastaPasta:
utACK 241f073932
Tree-SHA512: 15b371f24f5302853b85419e2b20c29749d6aae1c98a541d7471f1d3a681643063302c2a5ecce04dfad2da9101ea69d2f08a7e0e11a28609c6011d78273c57a7
18328279ec fix: force mnsync to skip gov obj sync on reconnection (UdjinM6)
08331bb950 fix: apply suggestions (UdjinM6)
3c3489d7a1 test: add test (UdjinM6)
41ab95dbf8 feat: skip governance checks for blocks below the best chainlock (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
A node can miss governance trigger sometimes and then it would stuck not being able to sync any further. This issue can be fixed manually by resetting sync status and reconsidering the "invalid" block. However, that's inconvenient. Also, what it does under the hood is it simply disables some parts of block validation. We could do that automagically and more precise if we would trust ChainLocks instead.
## What was done?
Skip governance checks for blocks below the best known chainlock, add tests.
## How Has This Been Tested?
Run tests.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 18328279ec
PastaPastaPasta:
utACK 18328279ec
Tree-SHA512: 3cc4e2707e24b36c9f64502561667d0cb66eced7019db7941781ab1b84cfd267b3dab4684c71b059e074450ea76dc8e342744bffdd1ca1be6ccceb34b3580659
