b21e522ce4 test: speedup wallet tests by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
In the course of testing #25297 by running all wallet-related functional tests (see https://github.com/bitcoin/bitcoin/pull/25297#issuecomment-1203365589), I noticed that the run-time of those tests vary a lot between runs, in fact too much for a useful comparison. This PR fixes this by making the tests both more deterministic and also faster, using the good ol' immediate tx relay trick (parameter `-whitelist=noban@127.0.0.1`).
master branch:
```
wallet_abandonconflict.py --descriptors | ✓ Passed | 7 s
wallet_abandonconflict.py --legacy-wallet | ✓ Passed | 23 s
wallet_balance.py --descriptors | ✓ Passed | 17 s
wallet_balance.py --legacy-wallet | ✓ Passed | 21 s
wallet_basic.py --descriptors | ✓ Passed | 32 s
wallet_basic.py --legacy-wallet | ✓ Passed | 56 s
wallet_bumpfee.py --descriptors | ✓ Passed | 44 s
wallet_bumpfee.py --legacy-wallet | ✓ Passed | 45 s
wallet_groups.py --descriptors | ✓ Passed | 89 s
wallet_groups.py --legacy-wallet | ✓ Passed | 94 s
wallet_hd.py --descriptors | ✓ Passed | 7 s
wallet_hd.py --legacy-wallet | ✓ Passed | 13 s
wallet_importdescriptors.py --descriptors | ✓ Passed | 26 s
wallet_listreceivedby.py --descriptors | ✓ Passed | 28 s
wallet_listreceivedby.py --legacy-wallet | ✓ Passed | 18 s
ALL | ✓ Passed | 520 s (accumulated)
Runtime: 526 s
```
PR branch:
```
wallet_abandonconflict.py --descriptors | ✓ Passed | 7 s
wallet_abandonconflict.py --legacy-wallet | ✓ Passed | 11 s
wallet_balance.py --descriptors | ✓ Passed | 8 s
wallet_balance.py --legacy-wallet | ✓ Passed | 8 s
wallet_basic.py --descriptors | ✓ Passed | 29 s
wallet_basic.py --legacy-wallet | ✓ Passed | 36 s
wallet_bumpfee.py --descriptors | ✓ Passed | 39 s
wallet_bumpfee.py --legacy-wallet | ✓ Passed | 32 s
wallet_groups.py --descriptors | ✓ Passed | 39 s
wallet_groups.py --legacy-wallet | ✓ Passed | 41 s
wallet_hd.py --descriptors | ✓ Passed | 8 s
wallet_hd.py --legacy-wallet | ✓ Passed | 11 s
wallet_importdescriptors.py --descriptors | ✓ Passed | 17 s
wallet_listreceivedby.py --descriptors | ✓ Passed | 7 s
wallet_listreceivedby.py --legacy-wallet | ✓ Passed | 9 s
ALL | ✓ Passed | 302 s (accumulated)
Runtime: 309 s
```
Note that an alternative approach could be to whitelist peers by default for nodes in the functional test framework and only enable the trickle relay for the few tests where it's really needed.
ACKs for top commit:
naumenkogs:
utACK b21e522ce4
Tree-SHA512: ac3c8f8f5a401d1b6af60ece9c77e72449f18920c2cb4a1bd65fb4d62cf428779ebf4e1d29009a882977b2252922df4e7183541e0da8de932f8cd479149e8a86
Passing a list of transactions `txlist` to `create_block` appends
them to the block, hence we don't need to do that manually anymore.
The merkle root calculation can also be removed, since this is done
in the end of the helper.
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
This commit adds missing test coverage for the bumpfee RPC
error "Transaction has descendants in the mempool", which is
thrown if the bumped tx has descendants in the mempool and is
_not_ connected to the bitcoin wallet. To achieve that, the
test framework's MiniWallet is used.
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
as the feeRate argument should soon be deprecated.
Also loosen one test (and a similar one) that caused a one-off CI failure with:
expected message
'Insufficient total fee 0.00000141, must be at least 0.00001704 (oldFee 0.00000999 + incrementalFee 0.00000705)'
actual message
'Insufficient total fee 0.00000141, must be at least 0.00001712 (oldFee 0.00001007 + incrementalFee 0.00000705)'
Create a fee_rate (sat/vB) RPC param and replace overloading the conf_target and
estimate_mode params in the following 6 RPCs with it:
- sendtoaddress
- sendmany
- send
- fundrawtransaction
- walletcreatefundedpsbt
- bumpfee
In RPC bumpfee, the previously existing fee_rate remains but the unit is changed
from BTC/kvB to sat/vB. This is a breaking change, but it should not be an
overly risky one, as the units change by a factor of 1e5 and any fees specified
in BTC/kvB after this commit will either be too low and raise an error or be 1
sat/vB and can be RBFed.
Update the test coverage for each RPC.
Co-authored-by: Murch <murch@murch.one>
0be29000c0 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be406 wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c005083 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa603 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1 wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f84 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1 wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack)
Pull request description:
Follow-up to #11413 providing a base to build on for #19543:
- bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219
- adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany`
- improves a few related RPC error messages and `ParseConfirmTarget()` / error message
- fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units
This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21.
ACKs for top commit:
kallewoof:
Concept/Tested ACK 0be29000c0
meshcollider:
Code review + functional test run ACK 0be29000c0
Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
-BEGIN VERIFY SCRIPT-
sed -i 's/Fee exceeds maximum configured by \-\maxtxfee/Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)/g' src/util/error.cpp test/functional/rpc_fundrawtransaction.py test/functional/rpc_psbt.py test/functional/rpc_rawtransaction.py test/functional/wallet_bumpfee.py test/functional/wallet_create_tx.py
-END VERIFY SCRIPT-
79d6332e9e moveonly: Fix indentation in bumpfee RPC (Andrew Chow)
431071c28a Hide bumpfee's psbt creation behavior behind -deprecatedrpc (Andrew Chow)
4638224f64 Add psbtbumpfee RPC (Andrew Chow)
Pull request description:
Adds a new RPC `psbtbumpfee` which always creates a psbt. `bumpfee` will then only be able to create and broadcast fee bumping transactions instead of changing its behavior based on `IsWalletSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)`.
Split from #18627
ACKs for top commit:
Sjors:
re-utACK 79d6332
meshcollider:
utACK 79d6332e9e
fjahr:
Code review ACK 79d6332e9e
Tree-SHA512: 1c92c4b4461bb30e78be3ee73165f624398ef33996ce36043b61a8931be667030d0fca12fd0b30097b78c56e4e9092c69582b237cbdac51d56f6be23d8c0f1bb
substitutes "for x in range(N):" by "for _ in range(N):"
indicates to the reader that a block is just repeated N times, and
that the loop counter is not used in the body
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
