764b3a3239 test: disable mocktime in p2p_eviction.py (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
No idea why CI has no issues but `p2p_eviction.py` fails locally after #6103 (my guess is that it's because P2PInterface can't work with mocktime properly).
## What was done?
Disable mocktime in `p2p_eviction.py`
## How Has This Been Tested?
Run tests locally
## 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)_
Top commit has no ACKs.
Tree-SHA512: a9be9032c7697ff47b2256395f0fb126deeccd9bee6f101a71a1f88e1f25b08fa039ed5eb4cd4b1b308e8136d64510a544b7019ed9147ea2e80f8cb83ff25412
fa5362a9a0c5665c1a4de51c3ce4758c93a9449e rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs (MarcoFalke)
Pull request description:
Wallet RPCs that allow a rescan based on block-timestamp or block-height
need to sync with the active chain first, because the user might assume
the wallet is up-to-date with the latest block they got reported via a
blockchain RPC.
ACKs for top commit:
meshcollider:
utACK fa5362a9a0c5665c1a4de51c3ce4758c93a9449e
Tree-SHA512: d4831f1f08f854f9a49fc969de86c438f856e41c2163c801a6ff36dc2f6299cb342b44663279c524a8b7ca9a50895db1243cd7d49bed79277ada857213f20a26
7e32fde912b3924fdb27ec1f658ac11fcf160b3e test: feature_cltv.py: don't return tx copies in modification functions (Sebastian Falbesoner)
9ab2ce0a6673acc7ee0f85158fc087fce0fc7dd8 test: drop unused node parameters in feature_cltv.py (Sebastian Falbesoner)
0c2139a3f160d1d443460e4c5928109a6ab8cd11 test: fix typo in feature_cltv.py (s/ctlv/cltv/) (Sebastian Falbesoner)
Pull request description:
This tiny PR cleans up the test `feature_cltv.py` in the following ways:
* fixes a typo (s/ctlv/cltv/); compared to CHECKLOCKTIMEVERIFY, CHECKTIMELOCKVERIFY probably also sounds good and you [even get some search results for it](https://www.google.com/search?q=%22CHECKTIMELOCKVERIFY%22), but it's still wrong ;)
* drops the unused "node" parameters from the tx modification functions
* don't return a copy from the tx modification functions; it's modified in-place, hence a copy is not needed and `cltv_validate(tx, ...)` looks more natural than `tx = cltv_validate(tx, ...)`
ACKs for top commit:
MarcoFalke:
review ACK 7e32fde912b3924fdb27ec1f658ac11fcf160b3e 📼
Tree-SHA512: d2e6230977442f6a511d0f7c99431a44ad3a423647f4f327ce2ce8efe78bf9616c0d2093f5e3c3550f690dcb3f625ddf53227505c01ced70227425f249c25364
0d9fdd329e81cb171d687042290f4e6b1507d7f4 test, doc: refer to the correct variable names in p2p_invalid_tx.py (aitorjs)
Pull request description:
_tx_orphan_no_fee_ and _tx_orphan_invalid_ don't exist as transactions.
Have been replaced by _tx_orphan_2_no_fee_ and _tx_orphan_2_invalid_ respectively.
**Motivation**: Comments are more accurate and easy understandable under the tests context (I think).
ACKs for top commit:
kristapsk:
utACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4
theStack:
ACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4 📃
Tree-SHA512: a4cafd931e51fe2a67085e10e9c61178c864c14982664d112b76327e040af08cd1de04eca4a8ae980fad57ba7078017ce02fc60e7658f38380e8172c2ae28b77
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
b01cd9471f435bb36b8ed5211a56baad51111ad2 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc19814743cb12960a99793197c811e2750a06b test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c4826529a2d30ffc850bce4d44da6019b test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202c4820b1ad5c375d3d671fd0a18e092 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)
Pull request description:
The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:
f8462a6d27/test/functional/feature_cltv.py (L26-L35)
This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
* _the stack is empty_
➡️ prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
* _the top item on the stack is less than 0_
➡️ prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
* _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
* _the top stack item is greater than the transaction's nLockTime field_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
* _the nSequence field of the txin is 0xffffffff_
➡️ prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500
The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").
ACKs for top commit:
MarcoFalke:
review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣
Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
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
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
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.
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
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
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
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
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
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
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
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
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
c9a600e0fa fix: linkage error - message signer better to be common code rather than libconsensus (Konstantin Akimov)
8299b3b369 feat: protxregistar implementation for descriptor wallets (Konstantin Akimov)
6f45432f76 refactor: removed unused SignSpecialTxPayloadByString (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
RPC `protx updateregistar` uses forcely LegacyScriptPubKeyMan instead using CWallet's interface.
It causes a failures such as
```
test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
```
## What was done?
New method `SignSpecialTxPayloadByHash` is implemented in interface instead exporting raw private key for some address.
See https://github.com/dashpay/dash-issues/issues/59 to track progress
## How Has This Been Tested?
Functional test `feature_dip3_deterministicmns.py` to run by both ways - legacy and descriptor wallets.
Run unit and functional tests.
Extra test done locally:
```diff
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
if self.options.descriptors is None:
# Prefer BDB unless it isn't available
- if self.is_bdb_compiled():
- self.options.descriptors = False
- elif self.is_sqlite_compiled():
+ if self.is_sqlite_compiled():
self.options.descriptors = True
+ elif self.is_bdb_compiled():
+ self.options.descriptors = False
```
to flip flag descriptor wallets/legacy wallets for all functional tests.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK c9a600e0fa
Tree-SHA512: 00aea1cd9db3537b9a9dcdee096d47ea48337edeea3f52ad54aea91781678b8641ac2dd86b67f61f87e3912945bcb5361a42a3279b6c08bb8d9f096bed8fe842
3971613285 feat: functional tests for RPC getgovernanceinfo (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/63
## What was done?
It adds functional test for `getgovernanceinfo`
## How Has This Been Tested?
Run unit/functional tests
Check output of `test/functional/test_runner.py -j20 --previous-releases --coverage --extended`
```
Uncovered RPC commands:
- cleardiscouraged
- debug
- getblockheaders
- getmerkleblocks
- getpoolinfo
- voteraw
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 3971613285
Tree-SHA512: f2d93db9068a707f2b33dd841b14f266db226fc5e907f19a49cf7a856b840f0c2c1f521c462996888933bb0ed636c288db12b1d36978c124cb536ea1ab9f3892
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
faa137eb9eac5554504b062a6dc865ca87fd572b test: Speed up rpc_blockchain.py by removing miniwallet.generate() (MarcoFalke)
fa1fe80c757df0adcbfaf41b5c5c8a468bc07b6f test: Change address type from P2PKH to P2WSH in rpc_blockchain (MarcoFalke)
fa4d8f3169e38cbdbae20258efebe7070c49f522 test: Cache 25 mature coins for ADDRESS_BCRT1_P2WSH_OP_TRUE (MarcoFalke)
fad25153f5c8e88f72cf666b16b0b0dbdc45d3b1 test: Remove unused bug workaround (MarcoFalke)
faabce7d07c5776e4116b1a7ad1f6c408a4a4e46 test: Start only the number of nodes that are needed (MarcoFalke)
Pull request description:
Speed up various tests:
* Remove unused nodes, which only consume time on start/stop
* Remove unused "bug workarounds"
* Remove the need for `miniwallet.generate()` by adding `miniwallet.scan_blocks()`. (On my system, with valgrind, generating 105 blocks takes 3.31 seconds. Rescanning 5 blocks takes 0.11 seconds.)
ACKs for top commit:
laanwj:
Code review ACK faa137eb9eac5554504b062a6dc865ca87fd572b
Tree-SHA512: ead1988d5aaa748ef9f8520af1e0bf812cf1d72e281ad22fbd172b7306d850053040526f8adbcec0b9a971c697a0ee7ee8962684644d65b791663eedd505a025
