886f1731bec4393dd342403ac34069a3a4f95eea Key pool: Fix omitted pre-split count in GetKeyPoolSize (Andrew Chow)
386a994b853bc5b3a2ed0d812673465b8ffa4849 Key pool: Change ReturnDestination interface to take address instead of key (Andrew Chow)
ba41aa4969169cd73d6b4f57444ed7d8d875de10 Key pool: Move LearnRelated and GetDestination calls (Andrew Chow)
65833a74076cddf986037c6eb3b29a9b9dbe31c5 Add OutputType and CPubKey parameters to KeepDestination (Andrew Chow)
9fcf8ce7ae02bf170b9bf0c2887fd709d752cbf7 Rename Keep/ReturnKey to Keep/ReturnDestination and remove the wrapper (Andrew Chow)
596f6460f9fd8273665c8754ccd673d93a4f25f0 Key pool: Move CanGetAddresses call (Andrew Chow)
Pull request description:
* The `pwallet->CanGetAddresses()` call in `ReserveDestination::GetReservedDestination` to `LegacyScriptPubKeyMan::GetReservedDestination` so that the sanity check results in a failure when a `ScriptPubKeyMan` individually cannot get a destination, not when any of the `ScriptPubKeyMan`s can't.
* `ScriptPubKeyMan::GetReservedDestination` is changed to return the destination so that future `ScriptPubKeyMan`s can return destinations constructed in other ways. This is implemented for `LegacyScriptPubKeyMan` by moving key-to-destination code from `CWallet` to `LegacyScriptPubKeyMan`
* In order for `ScriptPubKeyMan` to be generic and work with future `ScriptPubKeyMan`s, `ScriptPubKeyMan::ReturnDestination` is changed to take a `CTxDestination` instead of a `CPubKey`. Since `LegacyScriptPubKeyMan` still deals with keys internally, a new map `m_reserved_key_to_index` is added in order to track the keypool indexes that have been reserved.
* A bug is fixed in how the total keypool size is calculated as it was omitting `set_pre_split_keypool` which is a bug.
Split from #17261
ACKs for top commit:
ryanofsky:
Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea. Only change is moving earlier fix to a better commit (same end result).
promag:
Code review ACK 886f1731bec4393dd342403ac34069a3a4f95eea.
instagibbs:
code review re-ACK 886f1731be
Sjors:
Code review re-ACK 886f1731bec4393dd342403ac34069a3a4f95eea
Tree-SHA512: f4be290759f63fdc920d5c02bd0d09acc4b06a5f053787d4afcd3c921b2e35d2bd97617fadae015da853dc189f559fb8d2c6e58d53e4cabfac9af151cd97ad19
1a3a256d5e0443d19757c1f1fceb9c9ede17758a wallet: replace raw pointer with const reference in AddrToPubKey (Harris)
Pull request description:
This PR replaces a redundant reference-to-pointer conversion in **addmultisigaddress** from *wallet/rpcwallet.cpp*. It also makes the API from *rpc/util.h* look more straightforward as **AddrToPubKey** now uses const references like other functions from there.
I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".
The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001
There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.
Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140
Regards,
ACKs for top commit:
achow101:
ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a
meshcollider:
utACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a
promag:
Code review ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a.
hebasto:
ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 1a2b8ddab5694ef4c65fac69f011e38dd03a634e84a35857e13bd05ad99fe42af22ee0af6230865e3d2c725693512f3336acb055ede19c958424283e7a3856c4
3958295bc8a3787b66b0269190218a3764088f79 wallet: LearnRelatedScripts only if KeepDestination (João Barbosa)
55295fba4cbff36e9a8c3fed9c38e82ebe3c48b7 wallet: Lock address type in ReserveDestination (João Barbosa)
Pull request description:
Only mutates the wallet if the reserved key is kept.
First commit is a refactor that makes the address type a class member.
The second commit moves `LearnRelatedScripts` from `GetReservedDestination` to `KeepDestination` to avoid an unnecessary call to `AddCScript` - which in turn prevents multiple entries of the same script in the wallet DB.
ACKs for top commit:
achow101:
Re-ACK 3958295bc8a3787b66b0269190218a3764088f79
Sjors:
ACK 3958295bc8a3787b66b0269190218a3764088f79
ryanofsky:
Code review ACK 3958295bc8a3787b66b0269190218a3764088f79. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I'd like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.)
meshcollider:
utACK 3958295bc8a3787b66b0269190218a3764088f79
Tree-SHA512: 49a5f4b022b28042ad37ea309b28378a3983cb904e234a25795b5a360356652e0f8e60f15e3e64d85094ea63af9be01812d90ccfc08ca4f1dd927fdd8566e33f
d0dab897afaac0a18aa47d3ce673a4a43a69178a Refactor: Require scriptPubKey to get wallet SigningProvider (Andrew Chow)
4b0c718f8f48c678cbe4575e9a9cf9e62a30f0da Accumulate result UniValue in SignTransaction (Andrew Chow)
Pull request description:
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior. It passes new CScript arguments to
signing functions, but the arguments aren't currently used.
Split from #17261
ACKs for top commit:
instagibbs:
utACK d0dab897af
ryanofsky:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a. Thanks for the SignTransaction update. No other changes since last review
Sjors:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a
promag:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a.
meshcollider:
Code review ACK d0dab897afaac0a18aa47d3ce673a4a43a69178a
Tree-SHA512: c3f52df20fd9d6b3b5aa65562cf5f7dce7b7f44c148b0f988f8b578fce2a28e9b7bf010f5f04bb5bf60f5272b2899f1dbbfb8aee81579c21c9cba559d1d2bb70
0b75a7f0680d16a41043864a897470324917b1e8 wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd00eb032a19d142026e4d019944192da19 wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)
Pull request description:
This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
- 1st the recursive lock is removed and now it requires the lock to be held;
- 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.
ACKs for top commit:
achow101:
ACK 0b75a7f0680d16a41043864a897470324917b1e8
MarcoFalke:
ACK 0b75a7f0680d16a41043864a897470324917b1e8
ryanofsky:
Code review ACK 0b75a7f0680d16a41043864a897470324917b1e8. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?
Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15
fab558612278909df93bdf88f5727b04f13aef0f doc: Use precise permission flags where possible (MarcoFalke)
Pull request description:
Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour.
This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated.
Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help.
ACKs for top commit:
jnewbery:
reACK fab558612278909df93bdf88f5727b04f13aef0f
fjahr:
Code review ACK fab558612278909df93bdf88f5727b04f13aef0f
jonatack:
ACK fab558612278909df93bdf88f5727b04f13aef0f
Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
8a2a652e6fab5eb8224beefcc07d9011b61865a8 Remove redundant type information from rpc docs (David O'Callaghan)
Pull request description:
Simple edit of the RPC calls to remove redundant text ("A json object/array ...") from the beginning of help.
Fixes: #18258
Top commit has no ACKs.
Tree-SHA512: cbbf760e0b7b4eda61c40b420ed77f5d878318e37b0eb13e63567212240b2c4ecc15d84030e98075e21c9ae9016539adfd201e5661ea824166a76d335180c32f
## Issue being fixed or feature implemented
Added the missing rpc in `protx` help.
Release notes can be found in https://github.com/dashpay/dash/pull/5137
## 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
- [x] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
Co-authored-by: thephez <thephez@users.noreply.github.com>
## Issue being fixed or feature implemented
## What was done?
<!--- Describe your changes in detail -->
- `bls generate` and `bls fromsecret` rpcs will return `scheme` used to
serialise the public key. Valid returned values are `legacy` and`basic`.
- `bls generate` and `bls fromsecret`rpcs accept an incoming optional
boolean argument `legacy` that enforces the use of legacy BLS scheme for
the serialisation of the reply even if v19 is active.
## 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
- [x] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
Platform and research team have requested this change.
## What was done?
`quorum info` was updated with the introduction of new field
`previousConsecutiveDKGFailures` that be returned only for rotated
LLMQs.
This field will hold the number of previously consecutive failed DGKs
for the corresponding quorumIndex before the currently active one.
Note: If no previously commitments were found then 0 will be returned
for `previousConsecutiveDKGFailures`.
Example:
- DKG `A` was successful
- DKG `B` failed
- DKG `C` failed
- DKG `D` was successful
- DKG `E` was successful
- `previousConsecutiveDKGFailures` = 0 when requesting for quorum `A`
(because `A` is the first ever created quorum for that quorumIndex)
- `previousConsecutiveDKGFailures` = 2 when requesting for quorum `D`
- `previousConsecutiveDKGFailures` = 0 when requesting for quorum `E`
## 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
- [x] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
Co-authored-by: thephez <thephez@users.noreply.github.com>
