24d2d3341d07509ad3f37bb6f130446ad20ac807 QA: wallet_multiwallet: Check that recursive symlink directory and wallet.dat loops are ignored (Luke Dashjr)
69f59af54d15ee9800d5df86bcdb0e962c71e7c3 Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks (Luke Dashjr)
Pull request description:
Previously, an exception would be thrown, which could kill the node in some circumstances.
Includes test changes to cause failure.
Review with `?w=1`
ACKs for top commit:
hebasto:
re-ACK 24d2d3341d07509ad3f37bb6f130446ad20ac807, rebased only since my [previous](https://github.com/bitcoin/bitcoin/pull/19502#pullrequestreview-520552944) review.
promag:
Tested ACK 24d2d3341d07509ad3f37bb6f130446ad20ac807, test change fails on master.
meshcollider:
utACK 24d2d3341d07509ad3f37bb6f130446ad20ac807
Tree-SHA512: f701f81b3aa3d3e15cee52ac9e7c31a73c0d8166e56bf077235294507cbcee099829fedc432a1c4b6d8780885f4e37897b44b980b08125771de3c849c000499e
5e146022daa4336de94447e5b8e5418296286927 wallet: fix scanning progress calculation for single block range (Sebastian Falbesoner)
Pull request description:
If the blockchain is rescanned for a single block (i.e. start and stop hashes are equal, and with that also the estimated start/stop verification progress values) the progress calculation could lead to a NaN value caused by a division by zero (0.0/0.0), resulting in an invalid JSON result for the `getwalletinfo` RPC. This PR fixes this behaviour by setting the progress to zero in that special case. Fixes #20297.
The behaviour can easily be reproduced by continuously running single block rescans in an endless loop, e.g. via
```bash
#!/bin/bash
while true
do
bitcoin-cli rescanblockchain $(bitcoin-cli getblockcount)
done
```
and at the same time perform some `getwalletinfo` RPCs.
On the master branch, this leads to frequent invalid responses (tested on mainchain):
```
$ bitcoin-cli getwalletinfo
error: couldn't parse reply from server
$ curl --user `cat ~/.bitcoin/.cookie` --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getwalletinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
{"result":{"walletname":"","walletversion":169900,"format":"bdb","balance":0.00000000,"unconfirmed_balance":0.00000000,"immature_balance":0.00000000,"txcount":0,"keypoololdest":1603677276,"keypoolsize":1000,"hdseedid":"3196e33ecb47c7130e6ca60f2f895f9259860dca","keypoolsize_hd_internal":1000,"paytxfee":0.00000000,"private_keys_enabled":true,"avoid_reuse":false,"scanning":{"duration":0,"progress":},"descriptors":false},"error":null,"id":"curltest"}
```
(note that missing value for "progress" in the JSON result).
On the PR branch, the behaviour doesn't occur anymore.
ACKs for top commit:
MarcoFalke:
review ACK 5e146022daa4336de94447e5b8e5418296286927
promag:
Core review ACK 5e146022daa4336de94447e5b8e5418296286927.
Tree-SHA512: f0e6aad5a6cd08b36c5fe820fff0ef26663229b39169a4dbe757f3c795a41cf5c69c9dc90efe7515675ae1059307f8971123781a0514d10704123a6f28b125ab
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>
If the blockchain is rescanned for a single block (i.e. start and stop hashes
are equal, and with that also the estimated verification progress) the progress
calculation could lead to a NaN value caused by a division by zero, resulting in
an invalid JSON result for the getwalletinfo RPC. Fixed by setting the progress
to zero in that special case.
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Files like config.site.in are not referenced by any other script in our
tree, so we need to mark it manually with a "shellcheck shell="
directive and make sure that shellcheck is run on them.
538be4219ae7e65862e4aff540af88c9421e6061 wallet: fix importdescriptor silent fail (Ivan Metlushko)
Pull request description:
Currently `importdescriptor` command will successfully import a descriptor with hardened derivations into a watch-only wallet while silently failing to expand the descriptor to fill the cache. This leads to a broken wallet state and failure to load such wallet due to missing cache on subsequent restart.
ACKs for top commit:
laanwj:
Code review ACK 538be4219ae7e65862e4aff540af88c9421e6061
achow101:
ACK 538be4219ae7e65862e4aff540af88c9421e6061
meshcollider:
utACK 538be4219ae7e65862e4aff540af88c9421e6061
Tree-SHA512: 4bdd0ab4437d55b3f1a79c3a300a0b186089155c020fe220a73d0cce274de47d90371d88918d39fd795f9fccf8db328f1e322d29a6062f9ce94a1c254398f004
bd93fc9945bfd4be117990c5d861f61ddd451f96 Fix change detection of imported internal descriptors (Andrew Chow)
Pull request description:
Import internal descriptors were having address book entries added which meant they would be detected as non-change. Fix this and add a test for it.
ACKs for top commit:
laanwj:
Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96
meshcollider:
utACK bd93fc9945bfd4be117990c5d861f61ddd451f96
promag:
Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96.
Tree-SHA512: 8fa9e364be317627ec171eedffdb505976c0e7f1e55bc7e8cfdffa3aeea5db24d231f55166602cd0e97a5ba621acc871de0a765c75d0c65678f83e93c3b657c5
fae45c34d13330442d5135cb9ac5a56dc697497b test: Only try witness deserialize when checking for witness deserialize failure (MarcoFalke)
Pull request description:
Witness deserialize will fail always. (This is what the test is checking for)
Consequently, non-witness deserialize is also tried, and it might succeed accidentally. Avoid that by not trying non-witness deserialize.
Fixes #20249
ACKs for top commit:
jnewbery:
utACK fae45c34d1
Tree-SHA512: 45e65b31603e3ca839776a7ed30e363b32eba20dfb67b7b55bff06715876850d4f6ba22f8ea4911a62e1f8ffff395bf187b23c46ddc766516b97057df000deb3
58cfbc38e040925b51cb8d35d23b50e9cf06fb2a Ignoring (but warn) on duplicate -wallet parameters (Jonas Schnelli)
Pull request description:
I expect that there are many users with load on startup wallet definitions in `bitcoin.conf` or via startup CLI argument.
With the new `settings.json` r/w configuration file, users unloading and loading a wallet through the GUI or via the RPC calls might end up with a duplicate `-wallet` entry (one that still remains in bitcoin.conf or CLI) plus the new duplication in `settings.json` due to the unload/load.
Steps to reproduce
* create wallet (if via RPC set `load_on_startup` or unloadwallet/loadwallet then set `load_on_startup`).
* stop bitcoin
* start bitcoind again with same `--wallet=mywallet`
I guess it is acceptable to skip duplicates.
ACKs for top commit:
achow101:
Tested ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
meshcollider:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a
ryanofsky:
Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a. Changes since previous review: rebased, tweaked warning message, squashed/fixed test
Tree-SHA512: f94e5a999bdd7dc291f0bc142911b0a8033929350d6f6a35b58c4a06a3c8f83147be0f0c402d4e946dedbbcc85b7e023b672c731b6d7a8984d4780017c961cfb
For upgrade tests and possibly other tests, it is useful to inspect the
bdb file for the wallet (i.e. the wallet.dat file).
test_framework/bdb.py is an implementation of bdb file deserialization
specific for Bitcoin Core's usage.
0be29000c011dec0722481dbebb159873da6fa54 rpc: update conf_target helps for correctness/consistency (Jon Atack)
778b9be40667876c705e586849ea9c9e44cf451c wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack)
603c0050837ec65765208dd54dde354145fbe098 wallet: add rpc send explicit fee rate coverage (Jon Atack)
dd341e602d5160fc621c0299179b91403756b61d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack)
44e7bfa60313e4ae67da49e5ba4535038b71b453 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack)
6e1ea4273e52fdcd86c87628aa595c03a071ca8c test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack)
3ac7b0c6f1c68e74a84d868a454f508bada6b09d wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack)
2d8eba8f8425a2515022d51f1f5b4911329fbf55 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack)
1697a40b6f841a54ee0d9744ed7fd09034b0ddad wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack)
fc5721723d34f76f9e1ffd2e31f274ea6b22f894 wallet: fix SetFeeEstimateMode() error message (Jon Atack)
052427eef1c9da84c474c5161b1910d3328ef0da 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 0be29000c011dec0722481dbebb159873da6fa54
meshcollider:
Code review + functional test run ACK 0be29000c011dec0722481dbebb159873da6fa54
Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64 [wallet] Return object from upgradewallet RPC (Sishir Giri)
Pull request description:
Change the return type of upgradewallet to be an object for future extensibility.
Also return any error string returned from the `UpgradeWallet()` function.
ACKs for top commit:
MarcoFalke:
ACK 2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64
meshcollider:
Tested ACK 2ead31fb1b17c9b183a4b81f0ae4f48e5cf67d64
Tree-SHA512: bcc7432d2f35093ec2463ea19e894fa885b698c0e8d8e4bd2f979bd4d722cbfed53ec589d6280968917893c64649dc9e40800b8d854273b0f9a1380f51afbdb1
The "whitelist" and "connect_nodes" is not needed in feature_taproot.py,
so remove it.
The changes to key.py are required when running the unit tests from the
test folder. Failure on current master:
[test]$ python -m unittest functional/test_framework/key.py
.E
======================================================================
ERROR: test_schnorr_testvectors (functional.test_framework.key.TestFrameworkKey)
Implement the BIP340 test vectors (read from bip340_test_vectors.csv).
----------------------------------------------------------------------
Traceback (most recent call last):
File "test/functional/test_framework/key.py", line 526, in test_schnorr_testvectors
with open(os.path.join(sys.path[0], 'test_framework', 'bip340_test_vectors.csv'), newline='', encoding='utf8') as csvfile:
FileNotFoundError: [Errno 2] No such file or directory: 'test/test_framework/bip340_test_vectors.csv'
----------------------------------------------------------------------
Ran 2 tests in 0.775s
FAILED (errors=1)
The transaction is too large to fit into the mempool, so put it into a
block.
https://travis-ci.org/github/bitcoin/bitcoin/jobs/740987240#L7217
test 2020-11-03T01:31:08.645000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "./test/functional/test_framework/test_framework.py", line 126, in main
self.run_test()
File "./test/functional/feature_taproot.py", line 1448, in run_test
self.nodes[1].sendtoaddress(address=addr, amount=int(self.nodes[1].getbalance() * 70000000) / 100000000)
File "./test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "./test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Transaction too large (-6)
Without the fix a hex-string can not be parsed:
File "./test/functional/test_framework/blocktools.py", line 82, in create_block
txo.deserialize(io.BytesIO(tx))
TypeError: a bytes-like object is required, not 'str'
Also, remove io import and repace it with our FromHex() helper
New functional test coverage of tx download was added by #19988,
but `with p2p_lock` is redundant for some tests with `wait_until`
test helper, already guaranteeing test lock tacking.
c7b7e0a69265946aecc885be911c7650911ba2e3 tests: Make only desc wallets for wallet_multwallet.py --descriptors (Andrew Chow)
d4b67ad214ada7645c4ce2d5ec336fe5c3f7f7ca Avoid creating legacy wallets in wallet_importdescriptors.py (Andrew Chow)
6c9c12bf87f95066acc28ea2270a00196eb77703 Update feature_backwards_compatibility for descriptor wallets (Andrew Chow)
9a4c631e1c00eb1661c000978b133d7aa0226290 Update wallet_labels.py to not require descriptors=False (Andrew Chow)
242aed7cc1d003e8fed574bbebd19c7e54e23402 tests: Add a --legacy-wallet that is mutually exclusive with --descriptors (Andrew Chow)
388053e1722632c2e485c56a444bc75cf0152188 Disable some tests for tool_wallet when descriptors (Andrew Chow)
47d3243160fdec7e464cfb8f869be7f5d4ee25fe Make raw multisig tests legacy wallet only in rpc_rawtransaction.py (Andrew Chow)
59d3da5bce4ebd9c2291d8f201a53ee087938b21 Do addmultisigaddress tests in legacy wallet mode in wallet_address_types.py (Andrew Chow)
25bc5dccbfd52691adca6edd418dd54290300c28 Use importdescriptors when in descriptor wallet mode in wallet_createwallet.py (Andrew Chow)
0bd1860300b13b12a25d330ba3a93ff2d13aa379 Avoid dumpprivkey and watchonly behavior in rpc_signrawtransaction.py (Andrew Chow)
08067aebfd7e838e6ce6b030c31a69422260fc6f Add script equivalent of functions in address.py (Andrew Chow)
86968882a8a26312a7af29c572313c4aff488c11 Add descriptor wallet output to tool_wallet.py (Andrew Chow)
3457679870e8eff2a7d14fe59a479692738c48b6 Use separate watchonly wallet for multisig in feature_nulldummy.py (Andrew Chow)
a42652ec10c733a5bf37e418e45d4841f54331b4 Move import and watchonly tests to be legacy wallet only in wallet_balance.py (Andrew Chow)
4b871909d6e4a51888e062d322bf53263deda15e Use importdescriptors for descriptor wallets in wallet_bumpfee.py (Andrew Chow)
c2711e4230d9a423ead24f6609691fb338b1d26b Avoid dumpprivkey in wallet_listsinceblock.py (Andrew Chow)
553dbf9af4dea96e6a3e79bba9607003342029bd Make import tests in wallet_listtransactions.py legacy wallet only (Andrew Chow)
dc81418fd01021070f3f66bab5fee1484456691a Use a separate watchonly wallet in rpc_fundrawtransaction.py (Andrew Chow)
a357111047411f18c156cd34a002a38430f2901c Update wallet_importprunedfunds to avoid dumpprivkey (Andrew Chow)
Pull request description:
I went through all the tests and checked whether they passed with descriptor wallets. This partially informed some changes in #16528. Some tests needed changes to work with descriptor wallets. These were primarily due to import and watchonly behavior. There are some tests and test cases that only test legacy wallet behavior so those tests won't be run with descriptor wallets.
This PR updates more tests to have to the `--descriptors` switch in `test_runner.py`. Additionally a mutually exclusive `--legacy-wallet` option has been added to force legacy wallets. This does nothing currently but will be useful in the future when descriptor wallets are the default. For the tests that rely on legacy wallet behavior, this option is being set so that we don't forget in the future. Those tests are `feature_segwit.py`, `wallet_watchonly.py`, `wallet_implicitsegwit.py`, `wallet_import_with_label.py`, and `wallet_import_with_label.py`.
If you invert the `--descriptors`/`--legacy-wallet` default so that descriptor wallets are the default, all tests (besides the legacy wallet specific ones) will pass.
ACKs for top commit:
MarcoFalke:
review ACK c7b7e0a69265946aecc885be911c7650911ba2e3 🎿
laanwj:
ACK c7b7e0a69265946aecc885be911c7650911ba2e3
Tree-SHA512: 2f4e87815005d1d0a2543ea7947f7cd7593d8cf5312228ef85f8e096f19739b225769961943049cb44f6f07a35b8de988e2246ab9aca5bb5a0b2e62694d5637d
3d0556d41087f945ed0a47a5d770076ad42ce432 Increase feature_taproot inactive test coverage (Pieter Wuille)
525cbd425e2f6a1dbd0febc53d7ada22cec4661f Only relay Taproot spends if next block has it active (Pieter Wuille)
Pull request description:
There should be no change to mempool transaction behavior for witness v1 transactions as long as no activation is defined. Until that point, we should treat the consensus rules as under debate, and for soft-fork safety, that means spends should be treated as non-standard.
It's possible to go further: don't relay them unless the consensus rules are actually active for the next block. This extends non-relay to the period where a deployment is defined, started, locked in, or failed. I see no downsides to this, and the code change is very simple.
ACKs for top commit:
Sjors:
utACK 3d0556d41087f945ed0a47a5d770076ad42ce432
MarcoFalke:
review ACK 3d0556d41087f945ed0a47a5d770076ad42ce432 🏓
jnewbery:
utACK 3d0556d41087f945ed0a47a5d770076ad42ce432
Tree-SHA512: ca625a2981716b4b44e8f3722718fd25fd04e25bf3ca1684924b8974fca49f7c1d438fdd9dcdfbc091a442002e20d441d42c41a0e2096e74a61068da6c60267a
Although legacy wallet is still the default, for future use, add a
--legacy-wallet option to the test framework. Additional tests for
descriptor wallets have been enabled with the --descriptors option.
Tests that must be legacy wallet only are being started with
--legacy-wallet. Even though this option does not currently do anything,
this will be helpful in the future when descriptor wallets become the
default.
