## Issue being fixed or feature implemented
Previously noticed, that GetTransaction is called deep inside governance
objects and it is true indeed.
But so far it is used `nullptr` as a mempol object instead any real
mempool in GetTransaction and no usage of a global mempool or any other
mempool, this locks are useless.
This changes are important for mempool globalization.
## What was done?
Removed LOCK for ::mempool.cs in governance's code
## How Has This Been Tested?
Run unit/functional tests. Also done deglobalization of mempool to
validate that governance indeed doesn't use global mempool implicit in
#5169
## Breaking Changes
No breaking changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have assigned this pull request to a milestone
fa511042b0bbec02016761bcd0d30f57e0386550 doc: [test] Remove outdated comment in fuzz runner (MarcoFalke)
Pull request description:
All folders are soft-created with `os.makedirs`
ACKs for top commit:
RiccardoMasutti:
ACK fa51104
Tree-SHA512: 4051688946a205a981bbb005300fe3263495ead26591042b38ae44f4297c7689a613b560052fb5405a62054734d2599cfb0554a37c7b7369fb3a3636743d04a8
fa957f8dc9990e4479e4d2af46a63ceae89cd39b test: Add race:SendZmqMessage tsan suppression (MarcoFalke)
Pull request description:
Add suppression for `race:SendZmqMessage`, which isn't covered by the existing `zmq::*` suppression
Fixes#20618
ACKs for top commit:
hebasto:
re-ACK fa957f8dc9990e4479e4d2af46a63ceae89cd39b, as my previous comment is not directly related to this pull changes.
Tree-SHA512: 8642a8b79bbfa4bee89042b66e528f27fd78c5e84a33023df440662e9114e31445fd7b04940f44b11fa4ab7438d346385a21816289c818cce9958a9b16730452
58cfbc38e040925b51cb8d35d23b50e9cf06fb2a Ignoring (but warn) on duplicate -wallet parameters (Jonas Schnelli)
Pull request description:
I expect that there are many users with load on startup wallet definitions in `bitcoin.conf` or via startup CLI argument.
With the new `settings.json` r/w configuration file, users unloading and loading a wallet through the GUI or via the RPC calls might end up with a duplicate `-wallet` entry (one that still remains in bitcoin.conf or CLI) plus the new duplication in `settings.json` due to the unload/load.
Steps to reproduce
* create wallet (if via RPC set `load_on_startup` or unloadwallet/loadwallet then set `load_on_startup`).
* stop bitcoin
* start bitcoind again with same `--wallet=mywallet`
I guess it is acceptable to skip duplicates.
ACKs for top commit:
achow101:
Tested ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
meshcollider:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
ryanofsky:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a. Changes since previous review: rebased, tweaked warning message, squashed/fixed test
Tree-SHA512: f94e5a999bdd7dc291f0bc142911b0a8033929350d6f6a35b58c4a06a3c8f83147be0f0c402d4e946dedbbcc85b7e023b672c731b6d7a8984d4780017c961cfb
fa5f46600fb98f1b35346bedc1a66c9019d01114 test: Fix rpc_net intermittent issue (MarcoFalke)
Pull request description:
Without the sync, the nodes might generate blocks at the same height and thus never be able to sync
ACKs for top commit:
practicalswift:
ACK fa5f46600fb98f1b35346bedc1a66c9019d01114: patch looks correct
Tree-SHA512: 21255795c2121c71fc620beb766855e57c7af94a668331d1b625665e22eb4b485a2b5c3ad2bb9a7042744f3c3e49c71251bcec41ba25bca03fd54aae32968a3a
7eab781a145a35d0373c4ab4d237a82b4919e88d rpc: Set HTTP Content-Type in bitcoin-cli (Wladimir J. van der Laan)
Pull request description:
We don't set any `Content-Type` in the client. It is more consistent with our other JSON-RPC use to set it to `application/json`.
Note that our server doesn't enforce content types, so it doesn't make a difference in practice. But it is fairly strange HTTP behavior to not set it at all for a POST request.
This came up in #18950.
ACKs for top commit:
promag:
ACK 7eab781a145a35d0373c4ab4d237a82b4919e88d.
jonatack:
Tested ACK 7eab781a145a35d0373c4ab4d237a82b4919e88d
practicalswift:
ACK 7eab781a145a35d0373c4ab4d237a82b4919e88d: patch looks correct
fanquake:
ACK 7eab781a145a35d0373c4ab4d237a82b4919e88d - Looks fine to me.
Tree-SHA512: a9fa155324d0f7bff955585a336ead6bb60b721039f424521a435e4bb0fad3f4532e5cc7b7a9acc4e93585e8d3db3082c010138810f22c0e92b8f749b86ef653
8b39a875581bed1c2f40a7d9616bdb7cc642bf59 bugfix: make LoadWallet assigns status always (Akio Nakamura)
Pull request description:
In my enviroment, ```test/functional/wallet_multiwallet.py``` failed in line 237 for master( 147d50d63 ).
It got an expected rpc-error-message, but error code was not (-4) but (-18).
This is because that although loadwallet() in rpcwallet.cpp assumes LoadWallet() always assign some value to the 'status', but LoadWallet() does not do so in some situation.
This PR intends to fix above and prevends loadwallet() returns ambiguous error code.
ACKs for top commit:
hebasto:
re-ACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59, that is the same as 1728059730abef04f3fa84de0b6e20044be7a9d6.
ryanofsky:
Code review ACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59 (same as previous)
meshcollider:
utACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59
Tree-SHA512: a75d8240f60325bfdb69a07d392269fec97de743f38fe108371eb63a0aba5d8ce3cc484ecc69e81febf8040f5ab64f3a9450b98f8e07a0c17803784bb6f342bf
fa2ae0ac8d43086430a29c73940ad6b1cd129e96 span: Add Span::empty() and use it in script/descriptor (MarcoFalke)
fa8a99258947a9ee3749fa472180542920cd471c Work around memory-aliasing in descriptor ParsePubkey (MarcoFalke)
Pull request description:
While this is not undefined behaviour, the memory aliasing trick is confusing when reading the code. Having `a.size()==0` and then access `a[0]` works in this particular case, but should probably be avoided to harden the code for the future.
ACKs for top commit:
theStack:
re-ACK fa2ae0ac8d
elichai:
ACK fa2ae0ac8d43086430a29c73940ad6b1cd129e96
jonatack:
ACK fa2ae0ac8d43086430a29c73940ad6b1cd129e96
Tree-SHA512: 0ec7b09eef45504973a195923cdf1aa8522117c8e2f69b453e5ce9aa8a7e327c71138518022c32d05133dc99cb861101ed0f60fa891814ee3e9dab3a6fa61a84
55057ffc51697daafac1224664d5e3258ae0b116 rpc: deduplicate WriteHDKeypath() used in decodepsbt (Sebastian Falbesoner)
Pull request description:
The functionality is already provided in the BIP32 utility library `util/bip32.h` with the exact same name and function signature.
ACKs for top commit:
achow101:
ACK 55057ffc51697daafac1224664d5e3258ae0b116
instagibbs:
utACK 55057ffc51
jonatack:
ACK 55057ffc51697daafac1224664d5e3258ae0b116
Tree-SHA512: 074c1a71ffb32908926bf07f0c5428a46309f6e0d21e7c20b1008197c820b97776a441736d0b6fd8ab0c0852522a0b5a5ddb26a1e4a1100ca02aabc65a07a018
6e0d82c55bf4a9aae98c47b7cd00b2828b5dd0ee rpc: remove unused getbalances() code (Jon Atack)
Pull request description:
This line from 999931cf8f1 appears to be extraneous and replaced 2 lines after by `UniValue balances{UniValue::VOBJ};`.
ACKs for top commit:
Empact:
ACK 6e0d82c55b
hebasto:
ACK 6e0d82c55bf4a9aae98c47b7cd00b2828b5dd0ee, the `obj` local variable is not used until the end of the scope.
Tree-SHA512: a220ca9cda091e78144d9b7fbe4bf90e8338d6e8c8dc7bea27a8e62f3a8ac1d983ad12a48a0a3366b2d8b9586878dfc69c1ec34bf846b34c91e42cda48a59850
c1e07423083cd2a7e3f2b28f69a573ea1837af4d doc: Warn about wallet.dat re-use and backups (Albert)
Pull request description:
Following discussion in #18205, this PR adds a warning against re-use of the same wallet file on two different nodes, as that can cause problems due to race conditions between nodes (eg: both nodes using the same addresses at the same time for different things because they are not aware of the other node).
I've also included the rationale behind the warning but I've kept it short to make it clearer to users, not sure if I should have written a longer explanation instead.
Also, while this PR may help some users avoid problems, the changes are largely inconsequential, so feel free to close it if it's not worth the effort.
On an unrelated note, I've also set up [this site](https://corollari.github.io/bitcoin-core-docs/), which periodically pulls bitcoin core and turns its docs into a webpage. Browsing the docs can also be done locally or on github, so this doesn't add much value, but I personally find that more comfortable and it makes them more searchable.
Top commit has no ACKs.
Tree-SHA512: 5ce06026176917304932714470be8c3410d35698f925875b0955ecd3b1756ef52793feb469dd4bdac4921f1a24daf59001e9911f1f096f559fb28c250baae378
1b96a3cd1ebe725896f59614903184289fe62cf8 tests: reset fIsBareMultisigStd after bare-multisig tests (fanquake)
Pull request description:
Fixes: #18015
The bug this fixes is two-part.
1. The `fIsBareMultisigStd` global is being reused by other tests,
such as [script_p2sh_tests(set)](https://github.com/bitcoin/bitcoin/blob/master/src/test/script_p2sh_tests.cpp#L150), after being set to false.
2. The order our tests run in doesn't always? seem to be random,
which meant that the `script_p2sh` tests would only fail if they
were run in an order where the `transaction_tests` ran first,
mutating the `fIsBareMultisigStd` global.
This doesn't seem to happen when running make check, but if you
run `src/test/test_bitcoin and pass --random=99999`, the failure
in `script_p2sh` will occur (on most, but maybe not all systems):
```bash
src/test/test_bitcoin --random=99999
Running 389 test cases...
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[1].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[2].IsStandard
test/script_p2sh_tests.cpp:200: error: in "script_p2sh_tests/set": txTo[3].IsStandard
*** 3 failures are detected in the test module "Bitcoin Core Test Suite"
```
The new test for bare multisig was introduced in #17502.
ACKs for top commit:
Empact:
Code Review ACK 1b96a3cd1e
theStack:
ACK https://github.com/bitcoin/bitcoin/pull/18018/commits/1b96a3cd1ebe725896f59614903184289fe62c
Tree-SHA512: fd7578f9f3faa44d236cd007fc25e31f061acabdb8458559fde0e67d11ab5cafed15305993270c9943a50326574bc5f5301b09494a5b0d2de69e64978093ed45
3c77b8009de9457c356c0bf4362d11bb99a17bb7 fuzz: Improve coverage for CPartialMerkleTree fuzzing harness (practicalswift)
Pull request description:
Improve coverage for `CPartialMerkleTree` fuzzing harness.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK 3c77b8009de9457c356c0bf4362d11bb99a17bb7
Tree-SHA512: a1fa0f7650a5ee5ff83f35e41b9faf6c34671fc304b9af00e5b83073f21d50bcbe91c2428fa64d05dc42a7c521bfd24031e307c7f4abf9ded469d69a55c5d64a
5cafe2b25c1d2f6825b3c8103c280020929dd645 fuzz: Add missing ECC_Start to descriptor_parse test (Ivan Metlushko)
Pull request description:
Fixes fuzzing harness.
I also observed that the corpus for this test consists only of `xprv...` keys while we are using regtest parameters. So for proper fuzzing we need either A) to update the corpus and replace `xprv...` with `tprv...` B) switch to main net in the test
ACKs for top commit:
MarcoFalke:
review ACK 5cafe2b25c1d2f6825b3c8103c280020929dd645
practicalswift:
Tested ACK 5cafe2b25c1d2f6825b3c8103c280020929dd645
Tree-SHA512: 7415a98a445ce0f96219637d2362fecfc1191ad104f55d79ca92b0c92cde165e00646be5bf3fda956385e3cb22540eca457e575048493367cdf0e00a27d7cdb8
c0f09c2c9deaec4cfb35ea587363e6301dd17b88 fuzz: add missing overrides to signature_checker (Jon Atack)
Pull request description:
These functions in `fuzz/signature_checker.cpp` override virtual member functions and should be marked `override` instead of `virtual`, which is for introducing a new virtual function. The overridden virtual functions are in `script/interpreter.h:151/156/161`.
Also, per MarcoFalke suggestion, add missing parentheses in `fuzz/scriptnum_ops.cpp` and remove useless `unsigned int >= 0` conditional in `fuzz/script.cpp`.
These changes fix 5 compile warnings in gcc 10 and 3 in clang 11/12.
ACKs for top commit:
vasild:
ACK c0f09c2
MarcoFalke:
review ACK c0f09c2c9deaec4cfb35ea587363e6301dd17b88
Tree-SHA512: 76ce73ec577c1f23cf8646c31d44dcd6c6303732c47187d041a8921d0d24a50163989a375352ebc221abf2ac337bc0902149be46b6f9eebc071d2f364c407f71
2b78a11b48bad1fa30120ce851269ca9ce8833a5 doc: afl fuzzing comment about afl-gcc and afl-g++ (nsa)
Pull request description:
When trying to build the fuzz tests with `--enable-lcov` on a Ubuntu machine, noticed that the documentation was lacking with regards to the afl-gcc and afl-g++ options. `afl-clang-fast` and `afl-clang-fast++` in the examples just need to be replaced with `afl-gcc` and `afl-g++`. I also had to set the `-m` flag as well to get the fuzzers to run.
ACKs for top commit:
practicalswift:
ACK 2b78a11b48bad1fa30120ce851269ca9ce8833a5
MarcoFalke:
Concept ACK 2b78a11b48bad1fa30120ce851269ca9ce8833a5, haven't tested
Tree-SHA512: d8151afd79de949e8c6da49b69bbbf1470eb478c8ddcbc69b30e86bf9396c0f13835a655d4ae658f7dc4f36c35b02cd23b08358fb73a71e15bf14e76c1f365a4
872aa25fa1d71aa022cdfa02e5927d851d73b3a8 doc: add c++17-enable to fuzzing instructions (Martin Zumsande)
Pull request description:
Update the fuzzing doc because after the merge of #18901, C++17 is required for compilation.
ACKs for top commit:
practicalswift:
ACK 872aa25fa1d71aa022cdfa02e5927d851d73b3a8
MarcoFalke:
ACK 872aa25fa1d71aa022cdfa02e5927d851d73b3a8
Tree-SHA512: 47e37c033690de1d1fa644bf0cebb256036b32a5784021cc0d3b32e6188822d7f517d4342990dc7ec98de6d650794aeb85483157e69e141d6bd011993e124575
420fa0770f37619bfa29898d59dac45b6a477abb fuzz: use std::optional for sep_pos variable (Harris)
Pull request description:
This PR changes the original `size_t sep_pos` to `std::optional<size_t> sep_post_opt` to remove the warning when compiling fuzz tests.
```shell
warning: variable 'sep_pos' may be uninitialized when used here [-Wconditional-uninitialized]
```
Also, it adds `--enable-c++17` flag to CI fuzz scripts.
ACKs for top commit:
practicalswift:
ACK 420fa0770f37619bfa29898d59dac45b6a477abb
MarcoFalke:
ACK 420fa07
Tree-SHA512: e967d5d8ab8ee7394b243ff5b28bac72d30bd14774e4a206f8c87474fad22769da76e4ba4e03cbef83b8f60e5293e9d9293b613e2e2e59e187d4e59ae6b874ca
095bc9a10691505c3d0fdacb6caeb62bfdcf1732 fuzz: fix vector size problem in system fuzzer (Harris)
Pull request description:
This PR fixes a problem with vector resizing in system fuzzer (*case 7* there). Originally, this problem was discussed in PR https://github.com/bitcoin/bitcoin/pull/18908
ACKs for top commit:
MarcoFalke:
ACK 095bc9a10691505c3d0fdacb6caeb62bfdcf1732
practicalswift:
ACK 095bc9a10691505c3d0fdacb6caeb62bfdcf1732
brakmic:
> ACK [095bc9a](095bc9a106)
Tree-SHA512: 73e6004ee51d68a34b49c79d1329a8c4865c21da888801c0fcc7f1bcacb510bf371bb61675eda83e53d08e0f24712e671369719523b0ced0eb2a22607bfa1d3d
8a5014cd8a05b3ab86ae34a47653a82ce11bdf17 Fixes bitcoin#26490 by preventing notifications (John Moffett)
Pull request description:
This is a PR to address https://github.com/bitcoin/bitcoin/issues/26490
The menu bar currently subscribes to window focus change notifications to enable or disable certain menu options in response to the window status.
Notifications are automatically unsubscribed (disconnected in Qt parlance) if the sender is deleted -- in this case, the sender is the QTApplication object (`qApp`). However, MacOS 13 sends a window focus change notification *after* the main window has been destroyed but *before* `qApp` has been fully destroyed.
Since the menu bar is deleted in the main window's destructor, it no longer exists when it receives these notifications (in two different places via lambda expressions). The solution is to pass the main window (`this`) as context when subscribing to the notifications. In this [overloaded version](https://doc.qt.io/qt-5/qobject.html#connect-1) of `connect`, Qt automatically unsubscribes to notifications if the sender OR context (here the main window object) is destroyed. Since the spurious notifications are sent after the main window object is destroyed, this change prevents them from being sent.
Tested on Mac OS 13 and 12 only.
ACKs for top commit:
hebasto:
ACK 8a5014cd8a05b3ab86ae34a47653a82ce11bdf17
Tree-SHA512: 3dff0a252fe0e93dd68cf5503135ecf6a72bcf385ba38407d6021ab77cca323f8bbe58aeca90ec124aa2a22ab9d35b706946179ac3b5d171c96a7010de51a090
## Issue being fixed or feature implemented
autoresending was really slow
## What was done?
reduced the time range to from 1-3 hours from now
## How Has This Been Tested?
hasn't
## Breaking Changes
Shouldn't be
## 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
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] 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: UdjinM6 <UdjinM6@users.noreply.github.com>
fa80b4788bbe3ef00c5d767c0d89ba9809d8707c test: Remove global wait_until from p2p_getdata (MarcoFalke)
999922baed3a80b581ce46daa01c4cbca4fcbfd8 test: Default mininode.wait_until timeout to 60s (MarcoFalke)
fab47375fe0bdec1e557e087fdb0707c4dfa7cc2 test: pep-8 p2p_getdata.py (MarcoFalke)
Pull request description:
Using the global wait_until makes it impossible to adjust the timeout based on the hardware the test is running on.
Fix that by using the mininode member function.
So for example, `./test/functional/p2p_getdata.py --timeout-factor=0.04` gives a timeout of 2.4 seconds.
ACKs for top commit:
laanwj:
ACK fa80b4788bbe3ef00c5d767c0d89ba9809d8707c
Tree-SHA512: ebb1b7860a64451de2b8ee9a0966faddb13b84af711f6744e8260d7c9bc0b382e8fb259897df5212190821e850ed30d4d5c2d7af45a97f207fd4511b06b6674a
9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7 [docs] Improve commenting in ProcessGetData() (John Newbery)
2f032556e08a04807c71eb02104ca9589eaadf1b [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
e257cf71c851e25e1a533bf1d4296f6b55c81332 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
047ceac142246b5d51056a51dbf4645b31802be4 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
Pull request description:
Currently we'll stall peers that send us an unknown INV type in a GETDATA message. Be a bit more friendly and just drop the invalid request.
Ditto for blocks-relay-only peers that send us a GETDATA for a transaction.
There's a test for the first part. The second is difficult to test in the functional test framework since we aren't able to make blocks-relay-only connections.
ACKs for top commit:
sipa:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
brakmic:
ACK 9847e205bf
luke-jr:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
naumenkogs:
utACK 9847e20
ajtowns:
utACK 9847e205bf7edcac4c30ce4b6d62f482aa7bc1b7
Tree-SHA512: 6007f2fd839ffe737727f6fb8e8f083b2d9e05a510748f1d40b8f9be8fdf7b5419a36d8f1039923eec1ba2983e8f6f0436ec5fc196d9f6dcb0657f2ff8ff8e4c
1885ad35467f201f2a210057797aae8a450e7cdf RPC: remove duplicate line in getblock help (Fabian Jahr)
Pull request description:
Line simply seems duplicated in error.
Testing instructions:
Run `src/bitcoin-cli help getblock` on master branch to reproduce. Then build this PR and compare its results.
ACKs for top commit:
dhruv:
tACK `1885ad3`
kristapsk:
ACK 1885ad35467f201f2a210057797aae8a450e7cdf
Emzy:
tACK 1885ad35467f201f2a210057797aae8a450e7cdf
Tree-SHA512: 870c035cb553b0e1d5ef72e64231ef277e0392efe94bc6ecf47129023bd94a6d5a276f46529807f68a1db55c7baa94d9119c7264d9947bc4e5dd9dcefd1b13e7
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
