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
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
4e81732c57 Merge #21200: test: Speed up rpc_blockchain.py by removing miniwallet.generate() (Wladimir J. van der Laan)
4ab6aa4598 Merge #21293: test: Replace accidentally placed bit-OR with logical-OR (Wladimir J. van der Laan)
5a77c02188 Merge #19698: test: apply strict verification flags for transaction tests and assert backwards compatibility (Wladimir J. van der Laan)
259a767a38 Merge #21211: test: Move P2WSH_OP_TRUE to shared test library (MarcoFalke)
91e0359df4 Merge #21205: build: actually fail when Boost is missing (fanquake)
e3e69290a2 Merge #21188: scripted-diff: Remove redundant lock annotations in net processing (MarcoFalke)
5ec99ff3b4 Merge #20715: util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
292861a9bc Merge #20868: validation: remove redundant check on pindex (MarcoFalke)
e5249fb307 Merge #21012: ci: Fuzz with integer sanitizer (MarcoFalke)
785f7310ed Merge #20079: p2p: Treat handshake misbehavior like unknown message (MarcoFalke)
147d391c74 Merge #20566: refactor: Use C++17 std::array where possible (MarcoFalke)
Pull request description:
## What was done?
Backports from bitcoin v22
## How Has This Been Tested?
Run unit and functional tests
## Breaking Changes
N/A
## Checklist:
- [ ] 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 4e81732c57b9513d0cc3de53d4a61918b064a3e7; LGTM
UdjinM6:
utACK 4e81732
Tree-SHA512: f18a73427b7f86bdc4cf9a3c2569b970d814a68705ed386196a143914860b52362365f24283f5b69bf1684ac35bbec5051a9728c475795ffdf3cf3a6ce217c00
faa137eb9eac5554504b062a6dc865ca87fd572b test: Speed up rpc_blockchain.py by removing miniwallet.generate() (MarcoFalke)
fa1fe80c757df0adcbfaf41b5c5c8a468bc07b6f test: Change address type from P2PKH to P2WSH in rpc_blockchain (MarcoFalke)
fa4d8f3169e38cbdbae20258efebe7070c49f522 test: Cache 25 mature coins for ADDRESS_BCRT1_P2WSH_OP_TRUE (MarcoFalke)
fad25153f5c8e88f72cf666b16b0b0dbdc45d3b1 test: Remove unused bug workaround (MarcoFalke)
faabce7d07c5776e4116b1a7ad1f6c408a4a4e46 test: Start only the number of nodes that are needed (MarcoFalke)
Pull request description:
Speed up various tests:
* Remove unused nodes, which only consume time on start/stop
* Remove unused "bug workarounds"
* Remove the need for `miniwallet.generate()` by adding `miniwallet.scan_blocks()`. (On my system, with valgrind, generating 105 blocks takes 3.31 seconds. Rescanning 5 blocks takes 0.11 seconds.)
ACKs for top commit:
laanwj:
Code review ACK faa137eb9eac5554504b062a6dc865ca87fd572b
Tree-SHA512: ead1988d5aaa748ef9f8520af1e0bf812cf1d72e281ad22fbd172b7306d850053040526f8adbcec0b9a971c697a0ee7ee8962684644d65b791663eedd505a025
df8f2a11dcadd08840c5311631132a522650a83e test: Replace accidentally placed bit-OR with logical-OR (Hennadii Stepanov)
Pull request description:
This PR is a follow up of #19698.
ACKs for top commit:
glozow:
utACK df8f2a11dc
Tree-SHA512: 36aba3e952850deafe78dd39775a568e89e867c8a352f511f152bce7062f614f5bb4f23266dbb33da5292c9ee6da5ccefce117e3168621c71d2140c8e7f58460
5786a818e1a96bc1dd65b0e81b05998876357a74 Verify that all validation flags are backward compatible (gzhao408)
b10ce9aa48c8937cb91fca05e29c68098a364d93 [test] check verification flags are minimal/maximal (gzhao408)
a260c22cad0672dda11f42f649ebdc7cfa53b16a [test] Check for invalid flag combinations (gzhao408)
a7098a2a8d2d23ee3be1d71ab8c71475bf5a31ee [refactor] use CheckTxScripts, TrimFlags, FillFlags (gzhao408)
7a77727b2f66f3d723e03e917f0cabb459c49d62 Apply minimal validation flags to tx_invalid tests (gzhao408)
9532591bedaecf7c2debe779dec0a0debec2623b [test] add BADTX setting for invalid txns that fail CheckTransaction (gzhao408)
4c06ebf1281f0f387ab7493fe15176a05247525e [test] fix two witness tests in invalid tests with empty vout (gzhao408)
158a0b268ca2f73a5d504791359d1eff2cf27715 Apply maximal validation flags to tx_valid tests (gzhao408)
0a76a39b633760d4668d39859605c05629ee0025 [test] fix CSV test missing OP_ADD (gzhao408)
19db590d044efe7d474a16720e5b56e7b55db54c [test] remove unnecessary OP_1s from CSV and CLTV tests (gzhao408)
Pull request description:
This uses the first 4 commits of #15045, rebased and added some comments. The diff is quite large already and I want to make it easy to review, so I'm splitting it into 2 PRs (transaction and script). Script one is WIP, I'll link it when I open it.
Interpretation of scripts is dependent on the script verification flags passed in.
In tests, we should always apply **maximal** verification flags when checking that a transaction is **valid**; any additional flags should invalidate the transaction. A transaction should not be valid because we forgot to include a flag, and we should apply all flags by default.
We should apply **minimal** verification flags when asserting that a transaction is **invalid**; if verification flags are applied, removing any one of them should mean the transaction is valid.
New verify flags must be backwards compatible; tests should check backwards compatibility and apply the new flags by default. All `tx_invalid` tests should continue to be invalid with the exact same verify flags. All `tx_valid` tests that don't pass with new flags should _explicitly_ indicate that the flags need to be excluded, and fail otherwise.
1. Flip the meaning of `verifyFlags` in tx_valid.json to mean _excluded_ verification flags instead of included flags. Edit the test data accordingly.
2. Trim unneeded flags from tx_invalid.json.
3. Add check to verify that tx_valid tests have maximal flags and tx_invalid tests have minimal flags.
4. Add checks to verify that flags are soft forks (#10699) i.e. adding any flag should only decrease the number of acceptable scripts. Test by adding/removing random flags.
ACKs for top commit:
achow101:
ACK 5786a818e1a96bc1dd65b0e81b05998876357a74
laanwj:
ACK 5786a818e1a96bc1dd65b0e81b05998876357a74
Tree-SHA512: 19195d8cf3299e62f47dd3443ae4a95430c5c9d497993a18ab80de9e24b1869787af972774993bf05717784879bc4592fdabaae0fddebd437963d8f3c96d9a73
22220ef6d5f331c9e1f3e9487eaf07ab13693921 test: Move P2WSH_OP_TRUE to shared test library (MarcoFalke)
Pull request description:
Otherwise it can't be used in other tests (unit, fuzz, bench, ...)
ACKs for top commit:
darosior:
ACK 22220ef6d5f331c9e1f3e9487eaf07ab13693921
Tree-SHA512: 1b636e751281291f7c21ac51c3d014f6a565144c9482974391c516228e756442b077655eda970eb8bdb12974b97855a909b2b60d518026a8d5f41aa15ec7cbc8
c5da2749e2f7375e292fb0982e8e252ae1adbce3 build: actually stop configure if Boost isn't available (fanquake)
cad8b527eaf7a93877e2249960866fd4db2d1c14 build: explicitly install libboost-dev package (fanquake)
Pull request description:
If Boost is not found via AX_BOOST_BASE, we don't actually stop
configuring, only a warning is emitted:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version MINIMUM_REQUIRED_BOOST or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
```
Instead we usually fail when one of the other AX_BOOST_* macros fails to find a library. These macros are slowly being
removed, and in any case, it makes more sense to fail earlier if Boost is missing.
If Boost is unavailable, the failure now looks like:
```bash
checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version 1.58.0 or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in <boost/version.hpp>. See http://randspringer.de/boost for more documentation.
configure: error: Boost is not available!
```
Note that we now just pass the version into AX_BOOST_BASE, which fixes it's display in the output (rather than showing `MINIMUM_REQUIRED_BOOST`).
This PR also has a commit that adds `libboost-dev` to our install instructions and CI. This package is currently installed as a side-effect of installing our other libboost-*-dev packages. However as those continue to disappear, it makes sense to install boost-dev explicitly.
ACKs for top commit:
laanwj:
Code review ACK c5da2749e2f7375e292fb0982e8e252ae1adbce3
MarcoFalke:
Concept ACK c5da2749e2f7375e292fb0982e8e252ae1adbce3
Tree-SHA512: f866062f9d7d3a2316b6c887f17c664b9cfff41fdc0cb99ca79d641240fb01a5ae0d34140e515bc465219e1b43d5ca84f7c55f48b9c5b45a80ff2795dafd072b
fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 scripted-diff: Remove shadowing lock annotations (MarcoFalke)
Pull request description:
Would be good to not redundantly copy the lock annotation from the class declaration to the member implementation. Otherwise it may not result in a compile failure if a new lock requirement is added to the member implementation, but not the class declaration.
ACKs for top commit:
amitiuttarwar:
ACK `fafddfadda`, confirmed that the annotations removed were all redundant. confirmed the claim of potential issue :)
hebasto:
ACK fafddfadda0c77876ba764c5b65ee5fa8e53a5e0
jonatack:
Light utACK fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 verified that the removed annotations in the definitions correspond to those in their respective declarations
Tree-SHA512: ea095c6d4e0bedd70d4e2d8a42b06cfd90c161ebfcaac13558c5dc065601a732e5f812f332104b7daa087aa57b8b0242b177799d22eef7628d77d4d87f443bf2
fa61b9d1a68820758f9540653920deaeae6abe79 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24a36b62df35d12ecf6c6370671568c8 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac17f93decd4ee38c956e7aa55983f0d test: Add tests (MarcoFalke)
fac05ccdade8b34c969b9cd9b37b355bc0aabf9c wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a68820758f9540653920deaeae6abe79
fjahr:
Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
c943282b5e6312537f885c811d43120ce2f5b766 validation: remove redundant check on pindex (jarolrod)
Pull request description:
This removes a redundant check on `pindex` being a `nullptr`. By the time we get to this step `pindex` is always a `nullptr` as the branch where it has been set would have already returned.
Closes#19223
ACKs for top commit:
Zero-1729:
re-ACK c943282
ajtowns:
ACK c943282b5e6312537f885c811d43120ce2f5b766 - code review only
MarcoFalke:
review ACK c943282b5e6312537f885c811d43120ce2f5b766 📨
theStack:
re-ACK c943282b5e6312537f885c811d43120ce2f5b766
Tree-SHA512: d2dc58206be61d2897b0703ee93af67abed492a80e84ea03dcfbf7116833173da3bdafa18ff80422d5296729d5254d57cc1db03fdaf817c8f57c62c3abef673c
faff3991a9be0ea7be31685fb46d94c212c5da34 ci: Fuzz with integer sanitizer (MarcoFalke)
Pull request description:
Otherwise the suppressions file will go out of sync
ACKs for top commit:
practicalswift:
cr ACK faff3991a9be0ea7be31685fb46d94c212c5da34: patch looks correct
Tree-SHA512: 349216d071a2c5ccf24565fe0c52d7a570ec148d515d085616a284f1ab9992ce10ff82eb17962dddbcda765bbd3a9b15e8b25f34bdbed99fc36922d4161d307c
faaad1bbac46cfeb22654b4c59f0aac7a680c03a p2p: Ignore version msgs after initial version msg (MarcoFalke)
fad68afcff731153d1c83f7f56c91ecbb264b59a p2p: Ignore non-version msgs before version msg (MarcoFalke)
Pull request description:
Handshake misbehaviour doesn't cost us more than any other unknown message, so it seems odd to treat it differently
ACKs for top commit:
jnewbery:
utACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a
practicalswift:
ACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a: patch looks correct
Tree-SHA512: 9f30c3b5c1f6604fd02cff878f10999956152419a3dd9825f8267cbdeff7d06787418b41c7fde8a00a5e557fe89204546e05d5689042dbf7b07fbb7eb95cddff
fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 refactor: Use C++17 std::array where possible (MarcoFalke)
Pull request description:
Using the C++11 std::array with explicit template parameters is problematic because overshooting the size will fill the memory with default constructed types.
For example,
```cpp
#include <array>
#include <iostream>
int main()
{
std::array<int, 3> a{1, 2};
for (const auto& i : a) {
std::cout << i << std::endl; // prints "1 2 0"
}
}
```
ACKs for top commit:
jonasschnelli:
Code Review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2
practicalswift:
cr ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2
vasild:
ACK fac7ab1d
promag:
Code review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2.
Tree-SHA512: ef7e872340226e0d6160e6fd66c6ca78b2ef9c245fa0ab27fe4777aac9fba8d5aaa154da3d27b65dec39a6a63d07f1063c3a8ffb667a98ab137756a1a0af2656
76b4300fdf fix: Let addrman handle multiple ports for all networks (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
There is really no need to do have this restriction in `addrman`, we check `AllowMultiplePorts()` in `connman` which should be enough already. We can safely re-align `addrman` to its upstream implementation as suggested in #6043.
## What was done?
Drop port "discrimination" in `addrman`, remove related tests.
## How Has This Been Tested?
Run tests, run dash-qt on mainnet/testnet
## 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:
kwvg:
ACK 76b4300fdf
PastaPastaPasta:
utACK 76b4300fdf422097b0b39460e80ee4da98247f03; is it really a fix or a refactor? doesn't change observable system behavior
knst:
utACK 76b4300fdf
Tree-SHA512: 50363b5de7a6c6ad4de18c08b0a5cc31e89be8e5304f9684ce2cc609df4de07ff6d8d010556b5f6651c698e1a86960dba8005cc26fdb00bd03c856752d3e06ef
fbffe06dad fix: suppress lint warnings for edge due to both missing epoll and kpoll (Konstantin Akimov)
b799683d60 fix: pass reference instead copy of argument in util/edge (Konstantin Akimov)
9d941aacb9 fix: removed unused assigned (Konstantin Akimov)
d9e2e47685 fix: add a missing file util/wpipe to linter lists (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Some source files in src/util is missing to specify as dash specific for linters
## What was done?
Added to list of dash's linters
## How Has This Been Tested?
Run `test/lint/lint-all.sh`
## 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:
UdjinM6:
utACK fbffe06dad
PastaPastaPasta:
utACK fbffe06dad
kwvg:
utACK fbffe06dad
Tree-SHA512: 05bd40f987a93b47ca939f37d6e5b62e2044f2acce7a6ea6c361594eace808da17b17e0149095daecd0fb5418c6cbedbc70ad07ac3b7a1156a94188b04c2fd00
fb8a4db8f6 Merge bitcoin/bitcoin#26717: test: Improve `check-doc.py` pattern (MarcoFalke)
349cad2865 Merge bitcoin/bitcoin#26708: clang-tidy: Fix `modernize-use-nullptr` in headers (MarcoFalke)
6bf786d168 Merge bitcoin/bitcoin#25735: net: remove useless call to IsReachable() from CConnman::Bind() (fanquake)
012b0b7169 Merge bitcoin/bitcoin#24258: test: check localaddresses in getnetworkinfo for nodes with proxy (MarcoFalke)
c67f527b0b Merge bitcoin-core/gui#448: Add helper to load font (Hennadii Stepanov)
8e0abeb1c1 Merge bitcoin-core/gui#345: Connection Type Translator Comments (Hennadii Stepanov)
688b66e9d1 Merge bitcoin-core/gui#266: Doc: Copyright: Fix embedded font file location (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Trivial backports
## What was done?
## How Has This Been Tested?
Built and ran tests locally; p2p_addr_relay.py fails locally. Not sure why
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] 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:
UdjinM6:
utACK fb8a4db8f6
knst:
utACK fb8a4db8f6
Tree-SHA512: abb9469f25c6d45acea01da6d2b9cb6df94822f61d06f80e3008b32d2522016370a1e0b0c9ff95b92df22c4f227fc40f7765d76f1987eac7603155fe2d894593
b478406f9f refactor: clean-up, missing const in rpc/quorums (Konstantin Akimov)
50c99e84f8 fix: adopt platform restriction for new composite commands (Konstantin Akimov)
e3c4d66ef3 refactor: quorum sign methods (Konstantin Akimov)
2af9d86654 refactor: use new approach for rpc quorums NNN (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
See #6051
## What was done?
Commands starting from 'quorum ...' uses new a new way to make composite commands.
## How Has This Been Tested?
Run unit/functional tests.
Please notice, there's required extra changes for platform restrictions, the subcommand is no more first argument, but part of command, space separated.
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK b478406f9f
PastaPastaPasta:
utACK b478406f9f
Tree-SHA512: c53856a3f45fee44a8130876ed4545f6f2c0f59f74e4d4745e7354097fee3a74ed526cc8160de532dbb4aeb81b82638ee912a0961b910692e78889baa2c976c3
a86f975ad9 fix: build with gcc 13.2.0 - missing header memory in addrdb (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
```
In file included from test/addrman_tests.cpp:5:
./addrdb.h:51:99: error: ‘std::unique_ptr’ has not been declared
51 | std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman);
| ^~~
./addrdb.h:51:114: error: expected ‘,’ or ‘...’ before ‘<’ token
51 | std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman);
|
```
## What was done?
adds missing header `<memory>` in addrdb
## How Has This Been Tested?
Run build - it works
## 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:
UdjinM6:
utACK a86f975ad9
PastaPastaPasta:
utACK a86f975ad9
Tree-SHA512: e25f63b6b823b3ef0a0dcb43386bed144ab68e05d09552dc109fb7687e16422949cfc4deb1b65d6d0d842299bd0e13da673d90aae695dcedeec4bc27f33c6f5f
71d14528ca refactor: enumerate each CNode argument on a separate line (Kittywhiskers Van Gogh)
f8d1e5b3ec merge bitcoin#25174: Add thread safety related annotations for CNode and Peer (Kittywhiskers Van Gogh)
4847f6e96f refactor: move `m_initial_sync_finished` out of header (Kittywhiskers Van Gogh)
dba4cf056b merge bitcoin#25591: Version handshake to libtest_util (Kittywhiskers Van Gogh)
b9b13bd8ec merge bitcoin#24141: Rename message_command variables in src/net* and src/rpc/net.cpp (Kittywhiskers Van Gogh)
a6aa3735be merge bitcoin#20196: fix GetListenPort() to derive the proper port (Kittywhiskers Van Gogh)
c443cf4825 merge bitcoin#24078: Rename CNetMessage::m_command with CNetMessage::m_type (Kittywhiskers Van Gogh)
182e31d04c merge bitcoin#23801: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp (Kittywhiskers Van Gogh)
6e6c9442fa merge bitcoin#23758: Use type-safe mockable time for peer connection time (Kittywhiskers Van Gogh)
7beeae77b9 merge bitcoin#23614: add unit test for block-relay-only eviction (Kittywhiskers Van Gogh)
cf8f17e423 merge bitcoin#22735: Don't return an optional from TransportDeserializer::GetMessage() (Kittywhiskers Van Gogh)
224fb687c8 merge bitcoin#20541: Move special CAddress-without-nTime logic to net_processing (Kittywhiskers Van Gogh)
30ac41e068 merge bitcoin#22284: performance improvements to ProtectEvictionCandidatesByRatio() (Kittywhiskers Van Gogh)
ad4369fd83 merge bitcoin#21571: make sure non-IP peers get discouraged and disconnected (Kittywhiskers Van Gogh)
Pull request description:
## 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:
PastaPastaPasta:
utACK 71d14528ca
UdjinM6:
utACK 71d14528ca
Tree-SHA512: b214d50fd87a046f22a9b3baf676483b4eee45ad267a4e501b221bbc77ef1e4532f0ad1fc8931be66761a9c50f2033ceeabbaae2bdb42f7c9edf7708bac8a9eb
64f34770c5 refactor: remove unused header spork.h from rpc/masternode (Konstantin Akimov)
43e9ab4966 refactor: use new type of composite commands for masternode NNN (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
See #6051
## What was done?
Commands starting from 'masternode ...' uses new a new way to make composite commands.
## How Has This Been Tested?
Run unit/functional tests.
Please notice, we support both styles `masternodelist` and `masternode list` which are still both supported.
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 64f34770c5
Tree-SHA512: 5a658588dff7f29d3025d2a720c8efe8096f79e8d632800911573f2f9a42450c2f27da070a94c5739e2027153413cdcacde82a7b4614467e036c008719692ed9
