fa4d0aacf2bbddaf1709660ffd8d520570533aa8 test: * -> & (MarcoFalke)
Pull request description:
This changes background_cs from being a pointer to a reference to work
around a gcc false warning. Also, this makes the test easier to read.
Fixes bitcoin#23101
Can be reviewed with --ignore-all-space.
ACKs for top commit:
practicalswift:
cr ACK fa4d0aacf2bbddaf1709660ffd8d520570533aa8
jamesob:
ACK fa4d0aacf2
hebasto:
ACK fa4d0aacf2bbddaf1709660ffd8d520570533aa8, tested on Linux Mint 20.2 (x86_64) by merging this PR on top of the current master.
Tree-SHA512: 93a0d8859201f7074bea52fab8f6701409148bc50cfbb142cacfa6c991fc12c07584df04fead645f11703883df99535423d154f9945202e1c5aff49540d9b607
2917c33206 partial Merge bitcoin-core/gui#205: Save/restore TransactionView and recentRequestsView tables column sizes (Konstantin Akimov)
e5c2c03984 Merge bitcoin-core/gui#154: qt: Support macOS Dark mode (MarcoFalke)
29a98c7826 Merge bitcoin-core/gui#251: Improve URI/file handling message (MarcoFalke)
48d66fd1ab Merge bitcoin-core/gui#248: Fix: For values of "Bytes transferred" and "Bytes/s" with 1000-based prefix names use 1000-based divisor instead of 1024-based (MarcoFalke)
63b18006a3 Merge bitcoin-core/gui#221: qt, refactor: rpcconsole translatable string fixes and improvements (MarcoFalke)
0d1faa203e fix: removed maximum width of transaction list on overview page (Konstantin Akimov)
458384ab93 Merge bitcoin-core/gui#176: Fix TxViewDelegate layout (MarcoFalke)
d670240d13 Revert "fix: remove stretching from Overview page when it's not needed" (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Backports of QT related improvements from bitcoin v22
## What was done?
See commits for list of backports.
Changes related to improved behavior of columns while resizing for Transactions List and Recent Requests is dropped due to low performance and buggy behaviour. bitcoin-core/gui#205 and bitcoin-core/gui#229 are DNM due to incompatibility with our table view.
It reverts also #5992 as better fix is found (see css changes).
## How Has This Been Tested?
Run qt app, try to resize main view with overview page.
## 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:
light ACK 2917c33206 (CI failure is unrelated)
PastaPastaPasta:
light-ACK 2917c33206
Tree-SHA512: b84c7e254562d86da30515d05572587edd1b451d988bbcfe16b8874f13d6b581910bed9008a7f8a408aae9181faa86703463fd508711e6e09b299be232ba3f48
1c5ea38c68 merge bitcoin#24197: Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() (Kittywhiskers Van Gogh)
e5e37458bb merge bitcoin#24002: add thread safety lock assertion to WriteBlockIndexDB() (Kittywhiskers Van Gogh)
04a3f65032 merge bitcoin#23721: Move restorewallet() logic to the wallet section (Kittywhiskers Van Gogh)
e47d5ac81e merge bitcoin#23154: add assumeutxo notes (Kittywhiskers Van Gogh)
847d866ff5 merge bitcoin#22738: fix failure in feature_nulldummy.py on single-core machines (Kittywhiskers Van Gogh)
ad96ef2d25 merge bitcoin#22633: Replace remaining binascii method calls (Kittywhiskers Van Gogh)
b37f609fd0 merge bitcoin-core/gui#399: Fix "Load PSBT" functionality when no wallet loaded (Kittywhiskers Van Gogh)
94173f14dd merge bitcoin#21850: Remove `GetDataDir(net_specific)` function (Kittywhiskers Van Gogh)
6264c7b7c7 merge bitcoin#21953: fuzz: Add utxo_snapshot target (Konstantin Akimov)
8b7ea28e80 merge bitcoin#21754: Run feature_cltv with MiniWallet (Kittywhiskers Van Gogh)
bd750140be merge bitcoin#21762: Speed up mempool_spend_coinbase.py (Kittywhiskers Van Gogh)
72eeb9a0d6 merge bitcoin#21732: Move common init code to init/common (Kittywhiskers Van Gogh)
3944d4ed96 chore: resolve nit from dash#6085 (blockstorage backports) (Kittywhiskers Van Gogh)
92509e2eee fix: don't suppress `-logtimestamps` help if `HAVE_THREAD_LOCAL` undef (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/6138
* In [bitcoin#21754](https://github.com/bitcoin/bitcoin/pull/21754), the `scriptSig` padding multiplier (`24`) differs from upstream (`35`) as the `vsize` value it corresponds to must match what is ordinarily generated (`85` vs `96` upstream) in order to fulfill an assertion ([source](d9835515cc/test/functional/test_framework/wallet.py (L107))).
* In [bitcoin#21953](https://github.com/bitcoin/bitcoin/pull/21953), the hash associated with height `200` is generated like this (this is the same method used in [dash#5236](https://github.com/dashpay/dash/pull/5236)):
* Add the height desired to the `CRegTestParams::m_assumeutxo_data` map with a garbage hash value (like `uint256::ONE`). This is to avoid an unrecognized metadata failure ([source](5211886fb4/src/validation.cpp (L5755-L5761))) caused by looking through the map to see if the height's there.
* Change the `LogPrintf(..)` in the serialized hash check error log message located [here](5211886fb4/src/validation.cpp (L5876-L5880)) to a `std::cout << strprintf(..)`
* Edit the value of `mineBlocks` [here](5211886fb4/src/test/validation_chainstatemanager_tests.cpp (L248-L253)) to be 100 blocks _less_ than the desired height.
* Compile Dash Core and run `./src/test/test_dash -t validation_chainstatemanager_tests`
* Take the `got` value printed to your terminal window/`stdout` (the `expected` value should be our garbage value from earlier, ignore that). That's your good hash.
* Update the `CRegTestParams::m_assumeutxo_data` map entry with the correct entry, reverse every change _except_ the map entry (for obvious reasons) and the `mineBlocks` change.
* Remember to add/update the hash [here](5211886fb4/src/test/validation_tests.cpp (L29-L31)) in `validation_tests`, it simply tests the hardcoded chainparams value with its own hardcoded value. That's also why we don't use this test since it'll just regurgitate the garbage values we give it.
* Compile and re-run the test. If it passes, your hash is good. Revert the `mineBlocks` change.
* Profit?
## 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 1c5ea38c68
PastaPastaPasta:
utACK 1c5ea38c68
Tree-SHA512: 1ce0d4f1cef68990412e2e7046b36db7425059ee41b39e3681fa05d59fe24a0a74ad8c5d833c0e4c0686f693af665ca749e504b88ad30e708fc163045160aa58
85762dc633 60%+: bg, ro, vi (UdjinM6)
2d0e68dcd6 80%+: ar, de, es, fi, fr, it, ja, ko, nl, pl, pt, ru, sk, th, tr, zh_CN, zh_TW (UdjinM6)
f7992b0b03 en (UdjinM6)
49fc976121 dashstrings (UdjinM6)
c8333a59c5 chore: replace remaining `...` with `…` in translated strings (UdjinM6)
ac2e9ea1e7 qt: Extract translations correctly from UTF-8 formatted source (Hennadii Stepanov)
Pull request description:
## Issue being fixed or feature implemented
Mostly regular translation updates but with 2 additional fixes:
- ac2e9ea is a backport, without it `make translate` fails to update `dashstrings.cpp` properly (but I couldn't figure out which PR it belongs to 🤷♂️ )
- c8333a5 is needed to make it easier to replace all `...` with `…` in `*.ts` files locally to avoid annoying translators (`...` and `…` are visually the same in transifex interface)
## What was done?
## How Has This Been Tested?
## Breaking Changes
## 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:
utACK 85762dc633
Tree-SHA512: c3624d8e5b26b0fd4d488e6988e81000d05bdc66f9e126b5d3fe3c1f6bceaa846ee81dfc7c0d18613b0ac682a94e0b17007e86724e7cc02d9cfce87b22ce6916
218862a01848f69d54380c780bb5eae6dfdb1416 Display peers in -netinfo that we don't relay addresses to (Jon Atack)
3834e23b251ed7b4a47bbb981faba65b97ecbba0 Display peers in -netinfo that request we not relay transactions (Jon Atack)
0a9ee3a2c787e97213a0456b0d6253c549b71e09 Simplify a few conditionals in -netinfo (Jon Atack)
5eeea8e2575a36587e70743af3bd7c2d87b8cf36 Add addr_processed and addr_rate_limited stats to -netinfo (Jon Atack)
Pull request description:
Update CLI -netinfo to display the getpeerinfo `addr_processed`, `addr_rate_limited`, `addr_relay_enabled` and `relaytxes` data with auto-adjusting column widths.
```
$ ./src/bitcoin-cli -netinfo help
txn Time since last novel transaction received from the peer and accepted into our mempool, in minutes
"*" - the peer requested we not relay transactions to it (relaytxes is false)
addrp Total number of addresses processed, excluding those dropped due to rate limiting
"." - we do not relay addresses to this peer (addr_relay_enabled is false)
addrl Total number of addresses dropped due to rate limiting
```
ACKs for top commit:
0xB10C:
Code review and tested ACK 218862a01848f69d54380c780bb5eae6dfdb1416
Zero-1729:
re-tACK 218862a01848f69d54380c780bb5eae6dfdb1416
vasild:
ACK 218862a01848f69d54380c780bb5eae6dfdb1416
jarolrod:
tACK 218862a01848f69d54380c780bb5eae6dfdb1416
Tree-SHA512: bb9da4bdd71859b234f6e4c2c46257a57ef0d0e0b363d2b8fded128bcaa28132f64a0a4651c622e1de1e3b7c05c7587a4369e9e79799895884fda9745c63409d
fa2547fc52b90b4bbde250803df24d7f665383a7 fuzz: Avoid timeout in blockfilter fuzz target (MarcoFalke)
Pull request description:
Previously it would take 10 seconds to run this input, now it takes 10ms: [clusterfuzz-testcase-blockfilter-5022838196142080.log](https://github.com/bitcoin/bitcoin/files/7021883/clusterfuzz-testcase-blockfilter-5022838196142080.log)
The fix is moving the `MatchAny` out of the hot loop.
Also, to avoid unlimited runtime, cap the hot loop at 30k iterations.
ACKs for top commit:
GeneFerneau:
Approach ACK [fa2547f](fa2547fc52)
Tree-SHA512: a04e7388856930ec81222da8f05b665a923fe9482aeb4c55c9be4561aa7320a0703dbbf8d438ae92854e877a8e3b46777a29c0b652b8f34c29c2142cc5d63ccb
127b4608e9dbb8217c74c9332e82fcec8c326fa8 test: Check if specified config file cannot be opened (nthumann)
6bb54708e6457f21596793a7149dc6dfea1dc871 util: Check if specified config file cannot be opened (nthumann)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22612.
When running e.g. `./src/bitcoind -datadir=/tmp/bitcoin -regtest -conf=/tmp/bitcoin/regtest/bitcoin.conf` and the specified config cannot be opened (doesn't exist, permission denied, ...), the initialization silently uses the default config.
As voidburn already noted:
> I can't think of a situation in which a config file is specified explicitly (in the startup options, as per service unit linked above), but inaccessible, where the fail condition should be to keep booting using defaults instead.
With this patch applied, the initialization will fail immediately, if the specified config file cannot be opened. If no config file is explicitly specified, the behavior is unchanged. This not only affects `bitcoind`, but also `bitcoin-cli` and `bitcoin-qt`.
In the example below the datadir is accessible, but the config file is not due to insufficient permissions:
```
$ ./src/bitcoind -datadir=/tmp/bitcoin -regtest --debug=1 -conf=/tmp/bitcoin/regtest/bitcoin.conf
Error: Error reading configuration file: specified config file "/tmp/bitcoin/regtest/bitcoin.conf" could not be opened.
```
ACKs for top commit:
0xB10C:
ACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Zero-1729:
tACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
theStack:
Tested ACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Tree-SHA512: 4fe487921485426f1d1da8d256c388af517b984b639d776aec7b159b3e23b669824093d3bdd31139d9415ed5f5de405b3e6a51b110c8ab471f12b9c99ac67cc1
e1030a058c docs: add release notes for 6140 (pasta)
9ed292a6e1 feat: harden all sporks on mainnet to current values (pasta)
Pull request description:
## Issue being fixed or feature implemented
Harden all sporks on the mainnet; they are no longer necessary. Although retaining them might be beneficial in addressing bugs or issues, the greater priority is to protect mainnet by minimizing risks associated with potential centralization or even its perception. Sporks will continue to be valuable for testing on developer networks; however, on mainnet, the risks of maintaining them now outweigh the benefits of retaining them.
## What was done?
Adjust CSporkManager::GetSporkValue to always return 0 for sporks in general and 1 for SPORK_21_QUORUM_ALL_CONNECTED specifically.
## How Has This Been Tested?
Ran main net node with this patch. Sporks show as expected
## Breaking Changes
This is not a breaking change.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] 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 e1030a058c
UdjinM6:
utACK e1030a058c (CI failure is unrelated)
Tree-SHA512: f20d0f614b7e9d6eb5606c545d0747a9d415f2905512dd6100a2f9fb00bb6de02c8d5aa74cb41aa39163fde0ab05fe472913acc227b1b4afce7e984f8897940d
1bf0bf492f merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh)
5c1eb67c42 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh)
c440304c85 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh)
e303a4ec45 merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh)
301163c65e merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh)
732e871a6b merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh)
b402fd57fa merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh)
a08f2f48bf merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh)
472caa048a merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh)
d69ca833df merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh)
6df927fc60 chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6078
* Dependent on https://github.com/dashpay/dash/pull/6074
* Dependent on https://github.com/dashpay/dash/pull/6083
* Dependent on https://github.com/dashpay/dash/pull/6119
* Dependency for https://github.com/dashpay/dash/pull/6138
* In [bitcoin#24050](https://github.com/bitcoin/bitcoin/pull/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](37e026a038/src/spork.cpp (L44))) and upstream's usage of it to process translatable strings ([source](37e026a038/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 1bf0bf492f (with one nit)
knst:
utACK 1bf0bf492f
PastaPastaPasta:
utACK 1bf0bf492f
Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4
We use Stretched column width, it's not compatible with gui#205
Though, this type of columns have better UI, but it is very slow performance,
see https://github.com/dashpay/dash/pull/6111#pullrequestreview-2178370404
-tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Date, QHeaderView::Interactive);
-tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Label, QHeaderView::Stretch);
+tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Date, QHeaderView::ResizeToContents);
+tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Label, QHeaderView::Interactive);
tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Message, QHeaderView::Stretch);
-tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Amount, QHeaderView::Fixed);
+tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Amount, QHeaderView::ResizeToContents);
ef3e1d7272d294c65cc9904079571cf67dc6c463 qt: Improve URI/file handling message (Hennadii Stepanov)
Pull request description:
This PR:
- fixes missing spaces after full stops
- makes the translation context much bigger
The latter is the main motivation for this PR, as I became a translator 🐅
Screenshots:
- master (a9d1b40d53ec417eefbe767aa66701ef8e1801d5)
- this PR:
ACKs for top commit:
jarolrod:
ACK ef3e1d7272d294c65cc9904079571cf67dc6c463
Tree-SHA512: 8fbd1e3731b75866356fae201b3129126001600ca0197e83c05825e8c5bbbcf0132d6a6b808d7a5cbfbdde75ed1865ecbb651c30017570abd7c5803eff2b9306
d09ebc47233187ab8dd5a70df4d261353958978c Fix wrong(1024) divisor for 1000-based prefixes (wodry)
Pull request description:
v.0.21.0
I saw in the GUI peer window in the "received" column `1007 KB`, and after increasing to >=1024 I guess, it switched to `1 MB`. I would have expected the display unit to change from KB to MB already at value >=1000.
I looked into the code, and the values appear to be power-of-2 byte values, so the switching at >=1024 and not >=1000 seems correct.
But the unit display is not precisely correct, binary prefixes should be used for power-of-2 byte values.
To be correct, this PR changes ~~KB/MB/GB to KiB/MiB/GiB.~~ KB to kB and the divisor from 1024 to 1000.
ACKs for top commit:
hebasto:
ACK d09ebc47233187ab8dd5a70df4d261353958978c, tested on Linux Mint 20.1 (Qt 5.12.8) the both "Network Traffic" and "Peers" tabs of the "Node Window".
jarolrod:
ACK d09ebc47233187ab8dd5a70df4d261353958978c
leonardojobim:
Tested ACK d09ebc4723 on Ubuntu 20.04 Qt 5.12.8
Tree-SHA512: 8f830b08cc3fd36dc8a18f1192959fe55d1644938044bf31d770f7c3bf8475fba6da5019a2d2024d5b2c81a8dab112f360c555367814a14f4d05c89d130f25b0
6242beeb067139c01dd27c63ebcd24df5808cb15 Hoist repeated translated strings to RPCConsole struct members (Jon Atack)
0f035c12fb0a5c5f98fc2b9907d475c08018df36 RPCConsole::updateDetailWidget: convert strings to translated strings (Jon Atack)
Pull request description:
- fixups from #206 review feedback (thanks!), see commit message for details
- hoists repeatedly used translatable strings to the `RPCConsole` class for reuse
ACKs for top commit:
hebasto:
re-ACK 6242beeb067139c01dd27c63ebcd24df5808cb15
Talkless:
tACK 6242beeb067139c01dd27c63ebcd24df5808cb15, tested on Debian Sid with Qt 5.15.2. I see "Ban for.." translated to my native language as before, "To/From/Yes/No" are not but that's expected, as `.ts` files are not updated.
jarolrod:
ACK 6242beeb067139c01dd27c63ebcd24df5808cb15
Tree-SHA512: 20a296511c5ac03a816766237fa2731b0360dedebf1bea02711eb21d7e4eae2a63a051fe48f4726052edc3e6318952f01fef920cd4b22a8196c39c23d8e5cc3a
It happened due to 2 PRs merged one after each other without rebasing:
- enabling -Wdocumentation in bitcoin#21631 (#6113)
- renaming param while doxygen comment is forgotten in #6114
4731f7045f docs: release notes for bitcoin#21141 - walletnotify %h %b (Konstantin Akimov)
044ddb4c80 Merge bitcoin/bitcoin#21777: test: Fix feature_notifications.py intermittent issue (MarcoFalke)
5336f42ea8 Merge bitcoin/bitcoin#19801: test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65) (MarcoFalke)
1cc6aa6c83 Merge bitcoin/bitcoin#21691: test: Check that no versionbits are re-used (MarcoFalke)
b55fdf8e68 Merge #21235: p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool (MarcoFalke)
ddc6fca7f3 Merge #21343: doc: revamp macOS build doc (fanquake)
709652bff7 Merge #21141: wallet: Add new format string placeholders for walletnotify (Wladimir J. van der Laan)
a3bee9c8ec Merge #21424: Net processing: Tidy up CNodeState ctor (MarcoFalke)
aab2a665c3 Merge #20556: rpc: Properly document return values (submitblock, gettxout, getblocktemplate, scantxoutset) (fanquake)
c26722fc0e Merge #21331: rpc: replace wallet raw pointers with references (#18592 rebased) (MarcoFalke)
Pull request description:
## What was done?
Backports from bitcoin v22:
- bitcoin/bitcoin#21331
- bitcoin/bitcoin#20556
- bitcoin/bitcoin#21424
- bitcoin/bitcoin#21141
- bitcoin/bitcoin#21343
- bitcoin/bitcoin#21235
- bitcoin/bitcoin#21691
- bitcoin/bitcoin#19801
- bitcoin/bitcoin#21777
## 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:
UdjinM6:
utACK 4731f7045f
PastaPastaPasta:
utACK 4731f7045f
Tree-SHA512: dcee684563e4e07e838e96df0bf5e49d59e8c85aea6beca3239782e7d90a24f13d828b1201073585b888b0b154b4ba8bb90f88a36e3f923ac03b46f595d75500
121c032e41 fix: avoid calling functions that change wallet state inside of `assert(...)` (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Functions that change the state should not be called inside `assert`s
kudos to @kwvg for noticing https://github.com/dashpay/dash/pull/6116#discussion_r1681163803
## What was done?
Move them out
## How Has This Been Tested?
## 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
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
kwvg:
LGTM, utACK 121c032e41
PastaPastaPasta:
utACK 121c032e41
Tree-SHA512: 42b6f55a62b05e3e632febf92f9d5ac63f32a7754fdd9dffa816d71cb0f72a32ac8315fba5ed96d8bc652728e768f2eb399e5ebb4aa39afe1546fa16e1046347
5943c93bdd feat: introduce `coinjoinsalt` RPC to allow manipulating per-wallet salt (Kittywhiskers Van Gogh)
4fa3d396f4 wallet: refactor `InitCoinJoinSalt()` to allow for setting a custom salt (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
CoinJoin utilizes a salt to randomize the number of rounds used in a mixing session (introduced in [dash#3661](https://github.com/dashpay/dash/pull/3661)). This was done to frustrate attempts at deobfuscating CoinJoin mixing transactions but also has the effect of deciding the mixing threshold at which a denomination is considered "sufficiently mixed", which in turn is tied this salt, that is in turn, tied to the wallet.
With wallets that utilized keypool generation, this was perfectly acceptable as each key was unrelated to the other and any meaningful attempts to backup/restore a wallet would entail backing up the entire wallet database, which includes the salt. Meaning, on restore, the wallet would show CoinJoin balances as reported earlier.
With the default activation of HD wallets in legacy wallets (and the introduction of descriptor wallets that construct HD wallets by default), addresses are deterministically generated and backups no longer _require_ a backup of the wallet database wholesale.
Users who export their mnemonic and import them into a new wallet will find themselves with a different reported CoinJoin balance (see below).
https://github.com/dashpay/dash/assets/63189531/dccf1b17-55af-423d-8c36-adea6163e060
## Demo
**Based on [`c00a3665`](c00a366578)**
https://github.com/dashpay/dash/assets/63189531/c60c10e3-e414-46af-a64e-60605a4e6d07
## 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 _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
ACK 5943c93bdd
Tree-SHA512: 8e5bdd18ecce4c14683fbeb1812b0b683a5a5b38405653655c3a14b1ab32a22b244b198087876cd53ca12447c2027432afe4259ef3043be7fddf640911d998f0
e65984e173 refactor: remove unused includes from unit tests (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Noticed there were some not used includes in Unit tests and removed them.
It's not exhaustive list of unused headers, just something that was easy to spot.
## How Has This Been Tested?
Just build and see that there's no build error.
This PR also slightly reduced compilation time of project, it's measurable big changes to6-8 seconds of one CPU core faster. Tested by compiling only files that changed in single-thread:
```
$ rm src/test/test_dash-blockfilter_index_tests.o src/test/test_dash-block_reward_reallocation_tests.o src/test/test_dash-denialofservice_tests.o src/test/test_dash-dynamic_activation_thresholds_tests.o src/test/test_dash-validation_block_tests.o src/test/test_dash-validation_chainstate_tests.o src/test/test_dash-validation_flush_tests.o src/test/util/libtest_util_a-mining.o src/test/util/libtest_util_a-setup_common.o
$ cd src/test ; time make -j1
```
## 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 e65984e173
PastaPastaPasta:
utACK e65984e173
Tree-SHA512: 445c1515aee09e946eb73f12ca9c8064d26cd6148d2286fa079bf54806ea77af9eda890a36c8f34450791e868fbe933183bc7e507dd6e667304ccd92790bb009
252ffee815 fix: adjust doxygen for dash codebase for -Wdocumentation (Konstantin Akimov)
4f260cd4b1 fix: ignore warnings for dashbls/ (Konstantin Akimov)
0afffcccc4 Merge #21613: build: enable -Wdocumentation (fanquake)
Pull request description:
## What was done?
Backport bitcoin#21613 to enable -Wdocumentation and related fixes for dash code
## How Has This Been Tested?
Build with clang compiler and see no failures anymore with `--enable-werror`
## 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 252ffee815
PastaPastaPasta:
utACK 252ffee815
Tree-SHA512: 1670076665e26228567042ed33ce149780c3992aaa78cdeafeefdc5609b1d1590034a3a2ddc056b69120b2b4e8b0dffaff729b8178043aebe19b1c5880a10d0d
fa8eaee6a8531db970cc84436bf2ae8150a58642 test: Run versionbits_sanity for all chains (MarcoFalke)
faec1e9ee1f12612831ad5b0f0a767d87bd2d024 test: Address outstanding versionbits_test feedback (MarcoFalke)
fad4167871c3c9fde462e64e3ef3be937e585084 test: Check that no versionbits are re-used (MarcoFalke)
Pull request description:
ACKs for top commit:
jnewbery:
Code review ACK fa8eaee6a8531db970cc84436bf2ae8150a58642
ajtowns:
ACK fa8eaee6a8531db970cc84436bf2ae8150a58642 code review only
Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c
fa8177324392c923c6ce39056cfd870af55ab673 style-only: Remove whitespace (MarcoFalke)
fae77b9e6dc9e59b355d56df49c4d9685b6f40a4 net: Simplify ProcessGetBlockData execution by removing send flag. (Patrick Strateman)
fae7c0429f96e08bcac944f6fa30264636dfda8c log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects (MarcoFalke)
Pull request description:
* Clarify that "ignoring" really means "disconnect" in the log
* Revive a refactor I took from #13670
ACKs for top commit:
jnewbery:
utACK fa8177324392c923c6ce39056cfd870af55ab673
sipa:
utACK fa8177324392c923c6ce39056cfd870af55ab673
Tree-SHA512: 0a4fcb979cb82c4e26012881eeaf903c38dfbb85d461476c01e35294760744746a79c48ffad827fe31c1b830f40c6e4240529c71e375146e4d0313c3b7d784ca
06e1fb0b170a69996a7ce1ef5203785a7bc6b278 Add new format string placeholders for walletnotify to include relevant block information for transactions (Maayan Keshet)
Pull request description:
This patch includes two new format placeholders for walletnotify:
%b - the hash of the block containting the transaction (zeroed if a mempool transaction)
%h - the height of the block containing the transaction (zero if a mempool transaction)
I've included test suite changes to check and validate the above functional requirements as well as doc/help description changes.
**Motivation**
The walletnotify option is used to be notified of new transactions relevant to the wallet of the node.
A common usage pattern is to perform afterwards additional RPC calls to determine:
1. If this is a mempool transaction or not (i.e. are there any confirmations?)
2. What block was it included in?
3. Did this transaction was seen before and is now seen again because of a fork?
All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer.
Please let me know of any questions and suggestions.
ACKs for top commit:
laanwj:
ACK 06e1fb0b170a69996a7ce1ef5203785a7bc6b278
Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775
6927933782acb9b158787e6f35debb916793f6b1 [net processing] Add ChainSyncTimeoutState default initializers (John Newbery)
55966e0cc03f0e380d21a9434b048d4d515b6729 [net processing] Remove CNodeState ctor body (John Newbery)
Pull request description:
This addresses the two outstanding review comments from #21370.
ACKs for top commit:
practicalswift:
cr ACK 6927933782acb9b158787e6f35debb916793f6b1: patch looks correct
hebasto:
ACK 6927933782acb9b158787e6f35debb916793f6b1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: b3ef5c8a096e447887df255406b3a760f01c73e2b942374595416b4b4031fc69b89cd93168c45040489d581f340b2a62d3fbabd207d4307f587c00a7a7daacd1
fa7ff0790ec21d187f1a32310918b0c8d66e5266 rpc: Properly document submitblock return value (MarcoFalke)
fae542c28b269d4a8a39f48ef829734b1241dd4f rpc: Properly document getblocktemplate return value (MarcoFalke)
fabaccf031cfac718bf05b140f1901d93ee8be67 rpc: Properly document scantxoutset return value (MarcoFalke)
faa2059547389e342991ab0d9382f8694f74fce1 rpc: Properly document gettxout return value (MarcoFalke)
Pull request description:
Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476
ACKs for top commit:
fjahr:
utACK fa7ff0790ec21d187f1a32310918b0c8d66e5266
amitiuttarwar:
tACK fa7ff0790e
Tree-SHA512: 933cb8f003163d93dbedb302d4c162514c2698ec6d58dbb9a053da8b8b9a4459b0701a3d9e830ecdabd7f278a46b7a07a3af49ec60703a80fcd75390877294ea
7c90c67b7e6f598f9ffdc136ded2b533b78ed044 rpc: refactor rpc wallet functions to take references instead of pointers (fanquake)
48669340080feaff86b8fc0403ef22c820477697 rpc: remove calls to CWallet.get() (fanquake)
Pull request description:
This is a rebased #18592.
> This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here https://github.com/bitcoin/bitcoin/issues/18590
> It seems that this PR is indirectly related to this issue: https://github.com/bitcoin/bitcoin/pull/13063#discussion_r186740049
> Notice: I have deliberately **not** changed the class `WalletRescanReserver ` whose constructor expects a raw pointer, because it's external and affects other areas, which I didn't touch to avoid making this PR "viral".
> Fixes https://github.com/bitcoin/bitcoin/issues/18590
ACKs for top commit:
MarcoFalke:
ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044 🐧
ryanofsky:
Code review ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044. Changes easy to review with `--word-diff-regex=. -U0`
Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba
bfc083e9b7 Merge #21574: Drop JSONRPCRequest constructors after #21366 (MarcoFalke)
c0cdb0488b Merge #21572: Fix wrong wallet RPC context set after #21366 (MarcoFalke)
2f7814acdd Merge #21035: Remove pointer cast in CRPCTable::dumpArgMap (MarcoFalke)
d3b1ef374c refactor: simplify implementation of RPC composite commands (Konstantin Akimov)
3270becc9b chore: add TODO to make client parsing for composite commands (Konstantin Akimov)
d55759fa79 Merge #20012: rpc: Remove duplicate name and argNames from CRPCCommand (Wladimir J. van der Laan)
1d87ce4e86 Merge #18531: rpc: remove deprecated CRPCCommand constructor (MarcoFalke)
a7e538d7ae fix: missing changes from bitcoin#19250 (Konstantin Akimov)
68c5da41dc feat: fix help message - all subcommands support it now! (Konstantin Akimov)
d3e181f516 fix: add missing client's argument parsing for RPC commands (Konstantin Akimov)
37bd4009c1 refactor: use monostate instead std::optional in CoreContext (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Backports from bitcoin v22 rpc command related
## What was done?
See commits for backports.
Also:
- refactored and significantly simplified implementation of composite commands
- added missing changes from bitcoin#19250
- fix help message for rpc `help` - all subcommands support it now
- add missing client's argument parsing for RPC commands
- CoreContext uses std::monostate instead nullopt which is best-practice for std::variant
## How Has This Been Tested?
Run unit/functional tests.
Checked autocomplete for various commands
Checked help for various commands
## 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 bfc083e9b7
UdjinM6:
utACK bfc083e9b7
Tree-SHA512: b7b586eac2f848a6808c677252a5965577bc783778fd70d3025b8d510113de2b177d423d72ea5f61ddd8905673bf3458e55810ada371ee235fbaa19de8d2d36f
69c37f4ec2 rpc: make sure `upgradetohd` always has the passphrase for `UpgradeToHD` (Kittywhiskers Van Gogh)
619b640a77 wallet: unify HD chain generation in CWallet (Kittywhiskers Van Gogh)
163d31861c wallet: unify HD chain generation in LegacyScriptPubKeyMan (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
When filming demo footage for https://github.com/dashpay/dash/pull/6093, I realized that if I tried to create an encrypted blank legacy wallet and run `upgradetohd [mnemonic]`, the client would crash.
```
dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash'
dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed.
Posix Signal: Aborted
No debug information available for stacktrace. You should add debug information and then run:
dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa===
```
The expected set of operations when performing privileged operations is to first use `walletpassphrase [passphrase] [time]` to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., `upgradetohd` imports **always** need the passphrase, if encrypted).
You might have noticed that I used `upgradetohd [mnemonic]` instead of the correct syntax, `upgradetohd [mnemonic] "" [passphrase]` that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told.
```
Error: Please enter the wallet passphrase with walletpassphrase first.
```
Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so `EnsureWalletIsUnlocked()` is happy, so now the following happens.
```
upgradetohd()
| Insufficient validation has allowed us to supply a blank passphrase
| for an encrypted wallet
|- CWallet::UpgradeToHD()
|- CWallet::GenerateNewHDChainEncrypted()
| We get our hands on vMasterKey by generating the key from our passphrase
| and using it to unlock vCryptedMasterKey.
|
| There's one small problem, we don't know if the output of CCrypter::Decrypt
| isn't just gibberish. Since we don't have a passphrase, whatever came from
| CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the
| vMasterKey we just got is gibberish
|- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain()
|- res = LegacyScriptPubKeyMan::EncryptHDChain()
| |- EncryptSecret()
| |- CCrypter::SetKey()
| This is where everything unravels, the gibberish key's size doesn't
| match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out.
|- assert(res)
We assume are inputs are safe so there's no real reason we should crash.
Except our inputs aren't safe, so we crash. Welp! :c
```
This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (19512988c6), trying to do the same thing would return you a vague error
```
Failed to generate encrypted HD wallet (code -4)
```
In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to `upgradetohd`, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash.
```
upgradetohd()
| We've been supplied a passphrase, so we can try and validate it by
| trying to unlock the wallet with it. If it fails, we know we got the
| wrong passphrase.
|- CWallet::Unlock()
| | Before we bother unlocking the wallet, we should check if we're
| | already unlocked, if we are, we can just say "unlock successful".
| |- CWallet::IsLocked()
| | Wallet is indeed unlocked.
| |- return true;
| The validation method we just tried to use has a bail-out mechanism
| that we don't account for, the "unlock" succeded so I guess we have the
| right passphrase.
[...] (continue call chain as mentioned earlier)
|- assert(res)
Oh...
```
This pull request aims to resolve crashes caused by the above two edge cases.
## Additional Information
As this PR was required me to add additional guardrails on `GenerateNewCryptedHDChain()` and `GenerateNewHDChainEncrypted()`, it was taken as an opportunity to resolve a TODO ([source](9456d0761d/src/wallet/wallet.cpp (L5028-L5038))). The following mitigations have been implemented.
* Validating `vMasterKey` size (any key not of `WALLET_CRYPTO_KEY_SIZE` size cannot be used for encryption and so, cannot be a valid key)
* Validating `secureWalletPassphrase`'s presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase)
* Using `Unlock()` to validate the correctness of `vMasterKey`. (the two other instances of iterating through `mapMasterKeys` use `Unlock()`, see [here](1394c41c8d/src/wallet/wallet.cpp (L5498-L5500)) and [here](1394c41c8d/src/wallet/wallet.cpp (L429-L431)))
* `Lock()`'ing the wallet before `Unlock()`'ing the wallet to avoid the `IsLocked()` bail-out condition and then restoring to the previous lock state afterwards.
* Add an `IsCrypted()` check to see if `upgradetohd`'s `walletpassphrase` is allowed to be empty.
## 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:
knst:
utACK 69c37f4ec2
UdjinM6:
utACK 69c37f4ec2
PastaPastaPasta:
utACK 69c37f4ec2
Tree-SHA512: 4bda1f7155511447d6672bbaa22b909f5e2fc7efd1fd8ae1c61e0cdbbf3f6c28f6e8c1a8fe2a270fdedff7279322c93bf0f8e01890aff556fb17288ef6907b3e
77915de40a test: run `Interrupt()` before `Stop()`, add additional sanity check (Kittywhiskers Van Gogh)
a376e9357a fix: init `g_txindex` only once, downgrade from assert to exception (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
`g_txindex` should be initialized in `TestChainSetup`'s constructor but when backporting [bitcoin#19806](6bf39d7632 (diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R289)) ([dash#5236](https://github.com/dashpay/dash/pull/5236)), portions of the constructor were split into `TestChainSetup::mineBlocks()`, `g_txindex`'s init was left behind in the latter instead of the former.
This meant that every `mineBlocks()` call would re-create a `TxIndex` instance, which is not intended behaviour; and was recorded to cause `heap-use-after-free`s ([comment](https://github.com/dashpay/dash/pull/6085#issuecomment-2228109300), also the reason this PR was opened).
This PR aims to resolve that.
## Additional Information
* Crashes stemming from previous attempts (except for one attempt) were not reproducible with my regular local setup (`depends` built with Clang 16, Dash Core built with Clang 16, set of debug-oriented flags, unit tests run using `./src/test/test_dash`).
* Attempting to rebuild Dash Core with GCC 9 was insufficient, required to rebuild depends with GCC 9 as well
* `configure`'d with `CC=gcc CXX=g++ CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --enable-crash-hooks --enable-c++20 --disable-ccache`
* Unit tests must be run with `make check-recursive -j$(( $(nproc --all) - 2 ))`
* An index must be initialized **after** the chain is constructed, this seems to be corroborated by all other index usage ([source](09239a17c7/src/test/blockfilter_index_tests.cpp (L141)), [source](09239a17c7/src/test/coinstatsindex_tests.cpp (L33)), [source](09239a17c7/src/test/txindex_tests.cpp (L31)), all three use `Start()` for their respective indexes _after_ `TestChain100Setup`'s constructor runs `mineBlocks()`)
* Attempting to run `Start()` earlier (_before_ the `mineBlocks()` call in the constructor) results in erratic behaviour
* This also explains why my attempt at moving it back to `TestingSetup` (a grandparent of `TestChainSetup`) failed
* `Interrupt()` is supposed to be called before `Stop()` but this was erroneously removed in a [commit](cc9dcdd0e0 (diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6L413-L419)) that adopted `IndexWaitSynced`. This has since been resolved.
* In line [with](09239a17c7/src/test/blockfilter_index_tests.cpp (L138-L139)) [other](09239a17c7/src/test/coinstatsindex_tests.cpp (L29-L31)) [indexes](09239a17c7/src/test/txindex_tests.cpp (L28-L29)), an sanity check has been added. Additionally, as `TxIndex::Start()` is more akin to `CChainState::LoadGenesisBlock()` than `CChainState::CanFlushToDisk()`, the `assert` has been downgraded to an exception.
## 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 **(note: N/A)**
- [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:
knst:
utACK 77915de40a
UdjinM6:
utACK 77915de40a
PastaPastaPasta:
utACK 77915de40a
Tree-SHA512: 5051dcf62b6cad4597044b4d27dc54c5bafa8692ba31e03c8afc0f7ef80b790a3873cf0275bb2cf6260939a11f95625da79fc7987951e66114900a86276c037c
`g_txindex` should be initialized in `TestChainSetup`'s constructor but
in bitcoin#19806 (dash#5236), when portions of the constructor were
split into `mineBlocks()`, `g_txindex`'s init was left behind in the
latter instead of the former. This meant that every `mineBlocks()` call
would re-create a `TxIndex` instance, which is not intended behaviour.
Also, a runtime exception is more appropriate and closer to the usage of
`BOOST_REQUIRE` in other index `Start()` calls than the harsher `assert`
earlier it was possible to make it all the way to `EncryptSecret`
without actually having the passphrase in hand until being told off
by `CCrypter::SetKey`, we should avoid that.
also, let's get rid of checks that `UpgradeToHD` is now taking
responsibility for. no point in checking if the wallet is unlocked
as it has no bearing on your ability to upgrade the wallet.
additionally, let's do the lock-unlock validation test *before* trying
to generate a new chain. as a bonus, it lets us make sure that we
encrypt the HD chain using a key that's actually capable of unlocking
the wallet.
also, `upgradetohd` never checked if the passphrase was handed to us
(which matters in encrypted blank wallets) before calling UpgradeToHD,
so let's check it for them.
most of the steps are overlapping except for the encryption sections,
which we can make contingent on supplying of the keying material. `res`
isn't used anywhere since we plan on hard crashing if they fail anyway,
so if we got that far, we've already succeeded.
we will validate vMasterKey's size before starting the encryption
routine to prevent a value outside of our control from hard-crashing
the client.
b7c7bff6e0 rpc: remove keypool replenishment stat and warning for descriptor wallet (Kittywhiskers Van Gogh)
Pull request description:
## Breaking Changes
- `getcoinjoininfo` will no longer report `keys_left` and will not incorrectly warn about keypool depletion with descriptor wallets
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [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 **(note: N/A)**
- [x] 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 b7c7bff6e0
knst:
utACK b7c7bff6e0
UdjinM6:
re-utACK b7c7bff6e0
PastaPastaPasta:
utACK b7c7bff6e0
Tree-SHA512: d6e55698a30288308e206566bafb7ea4cd465f37c3f6975410ff15b246500e576ee006da712d28eaa1d4845451f19b218e8c4d11f311b860ed674ad52dca1e0c
c72ec70fdf feat: implement governance RPCs votealias and votemany for descriptor wallets (Konstantin Akimov)
490832959d refactor: new method to generate a signing message in CGovernanceVote (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
RPCs `governance votemany` and `governance votealias` use 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)
```
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## What was done?
Use CWallet's interfaces instead LegacyScriptPubKeyMan
## How Has This Been Tested?
Functional tests `feature_governance.py` and `feature_governance_cl.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:
UdjinM6:
utACK c72ec70fdf
PastaPastaPasta:
utACK c72ec70fdf
Tree-SHA512: 2c18f0d4acb1c4d57da81bf54f0d155682f558eeb7271df7e6fe75c126ef7f047562794a6730e3ca5351abc4e2daded06b874c2ab77f9c47b840c89d8a158c9f
9044522ef76f880760165d98fab024802ccfc062 Drop JSONRPCRequest constructors after #21366 (Russell Yanofsky)
Pull request description:
This just makes an additional simplification after #21366 replaced
util::Ref with std::any. It was originally suggested
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351 but
delayed for a followup. It would have prevented usage bug
https://github.com/bitcoin/bitcoin/pull/21572.
ACKs for top commit:
promag:
ACK 9044522ef76f880760165d98fab024802ccfc062, fixed conflict in src/wallet/interfaces.cpp.
Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
937fd4a66f048780bffc5e714d0c800de987ce93 Fix wrong wallet RPC context set after #21366 (Russell Yanofsky)
Pull request description:
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882
This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking. Suggested
change
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351
eliminates these and would be a good followup for a future PR.
This PR just implements the simplest possible fix.
ACKs for top commit:
theStack:
Code-review ACK 937fd4a66f048780bffc5e714d0c800de987ce93
meshcollider:
Code review ACK 937fd4a66f048780bffc5e714d0c800de987ce93
Tree-SHA512: 53e6265ed6c7abb47d2b3e77d1604edfeb993c3a2440f0c19679cfeb23516965e6707ff486196a0acfbeff21c79a9a08b5cd33bae9a232d33d0134bca1bd0ff3
9048c58e10841d9e1d709c0a325dd14684cec325 Remove pointer cast in CRPCTable::dumpArgMap (Russell Yanofsky)
14f3d9b908ed9e78997bfaad3d8a06357a89d46e refactor: Add RPC server ExecuteCommands function (Russell Yanofsky)
6158a6d3978a18d5ac4166ca2f59056d8ef71c3d refactor: Replace JSONRPCRequest fHelp field with mode field (Russell Yanofsky)
Pull request description:
This change is needed to fix the `rpc_help.py` test failing in #10102: https://cirrus-ci.com/task/5469433013469184?command=ci#L2275
The [`CRPCTable::dumpArgMap`](16b784d953/src/rpc/server.cpp (L492)) method currently works by casting RPC `unique_id` integer field to a function pointer, and then calling it. The `unique_id` field wasn't supposed to be used this way (it's meant to be used to detect RPC aliases) and as a result, this code segfaults in the `rpc_help.py` test in multiprocess PR #10102 because wallet RPC functions aren't directly accessible from the node process.
Fix this by adding a new `GET_ARGS` RPC request mode to retrieve argument information similar to the way the `GET_HELP` mode retrieves help information.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
MarcoFalke:
re-ACK 9048c58e10841d9e1d709c0a325dd14684cec325 👑
Tree-SHA512: cd1a01c1daa5bde2c2455b63548371ee4cf39688313969ad2016d9a0fd4344102e3fd43034058f253364518e9632d57cf21abffad0d6a2c0c94b7a6921cbe615
fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204 rpc: Remove duplicate name and argNames from CRPCCommand (MarcoFalke)
fa92912b4bb4629addcbfdfb7cc000be701614af rpc: Use RPCHelpMan for check-rpc-mappings linter (MarcoFalke)
faf835680be39811827504f77005b6603165f53e rpc: [refactor] Use concise C++11 code in CRPCConvertTable constructor (MarcoFalke)
Pull request description:
Currently, the RPC argument names are specified twice to simplify consistency linting. To avoid having to specify the argnames twice when adding new arguments, remove the linter and add an equivalent test based on RPCHelpMan.
ACKs for top commit:
laanwj:
ACK fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204
Tree-SHA512: 3f5f32f5a09b22d879f24aa67031639d2612cff481d6aebc6cfe6fd757cafb3e7bf72120b30466f59292a260747b71e57322c189d5478b668519b9f32fcde31a
faaf9c58e4aa809019d4ca12747dd47411988e37 remove CRPCCommand constructor that takes rpcfn_type function pointer (MarcoFalke)
fa19bb2cd8c575593583138a84e6bb3444d6196d remove dead rpc code (MarcoFalke)
Pull request description:
Remove the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37
promag:
Tested ACK faaf9c58e4aa809019d4ca12747dd47411988e37.
ryanofsky:
Code review ACK faaf9c58e4aa809019d4ca12747dd47411988e37. Two obviously good simplifications.
Tree-SHA512: 5de3b440f7b2ed2c3e86655d4f0e2e5df9c67e8ce3c7817d5ea5311d1a38690f2f3e28fab41aad6936be9fc884326d037e5f19e85d4d2fe281474dada13911ee
2db69d7b81 chore: add release notes for "quorum platformsign" (Konstantin Akimov)
283c5f89a2 feat: create new composite command "quorum platformsign" (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
It splits from #6100
With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only.
## What was done?
Implemented a new composite command "quorum platformsign"
This composite command let to limit quorum type for signing for case of whitelist.
After that old way to limit platform commands can be deprecated - #6105
## How Has This Been Tested?
Updated a functional tests to use platform signing for Asset Unlocks feature.
## 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:
UdjinM6:
utACK 2db69d7b81
PastaPastaPasta:
utACK 2db69d7b81
Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb
85abbb97b4 chore: add release notes for composite command for whitelist (Konstantin Akimov)
78ad778bb0 feat: test composite commands in functional test for whitelist (Konstantin Akimov)
a102a59787 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/66https://github.com/dashpay/dash-issues/issues/65
## What was done?
Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see https://github.com/dashpay/dash/pull/6052https://github.com/dashpay/dash/pull/6051https://github.com/dashpay/dash/pull/6055 and other related PRs
This PR makes whitelist feature to be compatible with composite commands.
Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things.
Platform at their side can use config such as this one (created based on shumkov's example):
```
rpc: {
host: '127.0.0.1',
port: 9998,
users: [
{
user: 'dashmate',
password: 'rpcpassword',
whitelist: null,
lowPriority: false,
},
{
username: 'platform-dapi',
password: 'rpcpassword',
whitelist: [],
lowPriority: true,
},
{
username: 'platform-drive-consensus',
password: 'rpcpassword',
whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
lowPriority: false,
},
{
username: 'platform-drive-other',
password: 'rpcpassword',
whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
],
lowPriority: true,
},
],
allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'],
},
```
## How Has This Been Tested?
Updated functional tests, see commits
## 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:
UdjinM6:
LGTM, utACK 85abbb97b4
PastaPastaPasta:
utACK 85abbb97b4
Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b
89ade3e340 rpc: disallow `coinjoin stop` if there's no CoinJoin session to stop (Kittywhiskers Van Gogh)
51b6b94fc0 rpc: make `coinjoin` a composite command (Kittywhiskers Van Gogh)
Pull request description:
## Breaking Changes
- `coinjoin stop` will now return an error if there is no CoinJoin mixing session to stop
## 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 **(note: N/A)**
- [x] 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 89ade3e340
UdjinM6:
utACK 89ade3e340
PastaPastaPasta:
utACK 89ade3e340
Tree-SHA512: cde350217dd0ad15e5908eaae63773cad79ec6043e4ff902e2a32b57248ae3f3261e3180c7e39d5385eef361b2c14098b308feb648171c689fef5a3e8467381a
1840c9441a fix: drop extra pings - follow up for #18638 (UdjinM6)
264e7f9e62 Merge #18638: net: Use mockable time for ping/pong, add tests (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Split from https://github.com/dashpay/dash/pull/6102
## What was done?
So far as bitcoin#19499 is backported, bitcoin#18638 can be finished and removed related workarounds.
## 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:
UdjinM6:
utACK 1840c9441a
PastaPastaPasta:
utACK 1840c9441a
Tree-SHA512: f34657c14514d6ee596175b3faf9ee44e58e8b0339939a0708d58ab2d119786830c183f9b236ed87c08ef8e1dbd031a38fc596b5aa4d38e10521658df4330e79
d3e842f605 refactor: remove dead code which has no use since composite commands are refactored (Konstantin Akimov)
58c5d431fe fix: follow-up to #6017 - enable one more assert in wallet_descriptor test (Konstantin Akimov)
dbed4a31af fix: update comment for wallet_keypool_hd due to bitcoin#17681 DNM (Konstantin Akimov)
4741bcc5c3 chore: remove outdated todo - removed by bitcoin#16898 (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Multiple TODO is reviewed and fixes in this PR
## What was done?
See commits
## 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:
UdjinM6:
utACK d3e842f605
PastaPastaPasta:
utACK d3e842f605
Tree-SHA512: 25080abcce8fa3dbb4e4c71d321b24cb9d23c073e6caf75366be58623023af50d28bc1cdac4098eb78a87b4fe0f3c33d39bb06257f0590b3e42e5fcad161ea2d
a4e970adb6de8425025ae3f62fb89d9e27a8ab1f build: enable -Wdocumentation if suppressing external warnings (fanquake)
3b0078f958c46e94b468c829522ba965f5549f11 doc: fixup -Wdocumentation issues (fanquake)
c6edcf1c710e4aaf1cafdbf8e86fe209b57bdeb8 build: suppress libevent warnings if supressing external warnings (fanquake)
Pull request description:
Enable `-Wdocumentation` by taking advantage of our `--enable-suppress-external-warnings` flag. Most of the CIs are using this flag now, so any regressions should be caught.
This also required modifying libevents flags when suppressing warnings, as depending on the version being built against, that could generate a large number of warnings. i.e:
```bash
In file included from httpserver.cpp:34:
In file included from ./support/events.h:12:
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:464:11: warning: parameter 'req' not found in the function declaration [-Wdocumentation]
@param req a request object
^~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:465:11: warning: parameter 'databuf' not found in the function declaration [-Wdocumentation]
@param databuf the data chunk to send as part of the reply.
^~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:467:11: warning: parameter 'call' not found in the function declaration [-Wdocumentation]
@param call back's argument.
^~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:939:4: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync]
@deprecated This function is deprecated; you probably want to use
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:946:1: note: add a deprecation attribute to the declaration to silence this warning
char *evhttp_decode_uri(const char *uri);
^
__AVAILABILITY_INTERNAL_DEPRECATED
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:979:5: warning: declaration is marked with '@deprecated' command but does not have a deprecation attribute [-Wdocumentation-deprecated-sync]
@deprecated This function is deprecated as of Libevent 2.0.9. Use
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:987:1: note: add a deprecation attribute to the declaration to silence this warning
int evhttp_parse_query(const char *uri, struct evkeyvalq *headers);
^
__AVAILABILITY_INTERNAL_DEPRECATED
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: warning: parameter 'query_parse' not found in the function declaration [-Wdocumentation]
@param query_parse the query portion of the URI
^~~~~~~~~~~
/usr/local/Cellar/libevent/2.1.12/include/event2/http.h:1002:11: note: did you mean 'uri'?
@param query_parse the query portion of the URI
^~~~~~~~~~~
uri
69 warnings generated.
```
Note that a lot of these have already been fixed upstream.
ACKs for top commit:
laanwj:
Concept and code review ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f
practicalswift:
cr ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f: automatic compiler feedback comes sooner and is more reliable than manual reviewer feedback
jonatack:
Light ACK a4e970adb6de8425025ae3f62fb89d9e27a8ab1f skimmed the changes, clang 11 build is clean with the change, verified -Wdocumentation build warnings with this change when a doc fix was reverted
Tree-SHA512: 57a1e30cffcc8bcceee72d85f58ebe29eae525861c70acb237541bd480c51ede89875c033042c0af376fdbb49fb7f588ef9282a47c6e78f9d4501c41f1b21eb6
1a691bd100 feat: add logging for RPC HTTP requests: command, user, http-code, time of running (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Currently there is no way to gather stats for http rpc in dash core. This PR aim to change it.
## What was done?
Implemented some basic stats for each RPC request:
- rpc command
- flag "is external"
- http status
- time to serve query (rpc time, not http time)
## How Has This Been Tested?
See new logs:
```log
[httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=false status=200 elapsed_time_ms=0
[httpworker.2] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=quorum is_external=false status=500 elapsed_time_ms=0
[httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command= is_external=false status=401 elapsed_time_ms=0
[httpworker.3] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=platform-user command=getbestblockhash is_external=true status=200 elapsed_time_ms=28
[httpworker.0] [httprpc.cpp:100] [~RpcHttpRequest] -- HTTP RPC request handled: user=operator command=getbestblockhash is_external=false status=200 elapsed_time_ms=0
```
## Breaking Changes
N/A
It doesn't change behavior of rpc server and http server.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] 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 1a691bd100
Tree-SHA512: b62fceedb9a901e87c23c4aa6e6dfa7226d44da84a081ea245b40e7ff887103302147cebe0f7ff90bf9c8d4fa9ecafbaa6f25f39d2008f62c4f2beeef2877b57
3d8ff0cd2d fix: cppcheck warning about coinst in rpc/coinjoin (Konstantin Akimov)
63dfdd7d42 fix: workaround for buggy cppcheck in masternode/utils (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
`cppcheck 2.11` on my localhost returns 2 failures, that are not caught by our CI, probably due to difference in version:
```
src/rpc/coinjoin.cpp:73:28: warning: Variable 'chainman' can be declared as reference to const [constVariableReference]
src/masternode/utils.cpp:0:0: warning: Internal Error. MathLib::toLongNumber: input was not completely consumed: 5s [cppcheckError]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
```
## What was done?
Adds missing const, workaround for probably a crash in cppcheck 2.11.
## How Has This Been Tested?
Run `test/lint/lint-cppcheck-dash.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 3d8ff0cd2d
PastaPastaPasta:
utACK 3d8ff0cd2d
Tree-SHA512: c4c7e839161a6eb8d7a25b48cd05914d94dc4e4a6d53cbd39728a74a39d626d9b533afd98a9803550d2754df5c86445e5157e0e48853af52ea5ea5cb2e15d97b
c944908399 refactor: simplify implementation of function CWallet::IsLocked (Konstantin Akimov)
685cf34cb9 fix: unlock descriptor wallet for mixing-only (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
As [noticed by kwvg](https://github.com/dashpay/dash/pull/6090#pullrequestreview-2153639183), mixing doesn't work with descriptor wallets if do "unlock only for mixing". This PR is aiming to fix this issue.
https://github.com/dashpay/dash-issues/issues/59
## What was done?
Removed default argument "bool mixing = false" from WalletStorage interface,
Refactored a logic of CWallet::IsLocked to make it shorter, clearer and unified with bitcoin.
## How Has This Been Tested?
A. Run Dash-Qt with descriptor wallet, run mixing, enter passphrase.
The wallet is partially unlocked (for mixing only) - possible to see yellow lock in status. Mixing happens.
B. Open "send transaction dialog", try to send a transaction: the app asks password to unlock wallet as expected.
Though, I am not sure how exactly to test that **all** rpc are indeed locked when descriptor wallet is unlocked for mixing only.
## 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:
LGTM, ~utACK~ light ACK c944908399
kwvg:
ACK c944908399
PastaPastaPasta:
utACK c944908399
Tree-SHA512: 236c895dd75042449282b051e90781ace1c941a3b2c34bb29ddadb6e62ba9c8d57c2a677ed98847630ff7fb6df4e14d2b59f3473d8f299ec104afeeb8103930c
0133c9866d feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06769 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1e96 refactor: replace static private member method to static method (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.
This PR aims to fix this issue, as requested by shumkov for needs of platform:
> mailformed signature and can’t verify signature due to unknown quorum is the same error?
> possible to distingush ?
## What was done?
Return enum in CL verifying code `chainlock_handler.VerifyChainLock`.
The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}`
## How Has This Been Tested?
Functional test `feature_llmq_chainlocks.py` is updated
## Breaking Changes
`submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet.
## 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 0133c9866d
PastaPastaPasta:
utACK 0133c9866d
Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
60e36b786a fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta)
Pull request description:
## Issue being fixed or feature implemented
This default doesn't actually exist in code.
## What was done?
Remove default from text
## How Has This Been Tested?
## Breaking Changes
None
## 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 60e36b786a
Tree-SHA512: 658eb2cf101d0450619461b7ffe6069de5c04c1fdcca216713f9374cc1e60413ec9b96c3a2931fb69a4c2f8277dd6677100270960a58197da3b92dffb1e9e327
It helps to detect case of Tip is way too far behind: the signature can be
still valid but core can't verify it yet but later it may become a valid signature.
fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3 net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2fa1153c6d76efc8113fa01b06943ece util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c5fb57d7c0b5f2bce9fbbe290be93c3
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
It fixes this failure:
```
src/rpc/coinjoin.cpp:73:28: warning: Variable 'chainman' can be declared as reference to const [constVariableReference]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
```
It shows this error:
```
src/masternode/utils.cpp:0:0: warning: Internal Error. MathLib::toLongNumber: input was not completely consumed: 5s [cppcheckError]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in test/lint/lint-cppcheck-dash.sh
```
It doesn't seems as error in our code, but I don't like this warning
createwallet has changed list of arguments: createwallet "wallet_name" ( disable_private_keys blank "passphrase" avoid_reuse descriptors load_on_startup )
load_on_startup used to be an argument 5 but now has a number 6.
Both arguments 5 and 6 are boolean and it can confuse an user.
To prevent confusion if user is not aware about this breaking changes,
the RPC createwallet throws an exception if user trying to create descriptor wallet but has not mentioned load_on_startup.
This requirement can be removed when major amount of users updated to v21
bebea4b9b6 fix: auto backup issue with descriptor wallets for CJ (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Qt CoinJoin session has problems (https://github.com/dashpay/dash/pull/5579#pullrequestreview-1912946808):
- Autobackup problems
- False keypool depletion reporting
https://github.com/dashpay/dash-issues/issues/59
## What was done?
Disables check for "remaining keys left" and "auto-backups" for non-legacy wallet.
## How Has This Been Tested?
Run unit/functional test
Try to run CJ mixing for descriptor wallet.
## 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 bebea4b9b6
Tree-SHA512: 610551001d054c447ddca9451ac6d94f3d063ecf3ccfab437d99324efc5f99ff86e59d80a36f4ff4983d3c8107aa19292c021cb3210fcf51e79919387169c414
684503db6b feat: add a marker experimental for descriptor wallets (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Descriptor wallets pass most of functional tests now, but they are not really tested in-the-real-environment by multiple users.
## What was done?
Add a marker "experimental" for 'createwallet' rpc, for create wallet UI form.
## How Has This Been Tested?
Run and check
## 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 684503db6b
Tree-SHA512: e843e5b8080c7cd4273ea532038f3ec65e0260419b2c2a3db07ee1fadd856a7ce47f367705b4ddb19c5fc91cee5a529a54d580dbf53428c3e5e8b433c96ec0c9
375838378a feat: bump protocol version to distinguish v21 from v20.1 (pasta)
Pull request description:
## Issue being fixed or feature implemented
See commit
## What was done?
## How Has This Been Tested?
## Breaking Changes
None really; will require MNs to upgrade in order to not be banned
## 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)_
Top commit has no ACKs.
Tree-SHA512: c4a61e3c9a1ccda9b00a8982eaaff6ea7672bf59d9538342b0e45a16977554224b44b34db48d3355d28e2b26a01ab3133a5e0c38bb00a3d7a1410cfd1fcfc4ae
6c5246803d test: add tests for both current and future behaviour (UdjinM6)
4e86bda4dc feat: stricter bestCLHeightDiff checks (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Current `bestCLHeightDiff` checks are too relaxed
## What was done?
Make`bestCLHeightDiff` checks stricter
## How Has This Been Tested?
Run a node on mainnet/testet, run tests
## Breaking Changes
Old nodes aren't aware of this new logic so it's activated via `mn_rr` hardfork
## 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:
utACK 6c5246803d
Tree-SHA512: 2028d0ceb00a2270c92831ef38488d009d0bac47be4fc6a23ac93efdcf74847f1b9e99a529863fb4e14c65f120adda4e12c5b9e084d0f667d5f0fbaf80e3701d
8fb863008e refactor: inline sorting and make available through argument (Kittywhiskers Van Gogh)
3e0fcf471f refactor: move accessing CBlockTreeDB global out of Get*Index (Kittywhiskers Van Gogh)
ee9d11214e refactor: move pair value swapping out of CTxMemPool::getAddressIndex() (Kittywhiskers Van Gogh)
808842b1a3 refactor: define all key/value pairings as *Entry and use them instead (Kittywhiskers Van Gogh)
488f0474a8 rpc: extend error-on-failure to GetSpentIndex (Kittywhiskers Van Gogh)
9a6503d9e8 refactor: make CBlockTreeDB::Read*Index arguments const (Kittywhiskers Van Gogh)
625982e8d2 refactor: make CTxMemPool::get*Index functions and arguments const (Kittywhiskers Van Gogh)
5ad49ad668 refactor: move Get{Address*, Timestamp, Spent}Index out of validation (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
This pull request is motivated by [bitcoin#22371](https://github.com/bitcoin/bitcoin/pull/22371), which gets rid of the `pblocktree` global.
The sole usage of `pblocktree` introduced by Dash is for managing our {address, spent, timestamp} indexes with most of invocations within `BlockManager` or `CChainState`, granting them internal access to `pblocktree` (now `m_block_tree_db`). The sole exception being `Get*Index`, that relies on accessing the global and has no direct internal access.
This pull request aims to refactor code associated with `Get*Index` with the eventual aim of moving gaining access to the global out of the function. `Get*Index` is called exclusively called through RPC, which makes giving it access to `ChainstateManager` quite easy, which makes switching from the global to accessing it through `ChainstateManager` when backporting [bitcoin#22371](https://github.com/bitcoin/bitcoin/pull/22371) a drop-in replacement.
Alongside that, the surrounding code has been given some TLC:
* Moving code out of `validation.cpp` and into `rpc/index_util.cpp`. The code is exclusively used in RPC logic and doesn't aid in validation.
* Add lock annotations for accessing `pblocktree` (while already protected by `::cs_main`, said protection is currently not enforced but will be once moved into `BlockManager` in the backport)
* `const`-ing input arguments and using pass-by-value for input arguments that can be written inline (i.e. types like `CSpentIndexKey` are still pass-by-ref).
* While `const`ing `CTxMemPool` functions were possible (courtesy of the presence of `const_iterator`s), the same is currently not possible with `CBlockTreeDB` functions as the iterator is non-`const`.
* Extend error messages to `GetSpentIndex` to bring it line with other `Get*Index`es.
* Define key-value pairings as a `*Entry` typedef and replacing all explicit type constructions with it.
* Make `CTxMemPool::getAddressIndex` indifferent to how `CMempoolAddressDeltaKey` is constructed.
* Current behaviour is to accept a `std::pair<uint160, AddressType>` and construct the `CMempoolAddressDeltaKey` internally, this was presumably done to account for the return type of `getAddressesFromParams` in the sole call for the `CTxMemPool::getAddressIndex`.
* This has been changed, moving the construction into the RPC call.
* Moving {height, timestamp} sorting out of RPC and into the applicable `Get*Index` functions.
## 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 (note: N/A)
- [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 8fb863008e
UdjinM6:
utACK 8fb863008e
Tree-SHA512: 425a383e8284bbd74a5e9bcda4a9d7988221197055f43faa591e6f0d579625cee28f6a6046dab951e7afa0c3e33af1778fb4bb5f0a2e1e5792fe0d9396897a14
5b3406094f2679dfb3763de4414257268565b943 net_processing: Boost inv trickle rate (Anthony Towns)
228e9201efb5574b1b96bb924de1d2e8dd1317f3 txmempool: have CompareDepthAndScore sort missing txs first (Anthony Towns)
Pull request description:
Couple of performance improvements when draining the inventory-to-send queue:
* drop txs that have already been evicted from the mempool (or included in a block) immediately, rather than at the end of processing
* marginally increase outgoing trickle rate during spikes in tx volume
ACKs for top commit:
willcl-ark:
ACK 5b34060
instagibbs:
ACK 5b3406094f
darosior:
utACK 5b3406094f2679dfb3763de4414257268565b943
glozow:
code review ACK 5b3406094f2679dfb3763de4414257268565b943
dergoegge:
utACK 5b3406094f2679dfb3763de4414257268565b943
Tree-SHA512: 155cd3b5d150ba3417c1cd126f2be734497742e85358a19c9d365f4f97c555ff9e846405bbeada13c3575b3713c3a7eb2f780879a828cbbf032ad9a6e5416b30
b23d94b14f partial bitcoin#23706: getblockfrompeer followups (Kittywhiskers Van Gogh)
7e5cc5e375 merge bitcoin#23702: Add missing optional to getblockfrompeer (Kittywhiskers Van Gogh)
c294457b52 merge bitcoin#20295: getblockfrompeer (Kittywhiskers Van Gogh)
63ac87f011 merge bitcoin#22722: update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (Kittywhiskers Van Gogh)
07e4c2cdd0 merge bitcoin#21934: Include versionbits signalling details during LOCKED_IN (Kittywhiskers Van Gogh)
960e7687d4 merge bitcoin#19651: importdescriptors update existing (Kittywhiskers Van Gogh)
1f31823fed merge bitcoin#21910: remove redundant fOnlySafe argument (Kittywhiskers Van Gogh)
69c5aa8947 merge bitcoin#21359: include_unsafe option for fundrawtransaction (Kittywhiskers Van Gogh)
169dce7e50 merge bitcoin#20286: deprecate addresses and reqSigs from rpc outputs (Kittywhiskers Van Gogh)
7cddf70c58 merge bitcoin#20867: Support up to 20 keys for multisig under Segwit context (Kittywhiskers Van Gogh)
7c59923845 merge bitcoin#18344: Fix nit in getblockchaininfo (Kittywhiskers Van Gogh)
ec0803a0f5 refactor: align `TxToUniv` argument list with upstream (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Closes https://github.com/dashpay/dash/issues/6000
* `TxToUniv`'s argument list needed to be rearranged to match upstream as closely as possible (i.e. placing Dash-specific arguments at the end of the list to allow for code to be backported unmodified, relying on default arguments instead of having to modify each invocation to insert the default argument in between).
This was due to a new `TxToUniv` variant being introduced in [bitcoin#20286](https://github.com/bitcoin/bitcoin/pull/20286)
* The maximum number of public keys in a multisig remains the same. The upper limit for bare multisigs is and always has been `MAX_PUBKEYS_PER_MULTISIG` ([source](19512988c6/src/script/interpreter.cpp (L1143-L1144))), which has always been 20 ([source](19512988c6/src/script/script.h (L28-L29))). The limit of up to 16 comes from P2SH overhead ([source](19512988c6/src/script/descriptor.cpp (L877-L880))) and that hasn't changed.
In effect, what [bitcoin#20867](https://github.com/bitcoin/bitcoin/pull/20867) does to Dash Core is change the error from "too many public keys" (as we'll be testing against the bare multisig limit) to "excessive redeemScript size" ([source](19512988c6/src/rpc/util.cpp (L223-L225))) (which is the _true_ limitation).
* Backporting [bitcoin#21934](https://github.com/bitcoin/bitcoin/pull/21934) required a minor change in the condition needed to emit `activation_height` as `has_signal` in the preceding block will evaluate true for both `STARTED` and `LOCKED_IN` while earlier it would only for `STARTED`.
* In [bitcoin#22722](https://github.com/bitcoin/bitcoin/pull/22722), a `self.stop_node` had to be added to account for the absurd fee warning that a new test condition introduces.
* [bitcoin#23706](https://github.com/bitcoin/bitcoin/pull/23706) is partial due to commits in it that rely on RPC type enforcement, which is currently not implemented in Dash Core.
* `feature_dip3_deterministicmns.py` depends on the reporting of `addresses` to validate multisigs containing expected payees. There is no replacement RPC call to report the pubkeys that compose a multisig address. In the interm, the `-deprecatedrpc=addresses` flag has been enabled to allow the test to run otherwise unmodified.
## Breaking Changes
* The following RPCs: `gettxout`, `getrawtransaction`, `decoderawtransaction`, `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`, `/rest/block` deprecated the following fields (which are no longer returned in the responses by default): `addresses`, `reqSigs`.
The `-deprecatedrpc=addresses` flag must be passed for these fields to be included in the RPC response. Note that these fields are attributes of the `scriptPubKey` object returned in the RPC response. However, in the response of `decodescript` these fields are top-level attributes, and included again as attributes of the `scriptPubKey` object.
* When creating a hex-encoded Dash transaction using the `dash-tx` utility with the `-json` option set, the following fields: `addresses`, `reqSigs` are no longer returned in the tx output of the response.
* The error message for attempts at making multisigs with >16 pubkeys will change to an "excessive redeemScript size" instead of the previous "too many public keys".
## 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
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK b23d94b14f
PastaPastaPasta:
utACK b23d94b14f
Tree-SHA512: 659d9ba3a0a9f8594b307a7056ab172309d5a0d9efec605bc4b345f7e0fb1032ad303af9e8f51dbd6549e82d0b738ad41eae8a5b3aebf59081f39df0d4b5ec7c
91aae7b2fe refactor: use properly RPCHelpMan for governance RPC getsuperblockbudget (Konstantin Akimov)
e92e5ef526 refactor: use properly RPCHelpMan for governance RPC voteraw (Konstantin Akimov)
87b3011e55 refactor: duplicated code between governance RPC list and diff (Konstantin Akimov)
faa61795fb docs: move comments from the old code to the new implementation of governance RPC (Konstantin Akimov)
39cd5e41b1 fix: format code for rpc governance (Konstantin Akimov)
006242114f refactor: use new style of composite commands for RPC gobject NNN (Konstantin Akimov)
42e0b37cb2 refactor: use properly RPCHelpMan for governance RPC getgovernanceinfo (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
See #6051
## What was done?
Commands starting from 'governance ...' uses new a new way to make composite commands.
This PR includes also a refactoring to remove common code between `gobject list` and `gobject diff`
This PR includes also a refactoring to properly use RPCHelpMan for remaining governance's RPC.
## 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
- [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 91aae7b2fe
PastaPastaPasta:
utACK 91aae7b2fe
Tree-SHA512: 46c00df924a7d2c5af799c605bfc479034019eed346f9d487827b0688993aa712ba3dc528d9bdd09b64f0e07b6940078337ca1baf0a1355f87eb792406c676ca
7b44af26ca refactor: move common code inside protx_register_common_wrapper (Konstantin Akimov)
89d5e26473 refactor: replace bunch of bool flags to the enum (Konstantin Akimov)
1351bc9aad refactor: move common code to the wrapper (Konstantin Akimov)
8d4c28a983 refactor: remove unused protx code from old implementation (Konstantin Akimov)
2a837f8243 refactor: use new composite for RPC 'protx NNN' (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
See #6051
## What was done?
Commands starting from 'protx ...' uses new a new way to make composite commands.
## 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
- [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 7b44af26ca
Tree-SHA512: a5124121de93ba566bfa6c7c58a5217446301dd027839d137e056df00388282af6be0feb0b81ea0217310e12231a68e3b405fa82b66df93a3fb159d341d7a199
adba60924c addrman: allow for silent overwriting of inconsistent peers.dat (Kittywhiskers Van Gogh)
fbb2b51d75 merge bitcoin#26909: prevent peers.dat corruptions by only serializing once (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
[bitcoin#22762](https://github.com/bitcoin/bitcoin/pull/22762) (backported as part of [dash#6043](https://github.com/dashpay/dash/pull/6043)) did away with then-existing behaviour of overwriting `peers.dat` silently if found corrupt with the rationale of preventing situations where the wrong file is pointed at or the file is written by a higher version of Core. Alongside a change in behaviour, refactoring also took place and further changes were built on top of them.
Since then, there have been reports of an increasing number of "Corrupt data. Consistency check failed with code -5: iostream error" errors from builds based on `develop`. Reverting the pull request that introduced this change in behaviour is non-trivial due to the number of backports that build on top of the refactoring brought along with it.
Nor were any other error messages found except for the one mentioned above. The tendency for `peers.dat` to corrupt itself has also been documented upstream ([bitcoin#26599](https://github.com/bitcoin/bitcoin/issues/26599)), with the issue marked as closed with the merger of [bitcoin#26909](https://github.com/bitcoin/bitcoin/pull/26909).
Therefore, to remedy the above problem, alongside backporting [bitcoin#26909](https://github.com/bitcoin/bitcoin/pull/26909), to avoid inconvenience, instead of reverting all progress made from backporting (as the benefits of not overwriting `peers.dat` for having the wrong magic altogether, for example, is something that doesn't need to be reverted), only inconsistent `peers.dat` files will be overwritten and the action logged with no user intervention required.
## 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
- [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 adba60924c
knst:
utACK adba60924c
PastaPastaPasta:
utACK adba60924c
Tree-SHA512: 3e09e7a77c82cce333fe9f02f137485e362f7816c450aef3d18950b9fd57276b4a21cbd1fe90b3eefd62ede0947970ed367c5917930a0656833bc38c0629e408
cc55ebbf93 merge bitcoin#22093: Try posix-specific CXX first for mingw32 host (Kittywhiskers Van Gogh)
638806b2cc merge bitcoin#22088: improve note on choosing posix mingw32 (Kittywhiskers Van Gogh)
7ad0141d66 merge bitcoin#23947: use host_os instead of TARGET_OS in configure output (Kittywhiskers Van Gogh)
9126006c18 merge bitcoin#20201: pkg-config related cleanup (Kittywhiskers Van Gogh)
77862d8f5f merge bitcoin#23494: minor boost tidyups (Kittywhiskers Van Gogh)
ef4b35060d merge bitcoin#23269: remove redundant warning flags (Kittywhiskers Van Gogh)
183d08f1d9 merge bitcoin#22133: Make QWindowsVistaStylePlugin available again (Kittywhiskers Van Gogh)
1fbdd009cd merge bitcoin#21430: Add -Werror=implicit-fallthrough compile flag (Kittywhiskers Van Gogh)
cae5496d0b merge bitcoin#21920: improve macro for testing -latomic requirement (Kittywhiskers Van Gogh)
78db324970 partial bitcoin#20938: fix linking against -latomic when building for riscv (Kittywhiskers Van Gogh)
972b4198d7 merge bitcoin#23007: remove WSL install instructions and point to upstream (Kittywhiskers Van Gogh)
54be58b494 merge bitcoin#22845: improve check for ::(w)system (Kittywhiskers Van Gogh)
5b86009d40 merge bitcoin#22464: fix 32-bit narrowing warning in bench/peer_eviction.cpp (Kittywhiskers Van Gogh)
a42202d86f merge bitcoin#22433: remove straggling boost thread_group related code (Kittywhiskers Van Gogh)
abdf4d7b9f build: use enough padding to match with labels in options printout (Kittywhiskers Van Gogh)
e22f4216cb doc: clean up `build-windows.md` (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 **(note: N/A)**
- [x] 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:
utACK [cc55ebb](cc55ebbf93)
Tree-SHA512: f4cf6506120facabd5fc2e968c1b14a1fca02cf5858f63cce9e9743aa5327b18a7855b2584829eb568d6664a8e12c4bbfdda2f954ea44e29aaaba1776b3d1535
In bitcoin#20286, a new `TxToUniv` will be defined and in order to
minimize upstream deviation caused by having to change function calls
to account for `const CSpentIndexTxInfo*` being placed _before_
`const CTxUndo*` (requiring us to therefore specify `nullptr` as those
calls do not expect spend information) instead of letting the default
value remain. To allow for that, their ordering will be swapped.
To prevent a confusion with the last two arguments between the
`core_io` definition and the `rpc/blockchain` definition, let's do the
inversion here too before the backport.
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
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
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
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
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
