a1d5b12ec0 Merge getreceivedby tally into GetReceived function (Andrew Toth)
Pull request description:
This PR merges the tally code of `getreceivedbyaddress` and `getreceivedbylabel` into a single function `GetReceived`. This reduces repeated code and makes it similar to `listreceivedbyaddress` and `listreceivedbylabel`, which use the function `ListReceived`. It will also make the change in #14707 simpler and easier to review.
ACKs for top commit:
theStack:
re-ACK a1d5b12ec0
meshcollider:
utACK a1d5b12ec0
Tree-SHA512: 43d9cd92f7c2c6a8b9c7509aa85a9b9233a6cfec1c43a9062e3bdfb83515413d1feafa8938c828351278ba22bd31c47e62ab5341e4bddc2493103b094d73b047
709998467e rpc: doc: Fix and extend getblockstats examples (Adam Soltys)
Pull request description:
This pull fixes the example curl command for `getblockstats` which doesn't work as is because it's missing a comma between the params and has single quotes around the second parameter.
It also adds an additional example of getting block stats by hash by using a known workaround (#15412) to get bitcoin-cli to treat the hash parameter as JSON instead of a string since there is ongoing deliberation about how or whether to fix the root issue (#15448).
ACKs for top commit:
theStack:
ACK 709998467e
Tree-SHA512: 84a5b7f449f06fff785bc0afbc1a7dfd55454bc76c52a8945e91556f87f3edfdc5a1780faab8fcfd6c415b734295b7c67d2e04ba7b6cfa91a77758af5dda53ae
a9ecbdfcaa test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034996 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)
Pull request description:
This PR fixes https://github.com/bitcoin/bitcoin/issues/18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed:
c0b389b335/src/net.h (L812)
and after a loaded filter is removed again through a `filterclear` message:
c0b389b335/src/net_processing.cpp (L3201)
The behaviour was introduced by commit 37c6389c5a (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell).
This default match-everything filter leads to some unintended side-effects:
1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) c0b389b335/src/net_processing.cpp (L1504-L1507)
2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) c0b389b335/src/net_processing.cpp (L3182-L3186)
This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message.
Here is a before/after comparison in behaviour:
| code part / scenario | master branch | PR branch |
| --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- |
| `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload` |
| `filteradd` processing, no filter was loaded | nothing | peer's banscore increases by 100 (i.e. disconnect) |
On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch).
Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set.
ACKs for top commit:
MarcoFalke:
re-ACK a9ecbdfcaa, only change is in test code 🕙
Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
92fe537cf7 test: fix intermittent race condition in interface_bitcoin_cli.py (Jon Atack)
c648e636b2 test: add wait_for_cookie_credentials() to test framework (Jon Atack)
Pull request description:
This PR adds a `wait_for_cookie_credentials()` method to the test framework and calls it before the `-rpcwait` tests, to avoid an intermittent race condition on the CI run with Valgrind where the cookie file isn't written yet when the CLI call with `-rpcwait` arrives to `get_auth_cookie()`.
To reproduce/test, build with
```diff
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 60c4d06f12..3dd06c4758 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -291,6 +291,7 @@ static bool InitRPCAuthentication()
bool StartHTTPRPC()
{
LogPrint(BCLog::RPC, "Starting HTTP RPC server\n");
+ UninterruptibleSleep(std::chrono::seconds{11});
if (!InitRPCAuthentication())
```
then run the test normally and with valgrind
```
test/functional/interface_bitcoin_cli.py -l debug
valgrind test/functional/interface_bitcoin_cli.py -l debug
```
Thanks to Marco Falke for all the help. Closes#18684.
Top commit has no ACKs.
Tree-SHA512: 1b76635b5b1d6b05138affef7ab788aa3bc3fc75b0c69ba778ecdf81063cfe02a8dd7667cfd63a6c6e19b2dac47d7a8b755e334d8af5c0ab9d4026808ee96c83
by calling wait_for_cookie_credentials() to ensure the cookie file is written
and auth credentials available for testing the CLI -rpcwait option before the
RPC connection is up.
fc289b7898 wallet: Refactor WalletRescanReserver to use wallet reference (João Barbosa)
Pull request description:
Simple refactor to `WalletRescanReserver` to use wallet reference instead of pointer.
Complements #18259.
ACKs for top commit:
MarcoFalke:
ACK fc289b7898
Tree-SHA512: b03e33f2d9df2870436aa3284137fd022dd89ea96a1b170fa27f8685ad4f986e6c4ba5975a84966c30d18430a4014d7d8740a1dff2f985c9ef8226ed18e69db9
9df32e820d scripted-diff: test: replace command with msgtype (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to https://github.com/bitcoin/bitcoin/pull/18533, which changed the naming of `strCommand` to `msg_type` in the network processing code. The same approach is done here for the function test framework, to get rid of the wrong "command" terminology for network mesage types. (Commands are usually used in the CLI or RPC context, so using the same name in the network message context would only be confusing.)
The commit was created through the following steps:
1. search for all occurences of the string "command" within the folder `test/functional`
```git grep -i command test/functional > command_finds```
2. manually sort out all false-positives, i.e. occurences of "command" which describe commands in the correct sense (mostly CLI or RPC related, also some with Socks5)
3. put the remaining occurences into a scripted-diff (a quite simple one, actually) that renames "command" to "msgtype" in the concerned files.
The name `msgtype` was intentionally chosen without the underscore `_` as classes beginning with `msg_` define concrete types of messages.
ACKs for top commit:
MarcoFalke:
ACK 9df32e820d . Makes sense that tests use the same naming as Bitcoin Core. See `NetMsgType` here: https://doxygen.bitcoincore.org/namespace_net_msg_type.html
Tree-SHA512: cd0ee08a382910b7f10ce583acdaf4f8a39f9ba4a22434a914415727eedd98bac538de9bf6633574d5eb86f62558bc8dcb638a3289d99b04f8481f34e7a9a0c7
0d32d66148 Remove -upgradewallet startup option (Andrew Chow)
92263cce5b Add upgradewallet RPC (Andrew Chow)
1e48796c99 Make UpgradeWallet a member function of CWallet (Andrew Chow)
c988f27937 Have UpgradeWallet take the version to upgrade to and an error message out parameter (Andrew Chow)
1833237123 Only run UpgradeWallet if the wallet needs to be upgraded (Andrew Chow)
9c16b1735f Move wallet upgrading to its own function (Andrew Chow)
Pull request description:
`-upgradewallet` is largely incompatible with many recent wallet features and versions. For example, it was disabled if multiple wallets were used and would not work with encrypted wallets that were being upgraded to HD.
This PR does away with the old method of upgrading upon startup and instead allows users to upgrade their wallets via an `upgradewallet` RPC. This does largely the same thing as the old `-upgradewallet` option but because the wallet is loaded, it can be unlocked to upgrade to HD. Furthermore it is compatible with multiwallet as it works on the individual wallet that is specified by the RPC.
ACKs for top commit:
meshcollider:
Code review ACK 0d32d66148
darosior:
ACK 0d32d66148
MarcoFalke:
ACK 0d32d66148🚵
Tree-SHA512: b425bf6f5d605e26506889d63c780895482f07cbc086193218e031e8504d3072d41e90d65cd41bcc98ee4c1eb048954bc5d4ac85435f7394892373aac89a3b0a
3718ae2ef8 [tests] Don't initialize PrecomputedTransactionData in txvalidationcache tests (John Newbery)
Pull request description:
PrecomputedTransactionData is initialized inside CheckInputScripts(). No need to pre-initialize it before calling into CheckInputScripts().
Normally, I wouldn't bother, but we're making changes to `PrecomputedTransactionData` in #17977 which would break these tests without removing these constructions. Might as well get these changes out of the way here.
ACKs for top commit:
robot-visions:
ACK 3718ae2ef8
sipa:
utACK 3718ae2ef8
Tree-SHA512: bc9c095035a7072a2a91941df38cdbb969e817264efbaa6dcb88cc3ab132d9264aa0751fa588d1a5e45f37b4d2bb1903cda078765f0bbcc87d9cc47cbec5356a
fa03713e13 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (take 2) (MarcoFalke)
Pull request description:
actually (?) fix#18561
See most recent traceback https://travis-ci.org/github/bitcoin/bitcoin/jobs/674668692#L7062
I believe the reason the error is still there is that ConnectionResetError is derived from OSError:
ConnectionResetError(ConnectionError(OSError))
And IOError is an alias for OSError since python 3.3, see https://docs.python.org/3/library/exceptions.html#IOError
So fix that by renaming IOError to the alias OSError and move the less specific catch clause down a few lines.
ACKs for top commit:
jonatack:
ACK fa03713e13
Tree-SHA512: 6e5b214ed9101bf8ebe7472dcc1f9e9d128e2575c93ec00c8d0774ae1a9b52a8c2a653a45a0eab8d881570b08dd5ffeddf5aca88a10438c366e1f633253cb0b5
27abd1a4f4 test: Replace boost::mutex with std::mutex (Hennadii Stepanov)
Pull request description:
This PR replaces `boost::mutex` with `std::mutex` in the `scheduler_tests` test suite.
ACKs for top commit:
theStack:
ACK 27abd1a4f4
sipa:
utACK 27abd1a4f4
Tree-SHA512: 062eed360a68910fb71552fd892bfd097442718a237446cfb8350bfd5d807da7251ead2b9755e1d7022598774ed23fa5432a589ac6f8cadddab404b439883466
ASLR is not currently working for the bitcoin-cli.exe binary. This is
due to it not having a .reloc section, which is stripped by default by
the mingw-w64 ld we use for gitian builds. A good summary of issues with
ld and mingw-w64 is available in this thread:
https://sourceware.org/bugzilla/show_bug.cgi?id=19011.
All other Windows binaries that we distribute (bitcoind, bitcoin-qt,
bitcoin-wallet, bitcoin-tx and test_bitcoin) do not suffer this issue,
and currently having working ASLR. This is due to them exporting
(inadvertent or not) libsecp256k1 symbols, and, as a result, the .reloc
section is not stripped by ld.
This change is a temporary workaround, also the same one described here:
https://www.kb.cert.org/vuls/id/307144/, that causes main() to be
exported. Exporting a symbol will mean that the .reloc section is not
stripped, and ASLR will function correctly.
fabfcad876 test: Bump timeout in wallet_import_rescan (MarcoFalke)
Pull request description:
Avoid timeouts when starting the node, also make error message more verbose
ACKs for top commit:
practicalswift:
ACK fabfcad876 -- patch looks correct
Tree-SHA512: 8fd60a05380349f521d0e814d2f268702dfbe57c7567a4f6e94435498dfdd32909179d75fded44757ecb1a93a4045842bc6d00bfd6cd18ba751513461359c7b0
92bcd70808 [wallet] allow transaction without change if keypool is empty (Sjors Provoost)
709f8685ac [wallet] CreateTransaction: simplify change address check (Sjors Provoost)
5efc25f963 [wallet] translate "Keypool ran out" message (Sjors Provoost)
Pull request description:
Extracted from #16944
First this PR simplifies the check when generating a change address, by dropping `CanGetAddresses` and just letting `reservedest.GetReservedDestination` do this check.
Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn't need the change address (e.g. when spending the entire balance), then it's all good. If we did need a change address, we throw the original error.
ACKs for top commit:
fjahr:
Code review ACK 92bcd70808
jonasschnelli:
utACK 92bcd70808
achow101:
ACK 92bcd70808
meshcollider:
Code review ACK 92bcd70808
Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006
This fixes the example curl command for `getblockstats` which is missing
a comma between the params and has single quotes around the second
parameter.
Besides fixing the existing example, this commit adds an additional
example of getting block stats by hash by using a known workaround with
bitcoin-cli to get it to treat the hash parameter as a JSON string by
wrapping it in both single and double quotes.
Co-Authored-By: Andrew Toth <andrewstoth@gmail.com>
Co-Authored-By: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
6f8b498d18 fuzz: http_request workaround for libevent < 2.1.1 (Sebastian Falbesoner)
Pull request description:
The fuzz test `http_request` calls the following two internal libevent functions:
* `evhttp_parse_firstline_`
* `evhttp_parse_headers_`
Before libevent 2.1.1 however, internal functions names didn't end with an underscore (see libevent commit 8ac3c4c25b and [Changelog for 2.1.1.-alpha](https://github.com/libevent/libevent/blob/master/ChangeLog#L1830) when the change was first mentioned) hence the build fails with a linking error.
This PR adds a preprocessor workaround to the test that checks for the libevent version (via ~`_EVENT_NUMERIC_VERSION`~ `LIBEVENT_VERSION_NUMBER`) and creates wrapper functions mapping to naming scheme without underscore in case the version is older than 2.1.1.
Tested with Ubuntu Xenial 16.04.6 LTS and clang-8.
ACKs for top commit:
hebasto:
ACK 6f8b498d18, tested on xenial:
Tree-SHA512: 3b9e0147b8aea22e417d418e3b6d4905f5be131c2b0ae4b0f8b9411c5606d2e22f1b23e1ecc6980ecab907c61404de09e588aae1ac43cf70cf9e8d006bbdee73
fa32097541 test: Create cached blocks not in the future (MarcoFalke)
Pull request description:
This avoids test failures when tests assume blocks are not from the future, like in wallet_dump: https://cirrus-ci.com/task/6607130193035264?command=ci#L3306
ACKs for top commit:
jonatack:
ACK fa32097541
Tree-SHA512: 60b6882e0e1df8c5d67f034533407a45d3685983891b67ff4631072bfd0a93a325c7ca18758d7a2df252e4fcdb7c87321cb1e84458b22782e57e719eec634c22
faceeae49a ci: Disable valgrind functionl tests on forked repos to avoid timeouts (MarcoFalke)
Pull request description:
Allows people to fork our repo and run the tests again
Also print more cache stats
ACKs for top commit:
hebasto:
ACK faceeae49a, tested on my own repo: https://travis-ci.org/github/hebasto/bitcoin/jobs/676257500
Tree-SHA512: 50e44edf94fcb997438eeaf7308b2b58a0854141ecb1fbb0ba7bf5ed662f19b60899f966f579cca90ad5e789234d0e90122d8c2c854da70339058adc3a475fa6
fa168d7542 rpc: Document all aliases for first arg of listtransactions (MarcoFalke)
fa5b1f067f rpc: Document all aliases for second arg of getblock (MarcoFalke)
fa86a4bbfc rpc: Rename first arg of generateblock RPC to "output" (MarcoFalke)
Pull request description:
This fixes a bug found with #18531:
* Currently the named argument for `generateblock` is documented as `address/descriptor`, but the server only accepts a named argument of `address`. Fix it by changing the name to `output` for both the documentation and the server code. Also, add tests to prove the server understands the new name `output`.
* Unrelated to that, there have been a bunch of aliases in the server code that are not present in the source code of the documentation. Fix that by adding the alias to the source code of the documentation. Only the first alias is displayed in the rendered documentation. Also, add tests to prove the server actually understands all aliases.
ACKs for top commit:
pierreN:
Tested ACK fa168d7 tests, help messages
Tree-SHA512: 05e15628e3a667b296f3783d20f764b450b959451b5360c7eaf5993156582d47a0f5882330ca2493b851eb46324d504953b90c875bc88a15c9e8c89eb3ef8d92
fa4632c417 test: Move boost/stdlib includes last (MarcoFalke)
fa488f131f scripted-diff: Bump copyright headers (MarcoFalke)
fac5c37300 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c417
jonatack:
ACK fa4632c417, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
38677274f9 rpc: settxfee respects -maxtxfee wallet setting (Fabian Jahr)
bda84a08a0 rpc: Add documentation for deactivating settxfee (Fabian Jahr)
Pull request description:
~~Closes 18315~~
`settxfee` can be deactivated by passing 0 as the fee. That does not seem to be documented for the user so this PR adds it in the description. The return value of a simple boolean seems also too simplified given the multiple dimensions that this deactivation feature enables. I.e. it does not seem intuitive if the returned boolean shows that the call succeeded or if means that `settxfee` is active. My suggested solution is to change the return value to a JSON object that included the "active" state and the currently set fee rate.
Examples:
```
$ src/bitcoin-cli settxfee 0.0000000
{
"active": false,
"fee_rate": "0.00000000 BTC/kB"
}
$ src/bitcoin-cli settxfee 0.0001
{
"active": true,
"fee_rate": "0.00010000 BTC/kB"
}
```
ACKs for top commit:
MarcoFalke:
ACK 38677274f9, seems useful to error out early instead of later #16257🕍
jonatack:
ACK 38677274f9
meshcollider:
LGTM, utACK 38677274f9
Tree-SHA512: 642813b5cf6612abb4b6cb63728081a6bd1659d809e0149c8f56060b6da7253fee989b3b202854f3051df3773c966799af30b612648c466b099f00590f356548
a2324e4d3f test: Improve naming and logging of avoid_reuse tests (Fabian Jahr)
1abbdac677 wallet: Prefer full destination groups in coin selection (Fabian Jahr)
Pull request description:
Fixes#17603 (together with #17843)
In the case of destination groups of >10 outputs existing in a wallet with `avoid_reuse` enabled, the grouping algorithm is adding left-over outputs as an "incomplete" group to the list of groups even when a full group has already been added. This leads to the strange behavior that if there are >10 outputs for a destination the transaction spending from that will effectively use `len(outputs) % 10` as inputs for that transaction.
From the original PR and the code comment I understand the correct behavior should be the usage of 10 outputs. I opted for minimal changes in the current code although there maybe optimizations possible for cases with >20 outputs on a destination this sounds like too much of an edge case right now.
ACKs for top commit:
jonatack:
Re-ACK a2324e4
achow101:
ACK a2324e4d3f
kallewoof:
ACK a2324e4d3f
meshcollider:
Tested ACK a2324e4d3f (verified the new test fails on master without this change)
Tree-SHA512: 4743779c5d469fcd16df5baf166024b1d3c8eaca151df1e8281b71df62b29541cf7bfee3f8ab48d83e3b34c9256e53fd38a7b146a54c79f9caa44cce3636971a
69ffddc83e 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 69ffddc83e
jonatack:
ACK 69ffddc83e, code review, compiled a fuzz build and started the bloom_filter fuzz test as a sanity check.
promag:
ACK 69ffddc83e.
Tree-SHA512: 6c53678545ad8e2fa1ffc0a8838e450462f26748a60632f738dc020f0eb494ae2c32841e6256e266ed9140177257a78b707123421942f3819a14ffcb9a99322f
9b5950db86 bnb: exit selection when best_waste is 0 (Andrew Chow)
Pull request description:
If we find a solution which has no waste, just use that. This solution
is what we would consider to be optimal, and other solutions we find
would have to also have 0 waste, so they are equivalent to the first
one with 0 waste. Thus we can optimize by just choosing the first one
with 0 waste.
Closes#18257
ACKs for top commit:
instagibbs:
utACK 9b5950db86
meshcollider:
utACK 9b5950db86
Tree-SHA512: 59565ff4a3d8281e7bc0ce87065a34c8d8bf8a95f628ba96b4fe89f1274979165aea6312e5f1f21b418c8c484aafc5166d22d9eff9d127a8192498625d58c557
