b35e74ba379bdc12ea6d49a45469f0d6aa74cc27 wallet, refactor: Remove duplicate map lookups in GetAddressBalances (João Barbosa)
Pull request description:
Now just one lookup in `balances` instead of three.
ACKs for top commit:
achow101:
ACK b35e74ba379bdc12ea6d49a45469f0d6aa74cc27
theStack:
ACK b35e74ba379bdc12ea6d49a45469f0d6aa74cc27
practicalswift:
ACK b35e74ba379bdc12ea6d49a45469f0d6aa74cc27
Tree-SHA512: a73c1b336406a569e3bb10290618c5950b944db58ed0b05ff202d097684bb3ba3a5942c8d30443960052aa16438c054e2d02977b67aa901cce665c4df0ee5602
4f9d9efb4e9afb1b012fca689ce77eb1d8d34f7d qt: Remove needless headers (Hennadii Stepanov)
Pull request description:
No symbols from the removed headers are used in the `qt/walletview.cpp`.
This is a small followup of https://github.com/bitcoin/bitcoin/pull/18027.
Top commit has no ACKs.
Tree-SHA512: 986ed5c8f3bac4c0053736ce84d738f8593d3dbf713109af3cb9b7051cd838f23152a39bb3c1e9694a993c4e7accf14e94e5beff5e7881155638cd44fbf7f46f
fa193c6b1b7da8f72a399bfddb1497655ce1685c Add missing includes to fix compile errors (MarcoFalke)
fa09ec83f3f23dacb807c6b6393cabf2a984e4ff Remove unused variables (MarcoFalke)
Pull request description:
This is required for #19183, but seems like good cleanup that can go in upfront.
ACKs for top commit:
practicalswift:
ACK fa193c6b1b7da8f72a399bfddb1497655ce1685c -- patch looks correct
hebasto:
ACK fa193c6b1b7da8f72a399bfddb1497655ce1685c, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 79b94e7f7ee3a1a8a8fb2ea1ecdf61f130f8b133a37865894da3dbbbf311979e7d1fc013b923fdd7dbf19a221e0232f664defbdb57aa44e0b8c45bfff3c71dcb
1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner)
Pull request description:
The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters.
However, the real reason of adding those flags (introduced with commit 37c6389c5a by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash.
According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165):
> the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :)
For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html).
The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix.
ACKs for top commit:
meshcollider:
utACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
laanwj:
Concept and code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
jkczyz:
ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
MarcoFalke:
ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
fjahr:
Code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8
Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
69ffddc83e0f3e265bf6cf7ae31489ae629fe6be refactor: Remove unused methods CBloomFilter::reset()/clear() (Sebastian Falbesoner)
Pull request description:
The method `CBloomFilter::reset()` was introduced by commit d2d7ee0e86 in 2015, but was never ever used, as far as I could find. As discovered by MarcoFalke, the method `clear()` is also unused outside of unit tests and is hence also removed.
ACKs for top commit:
MarcoFalke:
re-ACK 69ffddc83e0f3e265bf6cf7ae31489ae629fe6be
jonatack:
ACK 69ffddc83e0f3e2, code review, compiled a fuzz build and started the bloom_filter fuzz test as a sanity check.
promag:
ACK 69ffddc83e0f3e265bf6cf7ae31489ae629fe6be.
Tree-SHA512: 6c53678545ad8e2fa1ffc0a8838e450462f26748a60632f738dc020f0eb494ae2c32841e6256e266ed9140177257a78b707123421942f3819a14ffcb9a99322f
09502452bbbe21bb974f1de8cf53196373921ab9 IsUsedDestination should count any known single-key address (Gregory Sanders)
Pull request description:
This plugs the privacy leak detailed at https://github.com/bitcoin/bitcoin/issues/17605, at least for the single-key case.
ACKs for top commit:
meshcollider:
Code Review ACK 09502452bbbe21bb974f1de8cf53196373921ab9
Tree-SHA512: e1d68281675f05072b3087171cba1df9416a69c9ccf70c72e8555e55eadda2d0fd339e5a894e3a3438ff94b9e3827fb19b8b701faade70c08756b19ff157ee0c
6e77a7b65cda1b46ce42f0c99ca91562255aeb28 keypool: Add comment about TopUp and when to use it (Andrew Chow)
ea50e34b287e0da0806c1116bb55ade730e8ff6c keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow)
bb2c8ce23c9d7ba8d0e5538243e07218443c85b4 keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow)
Pull request description:
* The `TopUp()` in `CWallet::GetNewChangeDestination` is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
* An opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::GetNewDestination` to `CWallet::GetNewDestination`.
* Another opportunistic `TopUp()` is moved from `LegacyScriptPubKeyMan::ReserveKeyFromKeyPool`
Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally.
See also: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174
ACKs for top commit:
instagibbs:
utACK 6e77a7b65c only change is slight elaboration on comment
ryanofsky:
Code review ACK 6e77a7b65cda1b46ce42f0c99ca91562255aeb28. Only the comment changed since my previous review.
Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f
7cecf10ac32af0fca206ac5f24f482bdec88cb7d Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow)
bf6417142f36a2f75b3a11368bd73fe788ae1ccb Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow)
77a777118eaf78f10a439810d1c08d510a539aa0 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow)
35f962fcf0d5107ae6a3a9348e249a9b18ff7106 Clear mapKeys before encrypting (Andrew Chow)
14b5efd66ff0afbf3bf9158a724534a9090fc7fc Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow)
97c0374a46943b2ed38ea24eeeff1f1568dd55b3 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow)
e576b135d6451101d6a8219f55d80aefa216dc38 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow)
fd9d6eebc1eabb4675a118d19d38283da2dead39 Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow)
Pull request description:
Let wallet class handle locked/unlocked status and master key, and let keyman
handle encrypting its data and determining whether there is encrypted data.
There should be no change in behavior, but state is tracked differently. The
fUseCrypto atomic bool is eliminated and replaced with equivalent
HasEncryptionKeys checks.
Split from #17261
ACKs for top commit:
laanwj:
ACK 7cecf10
Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673
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
