Check that...
- mempool tracks & reattempts delivery of a transaction where a GETDATA hasn't
been requested by a peer yet.
- transaction delivery is not attempted again after GETDATA is received.
- transaction is removed from the unbroadcast set when its removed from the
mempool.
Since the mempool unbroadcast mechanism handles the reattempts for initial
broadcast, the wallet rebroadcast attempts can be much less frequent
(previously ~1/30 min)
Adds a --descriptors option globally to the test framework. This will
make the test create and use descriptor wallets. However some tests may
not work with this.
Some tests are modified to work with --descriptors and run with that
option in test_runer:
* wallet_basic.py
* wallet_encryption.py
* wallet_keypool.py
* wallet_keypool_topup.py
* wallet_labels.py
* wallet_avoidreuse.py
RPCOverloadWrapper overloads some deprecated or disabled RPCs with
an implementation using other RPCs to avoid having a ton of code churn
around replacing those RPCs.
9f5608c289 test: check for matching object hashes in wait_for_getdata (Danny Lee)
Pull request description:
Previously, `wait_for_getdata` only looked for the presence of a recent `"getdata"` message. Additionally checking the object hashes inside the message should make tests involving `wait_for_getdata` more robust.
`p2p_sendheaders.py` already overrides `wait_for_getdata` do this check; we can use the same approach consistently across all tests that call `wait_for_getdata`.
This PR is progress towards #18614 , but closing that issue would also involve some additional changes to `wait_for_getheaders`.
ACKs for top commit:
theStack:
ACK 9f5608c289🍻
Tree-SHA512: 8e7f95881c19631db014d4bb2399fea0d14686a32542f6ca3b60809744b0d684eac4e4c107c87143991f3cd0c2d4ab09d0c17486239768a9b40bee25f2e4d54a
3c21db7b78 [test] add 8 error messages to feature_segwit and change version to type (Gloria Zhao)
Pull request description:
Followup to [this](https://github.com/bitcoin/bitcoin/pull/15169/files#r303673472) comment on functional test feature_segwit.py verifying that unsigned witness transactions are invalid.
(1) Changes 8 error messages from "mandatory-script-verify-flag" to "non-mandatory-script-verify-flag" and with more specific error messages.
(2) Edits comments that incorrectly describe the test, namely that the `v` variable corresponds to using P2WSH versus P2WPKH, not witness versions.
ACKs for top commit:
MarcoFalke:
ACK 3c21db7b78🍾
Tree-SHA512: 3734ea3762667636c4fb20f5285634ab94d6b3527b7390fcc5e41b4582829dfe0099beabeaed42098613d168ede3385a6ffcd73989d1fa9dbd18004f5e9cf083
P2WPKH witness program without signature -> throws "hash mismatch" error
P2WSH witness program without signature -> throws "empty witness" error
same errors for P2SH_P2WPKH and P2SH_P2WSH respectively when passed redeemScript but no signature
P2SH_P2WPKH and P2SH_P2WSH with no signature fail with "Operation not valid with current stack size" when not signed due to missing input
change VER to TYPE and constants WIT_V0 to P2WPKH=0 and WIT_V1 to P2WSH=1
fac2fc4dd8 test: Increase debugging to hunt down mempool_reorg intermittent failure (MarcoFalke)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 4094b44afaa623e58b69f8d0332e60f0150b9ae2fd8bb265210d85546d887672ab8a3435cd9b086be14f69ab5b17e0f9fae06bd8aec1e7947ca766dd72b577c4
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
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.
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
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
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
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
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
This is the functional test framework pendant for
7777e3624f, which renamed "strCommand" with
"msg_type" in the network processing code.
-BEGIN VERIFY SCRIPT-
# Rename in test framework
sed -i 's/command/msgtype/g' ./test/functional/test_framework/messages.py ./test/functional/test_framework/mininode.py
# Rename in individual tests
sed -i 's/command/msgtype/g' ./test/functional/p2p_invalid_messages.py ./test/functional/p2p_leak.py
-END VERIFY SCRIPT-
check the following expected behaviors if no filter is set:
-> filtered block requests are ignored by the node
-> sending a 'filteradd' message is treated as misbehavior
(i.e. the peer's banscore increases by 100)
also fixes a bug in the on_inv() callback method, which
directly modified the type from BLOCK to FILTERED_BLOCK
in the received 'inv' message rather than just for the reply
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
When a wallet uses avoid_reuse and has a large number of outputs in
a single destination, it groups these outputs in OutputGroups that
are no larger than OUTPUT_GROUP_MAX_ENTRIES. The goal is to spend
as many outputs as possible from the destination while not breaking
consensus due to a huge number of inputs and also not surprise the
use with high fees. If there are n outputs in a destination and
n > OUTPUT_GROUP_MAX_ENTRIES then this results in one or many groups
of size OUTPUT_GROUP_MAX_ENTRIES and possibly one group of size
< OUTPUT_GROUP_MAX_ENTRIES.
Prior to this commit the coin selection in the case where
n > OUTPUT_GROUP_MAX_ENTRIES was skewed towards the one group of
size < OUTPUT_GROUP_MAX_ENTRIES if it exists and the amount to be
spent by the transaction is smaller than the aggregate of those
of the group size < OUTPUT_GROUP_MAX_ENTRIES. The reason is that
the coin selection decides between the different groups based on
fees and mostly the smaller group will cause smaller fees.
The behavior that users of the avoid_reuse flag seek is that the
full groups of size OUTPUT_GROUP_MAX_ENTRIES get used first. This
commit implements this by pretending that the small group has
a large number of ancestors (one smallet than the maximum allowed
for this wallet). This dumps the small group to the bottom of the
list of priorities in the coin selection algorithm.
0306d78cb4 Use getbalances in wallet_address_types tests (Jon Atack)
7eacdc5167 Shift coverage from getunconfirmedbalance to getbalances in wallet_abandonconflict tests (Jon Atack)
3e6f7377f6 Improve getbalances coverage in wallet_balance tests (Jon Atack)
Pull request description:
<strike>This PR updates several tests and then removes the `getunconfirmedbalance` RPC which was deprecated in facfb4111d a year ago.
Next steps: remove the deprecated `getwalletinfo` fields and the `getbalance` RPC in follow-ups, if there seems to be consensus on those removals.</strike>
Update:
`getunconfirmedbalance` RPC was deprecated in facfb4111d a year ago, but following the review comments below, this PR now only updates the test coverage to use `getbalances` while still leaving basic coverage for `getunconfirmedbalance` in wallet_balance.py.
That said, I've seen 3 regular contributors confused in the past 10 days by "DEPRECATED" warnings in the code that are not following the deprecation policy in [JSON-RPC-interface.md#versioning](https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning).
ISTM these warnings should either be removed, or the calls deprecated (`-deprecatedrpc`), or the policy updated to describe these warnings as a pre-deprecation practice.
ACKs for top commit:
jnewbery:
utACK 0306d78cb
Tree-SHA512: 692e43e9bed5afa97d905740666e365f0b64e559e1c75a6a398236d9e943894e3477947fc11324f420a6feaffa0c0c1532aa983c50090ca39d06551399e6ddd1
9cdddae3b4 test: add rpc_signrawtransaction logging (Jon Atack)
4d6cde38ce test: refactor rpc_signrawtransaction witness script tests (Jon Atack)
Pull request description:
As a follow-up to #18484, the new tests are good but bury the one non-duplicate line in each test that sets the witness script, and there is no logging in the testfile. This PR makes it easy to see what is unique to each of the new tests and adds logging.
ACKs for top commit:
theStack:
ACK 9cdddae3b4🥚🐰
Tree-SHA512: 7b1ca303326658afb90b7635abc9fe8bb65f0be004124d4dcf38702bb6f38bc06ce33c0642be4ad5d511453d003cdefeea691e66e3b963a4feb66f6237a3c241
fab9899204 test: Try once more when RPC connection fails on Windows (MarcoFalke)
faa655731e test: Document why connection is re-constructed on windows (MarcoFalke)
fa9f4f663c test: Remove python 3.4 workaround (MarcoFalke)
fae760f2b2 cirrus: Bump freebsd to 12.1 (MarcoFalke)
Pull request description:
Fixes: #18548
ACKs for top commit:
hebasto:
ACK fab9899204, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: c4e9ed8d995b63a820ca66984f152ac216c83ba1f318b61b15c6d375c0e936c08f6bc3d38c255dddf3ee8952f848c7ababf684854e07a7c1b1d8504e6b7208ba
555567ace9 test: Extend wallet_dump test to cover comments (MarcoFalke)
Pull request description:
ACKs for top commit:
ryanofsky:
Code review ACK 555567ace9. Nice new checks in this test. I confirmed this catches the missing FormatISO8601DateTime call you discovered in https://github.com/bitcoin/bitcoin/pull/17954#discussion_r406891999
Tree-SHA512: 71aa23dd039f3bcdee642b01151edd1a0d44f48cedd070f5858148c8cb8abd6f5edfd212daeba38e35c843da5ea6c799e5a952105fdecedac355a5a843c05a84
fa404f1e47 test: Check that the version message does not leak the local address of the node (MarcoFalke)
Pull request description:
Add test for #8740
ACKs for top commit:
theStack:
ACK fa404f1e47
Tree-SHA512: 4d1c10d1c02fba4b51bd8b9eb3a0d9a682b6aac8c3f6924e295fdca3faefa5ecc3eaa87d347cfec5d2b2bc49963c10fe0a37c463f36088ed0304a2e3716b963b
The interfaces for the methods wait_for_header() and wait_for_merkleblock() are
changed to take a hex string instead of an integer, improving type safety and
removing the burden from the caller to always do the transformation via
`int(...)`. As suggested by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/18593#discussion_r407062253
Implements the missing initialization/serialization methods for
msg_merkleblock, based on the already present class CMerkleBlock.
Also changes the method wait_for_merkleblock() to be more precise by waiting
for a merkleblock with a specified blockhash instead of an arbitrary one.
In the BIP37 test p2p_filter.py, this new method is used to make the test of
receiving merkleblock and tx if a filter is set to be more precise, by checking
if they also arrive in the right order.
7524b6479c Add tests for generateblock (Andrew Toth)
dcc8332543 Add generateblock rpc (Andrew Toth)
Pull request description:
The existing block generation rpcs for regtest, `generatetoaddress` and `generatetodescriptor`, mine everything in the mempool up to the block weight limit. This makes it difficult to test a system for several scenarios where a different set of transactions are mined. For example:
- Testing the common scenario where a transaction is replaced in the mempool but the replaced transaction is mined instead.
- Testing for a double-spent transaction where a transaction that conflicts with the mempool is mined.
- Testing for non-standard transactions that are mined.
- Testing the scenario where several blocks are mined without a specific transaction in the mempool being included in a block.
This PR introduces a new rpc, `generateblock`, that takes an array of raw transactions and txids and mines only those and the coinbase. Any txids must be in the mempool, but the raw txs can be anything conforming to consensus rules. The coinbase can be specified as either an address or descriptor.
This reopens#17653 since it was closed by mistake.
Thanks to instagibbs for code suggestions that I used here.
ACKs for top commit:
MarcoFalke:
re-ACK 7524b6479c📁
Tree-SHA512: 857106007465b5b9b8a84b6d07c17cbf8378a33a72d32ff79abea1d5ab4babb4d53a11ddbb14595aa1fac9dfa1391e3a11403d742f69951beea2f683e8a01cd4
5df0877f91 test: update and harden interface_bitcoin_cli tests (Jon Atack)
75019774c9 cli -getinfo: use getbalances instead of deprecated getwalletinfo balance (Jon Atack)
Pull request description:
Extracted from #18453 to preserve that PR as a discussion on multiwallet RPC/CLI.
This PR updates `bitcoin-cli -getinfo` to fetch the wallet balance from `getbalances` in order to no longer depend on `getwalletinfo.balance` which was deprecated a year ago in facfb41.
I found this when removing the getwalletinfo() `balance`, `unconfirmed_balance`, and `immature_balance` fields to see what broke from depending on them.
I didn't see any perceivable change in `-getinfo` run time from the change.
Test coverage for this change is provided by `test/functional/interface_bitcoin_cli.py`, which the second commit updates to (a) no longer depend on getwalletinfo.balances and (b) test the -getinfo blockcount and balance fields against non-default, non-zero values.
ACKs for top commit:
robot-visions:
ACK 5df0877
MarcoFalke:
ACK 5df0877
vasild:
re-ACK 5df0877f9
promag:
Code review ACK 5df0877f91.
theStack:
ACK 5df0877f91
Tree-SHA512: 0dd8c62f915b1c0112e42b132dcf74a141bdd1f51e7c17d4a698b374ec296f4f9836f7058dbe237cf24f9bfb32ea5000e14f7089e2e86472d9c6a175be26e910
fa1da3d4bf test: Add basic addr relay test (MarcoFalke)
fa1793c1c4 net: Pass connman const when relaying address (MarcoFalke)
fa47a0b003 net: Make addr relay mockable (MarcoFalke)
Pull request description:
As usual:
* Switch to std::chrono time to be type-safe and mockable
* Add basic test that relies on mocktime to add code coverage
ACKs for top commit:
naumenkogs:
utACK fa1da3d
promag:
ACK fa1da3d4bf (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review.
Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
faede1b293 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (MarcoFalke)
Pull request description:
Should fix issues such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/671910152#L7034
Top commit has no ACKs.
Tree-SHA512: ac659f29c5ec91985c916b734e24911cbf4e2c5c4b1f1891a7e6c2d2511ec285167550fb03848eee4a7a3cbc9f8cdb0c766f4e881d9e44368c7415d007006368
7a2ecf16df Wallet: Change IsMine check in CWallet::DelAddressBook from assert to failure (Luke Dashjr)
2952c46b92 Wallet: Replace CAddressBookData.name with GetLabel() method (Luke Dashjr)
d7092c392e QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet (Luke Dashjr)
Pull request description:
Follow-up to #18192, not strictly necessary for 0.20
ACKs for top commit:
MarcoFalke:
re-ACK 7a2ecf16df, only change is adding an assert_equal in the test 🔰
jnewbery:
utACK 7a2ecf16df
Tree-SHA512: e0933ee40f705b751697dc27249e1868ed4874254b174ebdd0a7150125d8c818402e66df2371718c7eeb90e67ee2317215fb260aa9b9d7b9b45ee436de2988ff
fa2251df5e test: Use one node to avoid a race due to missing sync in rpc_signrawtransaction (MarcoFalke)
Pull request description:
Node 0 creates a transaction in a block, and node 1 sends a spending transaction without properly syncing the utxo set.
Fixes intermittent test failure in rpc_signrawtransaction
```
test 2020-04-01T00:14:03.400000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\projects\bitcoin\test\functional\test_framework\test_framework.py", line 112, in main
self.run_test()
File "C:\projects\bitcoin/test/functional/rpc_signrawtransaction.py", line 213, in run_test
self.witness_script_test()
File "C:\projects\bitcoin/test/functional/rpc_signrawtransaction.py", line 208, in witness_script_test
self.nodes[1].sendrawtransaction(spending_tx_signed['hex'])
File "C:\projects\bitcoin\test\functional\test_framework\coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "C:\projects\bitcoin\test\functional\test_framework\authproxy.py", line 141, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: bad-txns-inputs-missingorspent (-25)
```
Full log: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31864368
ACKs for top commit:
achow101:
ACK fa2251df5e
Tree-SHA512: 9450d216d9989d6c44028ae4b9818790cfb00796e0de22331422f775f74d697bb14ebae0e88dca20c6b641363780da384fe94c708e20fce9cfde929fb343b12f
b224b4e7bd test: wallet_bumpfee assertion fixup (Jon Atack)
Pull request description:
Follow-up to #18516 to fix up an assertion as per suggested change in https://github.com/bitcoin/bitcoin/pull/18516#discussion_r404191587.
ACKs for top commit:
jnewbery:
ACK b224b4e7bd
Tree-SHA512: 4973bba73a67c1ffaf460921b3d454e9d66a40a67f73b7df742e24a0e389adba3946a3958a729391ee6bfa4ef844be759ebf71d14d788434c248e48a2bbe5bde
cd3b1569d9 Correctly compute redeemScript from witnessScript for signrawtransaction (Andrew Chow)
Pull request description:
`ParsePrevouts` uses `GetScriptForWitness` on the given witnessScript to find the corresponding redeemScript. This is incorrect when the witnessScript is either a P2PK or P2PKH script as it returns the corresponding P2WPK script instead of turning the witnessScript into a P2WSH script. Instead this should make the script a `WitnessV0ScriptHash` destination and get the script for that.
Test cases are also added. These will fail on master with a `redeemScript does not correspond to witnessScript`
Reported on [Bitcointalk](https://bitcointalk.org/index.php?topic=5236818.0)
ACKs for top commit:
MarcoFalke:
weak ACK cd3b1569d9, only checked that the test fails without the code change 🚰
instagibbs:
utACK cd3b1569d9
Tree-SHA512: afac671dbb52ce88bfb4a9ca3dd6065427ad52c9778d0549ad40e9286778f308adad24fb3b3c3089545d7f88c57c53d41224fd7a4bb207550eff2fe06600118f
25e03ba1ff test: relax bumpfee dust_to_fee txsize an extra vbyte (Jon Atack)
Pull request description:
Hopefully closes#18511 by allowing the transaction size to be 140-141 vbytes rather than strictly 141, and bumps with a slightly larger fee to ensure dust in the 140 vbyte case.
ACKs for top commit:
jnewbery:
utACK 25e03ba1ff
Tree-SHA512: 76a04e1ce090e48befe048ed6d412222d7f8bc951ff822850833061a0606b1bebc5289f7249737d3fb9aa26eb857f99543981037cea6babe3e578e2cfe8afcdb
0ed2d8e07d test: add BIP37 remote crash bug [CVE-2013-5700] test to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
Integrates the missing message type `filteradd` to the test framework and checks that the BIP37 implementation is not vulnerable to the "remote crash bug" [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700) anymore. Prior to v.0.8.4, it was possible to trigger a division-by-zero error on the following line in the function `CBloomFilter::Hash()`:
f0d6487e29/src/bloom.cpp (L45)
By setting a zero-length filter via `filterload`, `vData.size()` is 0, so the modulo operation above, called on any .insert() or .contains() operation then crashed the node. The test uses the approach of just sending an arbitrary `filteradd` message after, which calls `CBloomFilter::insert()` (and in turn `CBloomFilter::Hash()`) on the node. The vulnerability was fixed by commit 37c6389c5a (an intentional covert fix, [according to gmaxwell](https://github.com/bitcoin/bitcoin/issues/18483#issuecomment-608224095)), which introduced flags `isEmpty`/`isFull` that wouldn't call the `Hash()` member function if `isFull` is true (set to true by default constructor).
To validate that the test fails if the implementation is vulnerable, one can simply set the flags to false in the member function `UpdateEmptyFull()` (that is called after a filter received via `filterload` is constructed), which activates the vulnerable code path calling `Hash` in any case on adding or testing for data in the filter:
```diff
diff --git a/src/bloom.cpp b/src/bloom.cpp
index bd6069b..ef294a3 100644
--- a/src/bloom.cpp
+++ b/src/bloom.cpp
@@ -199,8 +199,8 @@ void CBloomFilter::UpdateEmptyFull()
full &= vData[i] == 0xff;
empty &= vData[i] == 0;
}
- isFull = full;
- isEmpty = empty;
+ isFull = false;
+ isEmpty = false;
}
```
Resulting in:
```
$ ./p2p_filter.py
[...]
2020-04-03T14:38:59.593000Z TestFramework (INFO): Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed
2020-04-03T14:38:59.695000Z TestFramework (ERROR): Assertion failed
[...]
[... some exceptions following ...]
```
ACKs for top commit:
naumenkogs:
utACK 0ed2d8e07d
Tree-SHA512: 02d0253d13eab70c4bd007b0750c56a5a92d05d419d53033523eeb3ed80318bc95196ab90f7745ea3ac9ebae7caee3adbf2a055a40a4124e0915226e49018fe8
4670006762 test: remove redundant sync_with_ping after add_p2p_connection (Jon Atack)
Pull request description:
Now that #18247 is merged, these calls are redundant.
ACKs for top commit:
vasild:
utACK 4670006
Tree-SHA512: bdbfe8bcf9dbdde0a8115e3a62bfe359910798d7a3010d920ffca07049cb5f97bf8fb9b6f70079b0607105192b61a6d665774e59a2b678597b47ad6237595ad5
3dc8c012f0 test: remaining replacements of (send_message+sync_with_ping) with send_and_ping (Sebastian Falbesoner)
Pull request description:
This is a tiny follow-up PR to #18494, substituting the remaining occurences of `send_message(...)`/`sync_with_ping(...)` pairs with `send_and_ping(...)`, as suggested in the comment https://github.com/bitcoin/bitcoin/pull/18494#pullrequestreview-386418913. Thanks to jonatack and [MarcoFalke](https://github.com/bitcoin/bitcoin/pull/18494#issuecomment-608496342) for giving me the hint to do this follow-up.
ACKs for top commit:
practicalswift:
ACK 3dc8c012f0
Tree-SHA512: 44d64332933c23a7f59c0415e008ce1b2b2e07177f81cb9473b7c71558188f1c698e8973de5cc940280e4697f9553af852d9a42841304f82469673d1c8162852
6112a20982 test: replace (send_message + sync_with_ping) with send_and_ping (Jon Atack)
Pull request description:
This is a follow-up to faf1d04731 yesterday.
ACKs for top commit:
vasild:
utACK 6112a20
MarcoFalke:
ACK 6112a20982 🎞
Tree-SHA512: 749644ac9a1ef0e1aa6c3ac5e899eb3fa7fb9c0909352f922a80412df2bc0e539692a7757af550eff4d4914cbe57b0c75ce3948f569acc7a52852e91a55ad457
9eefc6e92f gui: Delete progress dialog instead of hidding it (João Barbosa)
ee9e88ba27 wallet: Handle duplicate fileid exception (João Barbosa)
Pull request description:
Handle the duplicate fileid exception thrown at `CheckUniqueFileid` in tow cases:
- when duplicate wallets are set on the command line - catch in `LoadWallets`;
- when a duplicate wallet is loaded dynamically - catch in `LoadWallet`.
Fixes#16776.
ACKs for top commit:
jonatack:
Re-ACK 9eefc6e92f no change since last review 68e0ff0e1f530c942721aab49cf67ffc07104628
hebasto:
re-ACK 9eefc6e92f
Tree-SHA512: 46e3c1cd6708b54e2d1c4973a74c8d5428822e04cecbc147cf200eb034efa385e867bd749c7c639020e83c9813fae8fed64a851bdd99abf60c33b07e0363f5d5
ParsePrevouts uses GetScriptForWitness on the given witnessScript
to find the corresponding redeemScript. This is incorrect when the
witnessScript is either a P2PK or P2PKH script as it returns the
corresponding P2WPK script instead of turning the witnessScript
into a P2WSH script. Instead this should make the script a
WitnessV0ScriptHash destination and get the script for that.
Test cases are also added.
fac3716b09 test: check that peer is connected when calling sync_* (MarcoFalke)
Pull request description:
Without a connection there is no way to sync, so we can fail early and don't have to wait for the timeout
ACKs for top commit:
jonatack:
ACK fac3716b09
Tree-SHA512: 12f771473c23e152dae4bfb201fadb2c3530cf439de64fea07d048734614543080a5d05c9c36e6e398c6a69c8279f609d34706599571814172a11bcfbea4a3b9
0055922958 test: add BIP37 'filterclear' test to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
Integrates the message type `filterclear` to the test framework and adds a simple test to `p2p_filter.py`, checking that arbitrary txs get relayed again after deleting the filter.
ACKs for top commit:
naumenkogs:
utACK 0055922958
Tree-SHA512: fe64e99a526865770707d8077b9968d3923f248045ec7fa56cd380dba85ac77a71a473d244ef3aede2fc0d287b8d7c6bc0156b6033b0c949c2058cc08e255697
83e1d92413 test: listsinceblock block height checks (Jon Atack)
Pull request description:
This is the second commit of #17535.
This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in #17437. It also cleans up code in the test function without changing or removing existing checks.
ACKs for top commit:
fjahr:
tested ACK 83e1d92413
ryanofsky:
Code review ACK 83e1d92413. Nice test improvements!
Tree-SHA512: 92874b49a3bc0236500495f32dfcf683e1971ca3d4c51702c69ed4ce7dfce21273754f02f93d1243d73793701d9fdf49e14b149477cd249cbbd9e4e8d5bd49f8
fa15699969 test: Add basic test for BIP 37 (MarcoFalke)
Pull request description:
This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.
ACKs for top commit:
practicalswift:
Code review ACK fa15699969 -- more testing coverage is better than less testing coverage
Tree-SHA512: d52e8be79240dffb769105c087ae0ae9305d599282546e4ca7379c4c7add2dbcd668265b46670aa07c357638044cf0f61a6fab7dba8971dd0f80c8f99768686e
dcda81c471 test: add coverage for script parse error in ParseScript (pierrenn)
Pull request description:
Follow up on this suggestion : https://github.com/bitcoin/bitcoin/pull/18416#issuecomment-603966799
This adds a test case to raise the `script parse error` in `ParseScript`.
ACKs for top commit:
instagibbs:
utACK dcda81c471
Tree-SHA512: ae0ef2c00f34cee818c83582f190d5f4043159e922862f2b442b7b895b8ff3ca421533699247c12c367be77813b5205830a771cd47a18e8932807ccace2d6a1c
9ab14e4d21 Limit decimal range of numbers ParseScript accepts (pierrenn)
Pull request description:
Following up on this suggestion : https://github.com/bitcoin/bitcoin/pull/18413#issuecomment-602966490, prevent the output of `atoi64` in the `core_read.cpp:ParseScript` helper to send to `CScriptNum::serialize` values wider than 32-bit.
Since the `ParseScript` helper is only used by the tool defined in `bitcoin-tx.cpp`, this only prevents users to provide too much unrealistic values.
ACKs for top commit:
laanwj:
ACK 9ab14e4d21
Tree-SHA512: ee228269d19d04e8fee0aa7c0ae2bb0a2b437b8e574356e8d9b2279318242057d51fcf39a842aa3afe27408d0f2d5276df245d07a3f4828644a366f80587b666
c3857c5fcb wallet: remove CreateTotalBumpTransaction() (Jon Atack)
4a0b27bb01 wallet: remove totalfee from createBumpTransaction() (Jon Atack)
e347cfa9a7 rpc: remove deprecated totalFee arg from RPC bumpfee (Jon Atack)
bd05f96d79 test: delete wallet_bumpfee_totalfee_deprecation.py (Jon Atack)
a6d1ab8caa test: update bumpfee testing from totalFee to fee_rate (Jon Atack)
Pull request description:
Since 0.19, fee-bumping using `totalFee` was deprecated in #15996 and replaced by `fee_rate` in #16727. This changeset removes it.
ACKs for top commit:
laanwj:
ACK c3857c5fcb
Tree-SHA512: c1bb15d664baf4d2dea06981f36384af02057d125c51fcbc8640b9d5563532187c7b84aa952f7b575255a88ce383ed4d7495bec920a47b05b6fc0d432dce1f00
d056df033a Replace std::to_string with locale-independent alternative (Ben Woosley)
Pull request description:
Addresses #17866 following practicalswift's suggestion:
https://github.com/bitcoin/bitcoin/issues/17866#issuecomment-584287299
~Used ::ToString to avoid aliasing issues. Left uses in QT and test.~
ACKs for top commit:
practicalswift:
ACK d056df033a
laanwj:
ACK d056df033a
Tree-SHA512: 9e6966a9cdd14f4a1a40d9f0fa7c402aed22b2f1ad8681708e22b050d51a91c5d62220a9ec4c425be2d57acf5c964fca87a5e981b5cbff048bc3b6720dae92b7
fa92af5af3 ci: Run feature_block and feature_abortnode in valgrind (MarcoFalke)
fa01febeaf test: Remove ci timeout restriction in test_runner (MarcoFalke)
Pull request description:
Also revert commit 0a4912e46a, because some tests take too long for this to be useful anymore.
Top commit has no ACKs.
Tree-SHA512: 363f14766e1f4a5860ab668a516b41acebc6fbdf11d8defb3a95a772dbf82304ca1f5f14b1dbad97f2029503e03d92e8c69df0466a8872409c20665838f617ed
fa3fa27c45 fuzz: Remove option --export_coverage from test_runner (MarcoFalke)
aaaa055ff7 fuzz: Add option to merge input dir to test runner (MarcoFalke)
fa4fa88d76 doc: Remove --disable-ccache from docs (MarcoFalke)
Pull request description:
This is mainly useful for myself to merge pull requests like https://github.com/bitcoin-core/qa-assets/pull/4
I thought it wouldn't hurt to share the code.
Also remove the `--disable-ccache` from the docs to speed up builds when developing fuzzers.
Top commit has no ACKs.
Tree-SHA512: 818d85a90db86a7f4e8b001cc88342e5b28b02029d2bd4174440b28a8c4cc29b5406bd6348f72ddf909bb3d0f9bf7b1011976f6480e4418c8b7da5ecccae93e8
3ed772d221 [tests] remove bignum.py (John Newbery)
f950ec2520 [tests] remove bn2bin() (John Newbery)
3b9b38579c [tests] remove bn_bytes() function (John Newbery)
a760aa14a9 [tests] remove mpi2vch() function (John Newbery)
9a60bef50d [tests] don't encode the integer size in bignum (John Newbery)
1dc68aee66 [tests] add function comments to bignum (John Newbery)
f31fc0e92e [tests] fix flake8 warnings in script.py and bignum.py (John Newbery)
Pull request description:
Only one function is imported in script.py. Just move that function to script.py and remove the bignum.py module.
Remove unused functionality and fix some flake8 warnings along the way.
Top commit has no ACKs.
Tree-SHA512: 015f543ab545b5d5451896e2751d9c19334d9155b03faacd2023781e89833a2440f7f28741e9a8ac49badd9cdc012cbb6e038cdcdebeefaf9cb9d461c0689157
7a6627ae87 Fix mining to an invalid target + ensure that a new block has the correct hash internally in Python tests (Samer Afach)
Pull request description:
Test with block 47 in the `feature_block.py` creates a block with a hash higher than the target, which is supposed to fail. Now two issues exist there, and both have low probability of showing up:
1. The creation is done with `while (hash < target)`, which is wrong, because hash = target is a valid mined value based on the code in the function `CheckProofOfWork()` that validates the mining target:
```
if (UintToArith256(hash) > bnTarget)
return false;
```
2. As we know the hash stored in CBlock class in Python is stateful, unlike how it's in C++, where calling `CBlock::GetHash()` will actively calculate the hash and not cache it anywhere. With this, blocks that come out of the method `next_block` can have incorrect hash value when `solve=False`. This is because the `next_block` is mostly used with `solve=True`, and solving does call the function `rehash()` which calculates the hash of the block, but with `solve=False`, nothing calls that method. And since the work to be done in regtests is very low, the probably of this problem showing up is very low, but it practically happens (well, with much higher probability compared to issue No. 1 above).
This PR fixes both these issues.
Top commit has no ACKs.
Tree-SHA512: f3b54d18f5073d6f1c26eab89bfec78620dda4ac1e4dde4f1d69543f1b85a7989d64c907e091db63f3f062408f5ed1e111018b842819ba1a5f8348c7b01ade96
faf6f156ff test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
Pull request description:
The wallet rebroadcast functionality learns about new blocks via the validation interface queue. To avoid test failures such as https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31119387#L466 , we can sync with the queue before advancing the test.
ACKs for top commit:
jonatack:
ACK faf6f156 this makes sense; the fix was previously added to mempool_persist.py and wallet_zapwallettxes.py in #12217 and to wallet_balance.py in #16302. It is also used in src/test/validation_block_tests.cpp (processnewblock_signals_ordering) and src/bench/wallet_balance.cpp.
Tree-SHA512: d72fd4b597b669d8111007902b523e946712913cd6eea6f9a695b0f04ecbe2321d05019873af999a95b9e0aa0f5c140a17109b37503723e40c9eab24ec358eb7
fa2cf85e6f test: Fix race in p2p_segwit (MarcoFalke)
Pull request description:
Fixes#11696
Top commit has no ACKs.
Tree-SHA512: 09de07ea26236547586f5c373a0df2b68d84af5cfa8f40bd2ca9f951fc083c5f4b8a472a60668d99118bbd9f3942ec3d6a34f05944d47345acca41c95475bb27
111880aaf7 [test] Add coverage to estimaterawfee and estimatesmartfee (Ben Woosley)
Pull request description:
This adds light functional coverage to estimaterawfee - a subset of
the testing applied to estimatesmartfee, and argument validation
testing to both estimaterawfee and estimatesmartfee.
One valid estimatesmartfee signature test is commented out because it
fails currently.
Extracted from #12940
Top commit has no ACKs.
Tree-SHA512: 361a883457b28b2dc75081666e49d6dc6b5d76eed40d858abe2dd4f35ece152cf1f99c94480a91f42a896aa2a73cf55f57921316fe66970b2d7ba691a3b17e2d
4444edc2e6 ci: Enable all functional tests in valgrind (MarcoFalke)
Pull request description:
The travis timeout for our repo has been bumped to 2h, so we can run all tests in valgrind now
ACKs for top commit:
practicalswift:
ACK 4444edc2e6 -- regarding the three disabled cases (`feature_abortnode`, `feature_block` and `rpc_bind`): not a big deal since MSan will take care of those once #18288 is merged. More is more :)
Tree-SHA512: ea2f798112911b6d1f3d88cfcdf0a7cdb698687248343703d6fe55da144542c961c15d888bffb41672c10aa76765615cb7c7ff93d468bfad3c51f962f24e7abb
f1b4503114 bumpfee test: exit loop at proper time with new fee value being compared (Gregory Sanders)
2e4edc68f9 Add some test logging to wallet_bumpfee.py (Gregory Sanders)
Pull request description:
In the loop we accidentally used `origfee` which is not the value to check, and also allowed the loop to exit too early since the new fee must be strictly greater than `0.0005`.
Also converted/added a bunch of logging from comments.
Resolves https://github.com/bitcoin/bitcoin/issues/17716
ACKs for top commit:
MarcoFalke:
ACK f1b4503114🏈
Tree-SHA512: eb73297fc82b09b9ec08d85ba3f0bec662119d0ff63ccf5d978a7bad6a674b5915f5ed021ec42f72a732c9ee7af43212d1de87361f50a970df7755caec96f6d8
ffff9dcdc3 test: Explain why test logging should be used (MarcoFalke)
Pull request description:
Background is that some tests don't have any `self.log` call at all. Thus there are no "anchor points" and those tests are hard to debug because the logs can't easily be parsed by a human.
ACKs for top commit:
jonatack:
ACK ffff9dcdc3
instagibbs:
ACK ffff9dcdc3
fanquake:
re-ACK ffff9dcdc3
Tree-SHA512: 08d962e85c4892c2a0c58feb5dc697c680a9d68e41a79417da6fcd415e0c5c735c4533a985cf225bb89deb5ca717d9bedf990657958079185804caa512b10f5a
70a6b529f3 lint-cppcheck: Remove -DHAVE_WORKING_BOOST_SLEEP_FOR (Anthony Towns)
294937b39d scheduler_tests: re-enable mockforward test (Anthony Towns)
cea19f6859 Drop unused reverselock.h (Anthony Towns)
d0ebd93270 scheduler: switch from boost to std (Anthony Towns)
b9c4260127 sync.h: add REVERSE_LOCK (Anthony Towns)
306f71b4eb scheduler: don't rely on boost interrupt on shutdown (Anthony Towns)
Pull request description:
Replacing boost functionality with C++11 stuff.
Motivated by #18227, but should stand alone. Changing from `boost::condition_var` to `std::condition_var` means `threadGroup.interrupt_all` isn't enough to interrupt `serviceQueue` anymore, so that means calling `stop()` before `join_all()` is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from `g_lockstack` which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour.
Fixes#16027, Fixes#14200, Fixes#18227
ACKs for top commit:
laanwj:
ACK 70a6b529f3
Tree-SHA512: d1da13adeabcf9186d114e2dad9a4fdbe2e440f7afbccde0c13dfbaf464efcd850b69d3371c5bf8b179d7ceb9d81f4af3cc22960b90834e41eaaf6d52ef7d331
Changes from boost::chrono to std::chrono, boost::condition_var to
std::condition_var, boost::mutex to sync.h Mutex, and reverselock.h to
sync.h REVERSE_LOCK. Also adds threadsafety annotations to CScheduler
members.
9ff41f6419 tests: Add float to FUZZERS_MISSING_CORPORA (temporarily) (practicalswift)
8f6fb0a85a tests: Add serialization/deserialization fuzzing for integral types (practicalswift)
3c82b92d2e tests: Add fuzzing harness for functions taking floating-point types as input (practicalswift)
c2bd588860 Add missing includes (practicalswift)
Pull request description:
Add simple fuzzing harness for functions with floating-point parameters (such as `ser_double_to_uint64(double)`, etc.).
Add serialization/deserialization fuzzing for integral types.
Add missing includes.
To test this PR:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/float
…
```
Top commit has no ACKs.
Tree-SHA512: 9b5a0c4838ad18d715c7398e557d2a6d0fcc03aa842f76d7a8ed716170a28f17f249eaede4256998aa3417afe2935e0ffdfaa883727d71ae2d2d18a41ced24b5
2a95c7c956 ci: Check for submodules (Emil Engler)
Pull request description:
See #18019.
The current solution looks like this (I also tested with multiple submodules):
```
These submodules were found, delete them:
355a5a310019659d9bf6818d2fd66fbb214dfed7 curl (curl-7_68_0-108-g355a5a310)
```
The submodule example command was `git submodule add https://github.com/curl/curl.git curl`
ACKs for top commit:
laanwj:
ACK 2a95c7c956
Tree-SHA512: 64bf388123f0a88d12e3e41ff29bc190339377a0615c35dc3f2700bb7773470a8fa426e0ff57188a60ed88bded39f75082ff0b73118651ff403b163422395005
1ba3e1cc21 init: move asmap code earlier in init process (Jon Atack)
5ba829e12e rpc: fix getpeerinfo RPCResult `mapped_as` type (Jon Atack)
c90b9a2399 net: extract conditional to bool CNetAddr::IsHeNet (Jon Atack)
819fb5549b logging: asmap logging and #include fixups (Jon Atack)
dcaf543ba0 test: add functional test for an empty, unparsable asmap (Jon Atack)
b8d0412b21 config: separate the asmap finding and parsing checks (Jon Atack)
81c38a2497 config: enable passing -asmap an absolute file path (Jon Atack)
fbe9b024f0 config: use default value in -asmap config (Jon Atack)
08b992675c test: add feature_asmap functional tests (Jon Atack)
Pull request description:
This PR builds on PR #16702 to add functional tests / sanity checks and user-facing refinements for passing `-asmap` to configure ASN-based IP bucketing in addrman. As per our review discussion in that PR, the idea here is to handle aspects like functional tests and config arg handling that can help the PR be merged while enabling the author to focus on the bucketing itself.
- [x] add feature functional tests to verify node behaviour and debug log output when launching
- `bitcoind` with no `-asmap` arg
- `bitcoind -asmap=RELATIVE_FILENAME` to the unit test data skeleton asmap
- `bitcoind -asmap` with no filename specified using the default asmap file
- `bitcoind -asmap` with no filename specified and a missing default asmap file
- [x] add the ability to pass absolute path filenames to the `-asmap` config arg in addition to datadir-relative path filenames as per https://github.com/bitcoin/bitcoin/pull/16702#discussion_r361300447, and add test coverage
- [x] separate the asmap file finding and parsing checks, which allows adding tests for the case of a found but unparseable or empty asmap
- [x] add test for an empty asmap
- [x] various asmap fixups
- [x] move the asmap init code earlier in the init process to provide immediate feedback when passing an `-asmap` config arg. This speeds up the `feature_asmap` functional test from 60 to 5 seconds! Credit to Wladimir J. van der Laan for the suggestion.
ACKs for top commit:
practicalswift:
ACK 1ba3e1cc21 -- diff looks correct
fanquake:
ACK 1ba3e1cc21
Tree-SHA512: e9094460a597ac5597449acfe631c87b71d3ede6a12c7ae61b26d1161b3eefed8e7e25c4fb0505864cebd89300b7c4cf9378060aa9155441029315df15fa3283
fa6df0de53 test: Bump timeouts to accomodate really slow disks (MarcoFalke)
Pull request description:
Needed these patches locally for some arm machines with slow storage
ACKs for top commit:
practicalswift:
ACK fa6df0de53
fanquake:
ACK fa6df0de53
Tree-SHA512: 22f2f6f7ed05f26013431126bb179b029dbc931f02d0e58f8970c6d477f43e3106d76c9732942034cb2cfcb827191e338a082f953ccb69531a19ee6dab9a7e1a
and update feature_asmap.py and test_runner.py
This commit moves the asmap init.cpp code from the end of "Step 12: start node"
to "Step 6: network initialization" to provide feedback on passing an -asmap
config arg much more quickly. This change speeds up the feature_asmap.py
functional test file from 60 to 5 seconds by accelerating the 2 tests that use
`assert_start_raises_init_error`.
Credit to Wladimir J. van der Laan for the suggestion.
- move asmap #includes to sorted positions in addrman and init (move-only)
- remove redundant quotes in asmap InitError, update test
- remove full stops from asmap logging to be consistent with debug logging,
update tests
to verify node behaviour and debug log when launching bitcoind in these cases:
1. `bitcoind` with no -asmap arg, using /16 prefix for IP bucketing
2. `bitcoind -asmap=<relative path>`, using the unit test skeleton asmap
3. `bitcoind -asmap/-asmap=` with no file specified, using the default asmap
4. `bitcoind -asmap` with no file specified, and a missing default asmap file
The tests are order-independent. The slowest test (missing default asmap file)
is placed last.
1ef28b4f7c Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)
Pull request description:
Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220
Sjors documenting the issue:
```
A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))
{
"inputs": [
{
"has_utxo": true,
"is_final": false,
"next": "finalizer"
}
],
"estimated_vsize": 141,
"estimated_feerate": 1e-05,
"fee": 1.41e-06,
"next": "signer"
}
I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
```
It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.
Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.
ACKs for top commit:
Sjors:
ACK 1ef28b4f7c, much nicer. Don't forget to document the bug fix.
achow101:
ACK 1ef28b4f7c
Empact:
ACK 1ef28b4f7c
Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
this also fixes a bug that was uncovered with this checks:
for the BIP112 version 1 tx tests, certain txs (bip112txs_vary_OP_CSV_v1) have
been sent twice due to a typo, leading also to a failure as expected but for the
wrong reason
With BIP112 activated, the operation OP_CHECKSEQUENCEVERIFY (former OP_NOP3)
leads to script interpreter termination with an error if one of the following
conditions is true:
-> stack is empty
-> top item on stack is negative (< 0)
-> top item on stack has disable flag unset and at least one of
four other conditions is true (contains the core CSV logic)
This commits adds the missing empty stack failure test to the functional test
by prepending a valid scriptSig with just OP_CHECKSEQUENCEVERIFY. If BIP112 is
inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and
the transaction remains valid -- if it is active, the tx is invalid due to an
empty stack (for both tx versions 1 and 2, as well).
fa45d60646 test: Reduce unneeded whitelist permissions in tests (MarcoFalke)
Pull request description:
It makes the tests confusing and fragile when overwriting default command line values that are not needed to be overwritten.
ACKs for top commit:
fanquake:
ACK fa45d60646
laanwj:
ACK fa45d60646
Tree-SHA512: 8ae5ad8c6be156b1a983adccbca8d868ef841e00605ea88e24227f1b7493987c50b3e62e68dd7dc785ad73d6e14279eb13d7a151cb0a976426fe2fd63ce5cbcd
b902bd66b0 test: check custom descendant limit in mempool_packages.py (Sebastian Falbesoner)
Pull request description:
This is a follow-up PR to #17435, testing the custom descendant limit, passed by the argument `-limitdescendantcount`. ~~It was more tricky than expected, mainly because we don't know for sure at which point node1 has got all the transactions broadcasted from node0 (for the ancestor test this wasn't a problem since the txs were immediately available through `invalidateblock`) -- a simple `sync_mempools()` doesn't work here since the mempool contents are not equal due to different ancestor/descendant limits. Hence I came up with a "hacky manual sync":~~
1. ~~wait until the mempool has the _expected_ tx count (see conditions below)~~
2. ~~after that, wait some time and get sure that the mempool contents haven't changed in-between~~
~~Like for~~ Similar to the ancestor test, we overall check for ~~three~~ four conditions:
- the # of txs in the node1 mempool is equal to the descendant limit (plus 1 for the parent tx, plus the # txs from the previous ancestor test which are still in) ~~(done by the hacky sync above)~~
- all txs in node1 mempool are a subset of txs in node0 mempool
- part of the constructed descendant-chain (the first ones up to the limit) are contained in node1 mempool
- the remaining part of the constructed descendant-chain (all after the first ones up to the limit) is *not* contained in node1 mempool
ACKs for top commit:
JeremyRubin:
Excellent. utACK b902bd6
Tree-SHA512: 7de96dd248f16ab740e178ac5b64b57ead18cdcf74adfe989709d215e4a67b6b6d20de22c48e885d5f2edc55caaddd44a4261e996c5c87687ceb6a47f1d1fdaf
To test the custom descendant limit on node1 (passed by the argument
-limitdescendantcount), we check for four conditions:
-> the # of txs in the node1 mempool is equal to the limit
(plus 1 for the parent tx, plus the # txs from the previous ancestor
test which are still in)
-> all txs in node1 mempool are a subset of txs in node0 mempool
-> part of the constructed descendant-chain (the first ones up to the
limit) are contained in node1 mempool
-> the remaining part of the constructed descendant-chain (all after the
first ones up to the limit) is *not* contained in node1 mempool
facb71576c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b346, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
5bad7921d0 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9061 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
d6d2602a32 add: test that transactions expire from mempool (0xb10c)
Pull request description:
This adds the functional test `mempool_expiry.py` covering mempool transaction expiry. Both the default `DEFAULT_MEMPOOL_EXPIRY` of 336 hours (two weeks, set in #9312) and the user definable mempool expiry via the `-mempoolexpiry=<n>` command line option are tested. The test checks that descendants of expired transactions are removed as well.
*Notes for reviewers*
- `LimitMempoolSize()` (which is the only caller of `CTxMemPool::Expire()`) is only called when a transaction is added to the mempool. In order to test expiry of a transaction-that-should-expire, the mocktime is set and a random transaction is broadcast to trigger `LimitMempoolSize()`. The transaction-that-should-expire is then checked for expiry. LMK if there is another way, but I don't think there is.
ACKs for top commit:
MarcoFalke:
ACK d6d2602a32
theStack:
ACK d6d2602a32
promag:
Code review ACK d6d2602a32.
Tree-SHA512: eb68cd9e2d870872b8e8e1522fed8954fb99cc9e4edda4b28bb2a4e41cddbc53fe6f7d9c090f1e0e98ab49beb24bf37ff3787a9e9801a95e8ae9ca9eb34fe6f0
