060bb55508245776bb6a39c8b7849769ee588d69 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba547685ca4163316017ab78e965c7ad1 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db54166497685b80a12c51db6772982e01fe rpc: add "ischange: true" in wallet gettransaction decoded tx output (Matthew Zipkin)
Pull request description:
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Example of the new field:
```
"vout": [
{
"value": 1.00000000,
"n": 0,
"scriptPubKey": {
"asm": "0 5483235e05c76273b3b50af62519738781aff021",
"desc": "addr(bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu)#d42g84j6",
"hex": "00145483235e05c76273b3b50af62519738781aff021",
"address": "bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu",
"type": "witness_v0_keyhash"
}
},
{
"value": 198.99859000,
"n": 1,
"scriptPubKey": {
"asm": "0 870ab1ab58632b05a417d5295f4038500e407592",
"desc": "addr(bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju)#tgapemkv",
"hex": "0014870ab1ab58632b05a417d5295f4038500e407592",
"address": "bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju",
"type": "witness_v0_keyhash"
},
"ischange": true
}
]
```
ACKs for top commit:
furszy:
ACK [060bb55](060bb55508)
maflcko:
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
achow101:
ACK 060bb55508245776bb6a39c8b7849769ee588d69
rkrux:
lgtm ACK 060bb55508245776bb6a39c8b7849769ee588d69
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 doc: Release note for removed watchonly parameters and results (Ava Chow)
15710869e19e707ef03492c55030dcefa16269d8 wallet: Remove ISMINE_WATCH_ONLY (Ava Chow)
4439bf4b41a6997d4d965f00a8c40efa9cf6895b wallet, spend: Remove fWatchOnly from CCoinControl (Ava Chow)
1337c72198a7d32935431d64e9e58c12f9003abc wallet, rpc: Remove watchonly from RPCs (Ava Chow)
e81d95d435744e48615973dc22acce1a291bd20d wallet: Remove watchonly balances (Ava Chow)
d20dc9c6aae089ab926fd135febd69a8f0744a18 wallet: Wallets without private keys cannot grind R (Ava Chow)
9991f49c38c084967ca66791d838c99b42f000eb test: Watchonly wallets should estimate larger size (Ava Chow)
Pull request description:
Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet's internals.
With all of the watchonly things removed, ISMINE_WATCH_ONLY is removed as well.
Split from #32523
Depends on #32594 for tests that are easier to read
ACKs for top commit:
Eunovo:
ACK b1a8ac07e9
maflcko:
re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 🌈
rkrux:
ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
furszy:
light code review ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
Tree-SHA512: bc87f37a13294f7208991be8f93899b49e5bdf87c70e0f66d9c4cb09c03be6c202320406f27e9a35aa2f57319d19a3f0c07d5e5ddbc97c7edab165b1656d6612
666016e56b28b77f798dc85c767b95c1ca0abfae ci: use --usecli in one of the CI jobs (Martin Zumsande)
7ea248a020997fcbbd6d62a4ec191aa858c463ca test: Disable several (sub)tests with cli (Martin Zumsande)
f420b6356b6f886638282a6aa9309b9982768775 test: skip subtests that check for wrong types with cli (Martin Zumsande)
6530d0015b958412825e1b8ae2aaefeee4372f08 test: add function to convert to json for height_or_hash params (Martin Zumsande)
54d28722baeac84950bbe6da1d315b9202012259 test: Don't send empty named args with cli (Martin Zumsande)
cca422060e96abbdca68a931fd45653738923caa test: convert tuple to json for cli (Martin Zumsande)
af34e980866e16970e0c4b837f56cd29038ae8bc test: make rpc_psbt.py usable with --usecli (Martin Zumsande)
8f8ce9e1740dfb5249b20d2bf759c23080367553 test: rename .rpc to ._rpc and remove unnecessary uses (Martin Zumsande)
5b088859863224a94514c78ea841d7314ec8fa50 test: enable functional tests with large rpc args for cli (Martin Zumsande)
7d5352ac7373e2beb9af671cef0a1a1e9ecb6658 test: use -stdin for large rpc commands (Martin Zumsande)
6c364e0c10de4d8036e696b3726bb0acffb94617 test: Enable various tests for usage with cli (Martin Zumsande)
Pull request description:
Fixes#32264
I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with `self.supports_cli = False`. There are several reasons why existing tests fail with `--usecli` on many systems, the most important ones are:
- Most common reason is that the test executes a RPC call with a large arg that exceeds `MAX_ARG_STRLEN` of the OS, which is usually 128kb on linux: This is fixed by using `-stdin` for these large calls (idea by 0xB10C)
- they test specifically the rpc interface - nothing to do there except disabling.
- Some functional test submit wrong types to params on purpose to test the error message (which is different when using the cli) - deactivated these specific subtests locally for the cli when there is just one or two of them, deactivated the entire tests when there are more spots
- When python sets `None` for an arg, the cli converts this to 'null' in `arg_to_cli`. This is fine e.g. for boolean args, but doesn't work for strings where it's interpreted as the string 'null'. Bypass this for named args by not including args in case the value is `None` for the cli is used (it's effectively the same as leaving the optional arg out).
- the `height_or_hash` param used in some RPC needs to be converted to a JSON (effectively adding full quotes).
- Some tests were marked with `self.supports_cli = False` in the past but run fine on master today - enabled those.
In total, this PR fixes all tests that fail on master and reduces the number of tests that are deactivated (`self.supports_cli = False`) from 40 to 21.
It also adds `--usecli` to one CI job (multiprocess, i686, DEBUG) to detect regressions.
ACKs for top commit:
maflcko:
re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae 🔀
pinheadmz:
re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae
Tree-SHA512: 7a1efd212649ca100b236a1239294d40ecd36e2720e3b173a230b14545bb40b135111db7fed8a0d1448120f5387da146a03f1912e2028c8d03a0b6a3ca8761b0
Descriptor wallets are already created by default since v23.0, but
since the recent legacy wallet removal this parameter *must* be True
(see commit 9f04e02ffaee0fe64027dc56c7bea3885254321a), i.e. still
passing it wouldn't contain any information for test readers
anymore. So simply drop them in the functional tests in order to
reduce code bloat.
-BEGIN VERIFY SCRIPT-
sed -i 's/, descriptors=True//g' $(git ls-files -- 'test/functional' ':(exclude)test/functional/wallet_backwards_compatibility.py')
sed -i '/descriptors=True,/d' ./test/functional/mempool_persist.py
-END VERIFY SCRIPT-
b104d442277090337ce405d92f1398b7cc9bcdb7 test: Remove RPCOverloadWrapper (Ava Chow)
4d32c19516fdb65b364638fa088b8d7167b438b6 test: Replace importpubkey (Ava Chow)
fe838dd391be669ccd0765b95f81c25fecfd3636 test: Replace usage of addmultisigaddress (Ava Chow)
d3142077794f4e910d3cdc749020d725e30feb24 test: Replace usage of importaddress (Ava Chow)
fcc457573f9b39e6f173a4f51c45d7dbb47e7ab0 test: Replace importprivkey with wallet_importprivkey (Ava Chow)
94c87bbbd06eb9a57930b9f59315533cfbe8f460 test: Remove unnecessary importprivkey from wallet_createwallet (Ava Chow)
Pull request description:
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.
For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.
For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did. This is mainly to reduce verbosity as `importprivkey` was more widely used throughout the tests.
Some tests that used these RPCs are now also no longer relevant and have been removed.
ACKs for top commit:
Sjors:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
pablomartin4btc:
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
rkrux:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
w0xlt:
ACK b104d44227
Tree-SHA512: ded2f73829e2ce28466d4a9738eb382783ad990daee5d1859dbc4d354e6f8eec0c483ed5ecb1287fe0dd24ac332065b733a30d71b126b841bd7cd49e9a094b6d
During init, the test framework will start using rpc after the
mempool was loaded. It will not wait for postInitProcess or
outstanding transactionAddedToMempool notifications, leading to
a possible race, in which listunspent is being called while the
tx is still in Inactive status. Prevent this by processing
outstanding notifications.
There could be a race with outstanding TxAddedToMempool notifications
being applied to the soon-to-be created wallet.
Fixes an intermittent timeout reproducable by adding a sleep to
AddToWallet.
Removes all legacy wallet specific functional tests.
Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.
98570fe29bb08d7edc48011aa6b9731c6ab4ed2e test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d193b91ca42f18759098d3fef6eaab05 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner)
Pull request description:
Parsing legacy public keys can fail for three reasons (in this order):
- pubkey is not in hex
- pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively)
- pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check)
Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user.
Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`.
Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific.
The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before.
ACKs for top commit:
stratospher:
tested ACK 98570fe.
davidgumberg:
ACK 98570fe29b
Eunovo:
Tested ACK 98570fe29b
achow101:
ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
5952292133d6cc889f51ae771f2e0557311e1efe wallet, rpc: show mempool conflicts in `gettransaction` result (ishaanam)
54e07ee22ff16fc68583ade0d2b8ffffc81d444a wallet: track mempool conflicts (ishaanam)
d64922b5903e5ffc8d2ce0e6761f99f173b60800 wallet refactor: use CWalletTx member functions to determine tx state (ishaanam)
ffe5ff1fb622a8da11b66289e1b778e45e449811 scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted (ishaanam)
180973a94180f9849bf7cb0dab7c9177a942efb8 test: Add tests for wallet mempool conflicts (ishaanam)
Pull request description:
The `mempool_conflicts` variable is added to `CWalletTx`, it is a set of txids of txs in the mempool conflicting with the wallet tx or a wallet tx's parent. This PR only changes how mempool-conflicted txs are dealt with in memory.
`IsSpent` now returns false for an output being spent by a mempool conflicted transaction where it previously returned true.
A txid is added to `mempool_conflicts` during `transactionAddedToMempool`. A txid is removed from `mempool_conflicts` during `transactionRemovedFromMempool`.
This PR also adds a `mempoolconflicts` field to the `gettransaction` wallet RPC result.
Builds on #27145
Second attempt at #18600
ACKs for top commit:
achow101:
ACK 5952292133d6cc889f51ae771f2e0557311e1efe
ryanofsky:
Code review ACK 5952292133d6cc889f51ae771f2e0557311e1efe. Just small suggested changes since last review
furszy:
ACK 59522921
Tree-SHA512: 615779606723dbb6c2e302681d8e58ae2052ffee52d721ee0389746ddbbcf4b4c4afacf01ddf42b6405bc6f883520524186a955bf6b628fe9b3ae54cffc56a29
This deduplicates code and leads to more consistent and detailed error
messages. Affected are legacy import RPCs (`importpubkey`,
`importmulti`) and other ones where solving data can be provided
(`fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendall`).
Update CheckPackageLimits to use util::Result to pass the error message
instead of out parameter.
Also update test to reflect the error message from `CTxMempool`
`CheckPackageLimits` output.
This commit introduces a helper `create_outpoints` to execute the
`send` RPC and immediately return the target address outpoints as UTXO
dictionary in the common format, making the tests more readable and
avoiding unnecessary duplication.
710b83938ab5bbc4bd324d8b2e69461a2a1d2eec rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs (Harris)
Pull request description:
Reopens#18570 and closes#18567.
I have rebased the original PR.
Not sure why the original got closed as it was about to get merged.
ACKs for top commit:
achow101:
ACK 710b83938ab5bbc4bd324d8b2e69461a2a1d2eec
Tree-SHA512: d4478d990be98b1642e9ffb2930987f4a224e8bd64e2e35a5dda927a54c509ec9d712cd0eac35dc2bb89f00a1678e530ce14d7445f1dd93aa3a4cce2bc9b130d
057057a2d7e23c2e29cbfd29a5124b3947a33757 Add test for `sendmany` rpc that uses `subtractfeefrom` parameter (Yusuf Sahin HAMZA)
Pull request description:
This PR adds test that uses `sendmany` rpc to send **BTC** to multiple addresses using `subtractfeefrom` parameter, then checks receiver addresses balances to make sure fees are subtracted correctly.
ACKs for top commit:
achow101:
ACK 057057a2d7e23c2e29cbfd29a5124b3947a33757
Tree-SHA512: 51167120d489f0ff7b8b9855424d07cb55a8965984f904643cddf45e7a08c350eaded498c350ec9c660edf72c2f128ec142347c9c79d5043d9f6cd481b15cd7e
Review note: The changes are complete, because self.options.descriptors
is set to None in parse_args (test_framework.py).
A value of None implies -disablewallet, see the previous commit.
So if a call to add_wallet_options is missing, it will lead to a test
failure when the wallet is compiled in.
By throwing a custom exception from `Univalue::checkType` (instead of a plain
std::runtime_error) and catching it on the RPC server request handler.
So we properly return RPC_TYPE_ERROR (-3) on arg type errors and
not the general RPC_MISC_ERROR (-1).
a6b0c1fcc06485ecd320727fa7534a51a20608c1 doc: add releases notes for 25504 (listsinceblock updates) (Antoine Poinsot)
0fd2d144540b720626fc065a3cef5188831b5ee2 rpc: add an include_change parameter to listsinceblock (Antoine Poinsot)
55f98d087efd2609d808c082d5770306cc489409 rpc: output parent wallet descriptors for coins in listunspent (Antoine Poinsot)
b724476158a7dfeef9edfda3f519dfd6f93202a8 rpc: output wallet descriptors for received entries in listsinceblock (Antoine Poinsot)
55a82eaf91d252a04a0cc8ad7d948d956c6cb24f wallet: allow to fetch the wallet descriptors for a given Script (Antoine Poinsot)
Pull request description:
Wallet descriptors are useful for applications using the Bitcoin Core wallet as a backend for tracking coins, as they allow to track coins for multiple descriptors in a single wallet. However there is no information currently given for such applications to link a coin with an imported descriptor, severely limiting the possibilities for such applications of using multiple descriptors in a single wallet. This PR outputs the matching imported descriptor(s) for a given received coin in `listsinceblock` (and friends).
It comes from a need for an application i'm working on, but i think it's something any software using `bitcoind` to track multiple descriptors in a single wallet would have eventually. For instance i'm thinking about the BDK project. Currently, the way to achieve this is to import raw addresses with labels and to have your application be responsible for wallet things like the gap limit.
I'll add this to the output of `listunspent` too if this gets a few Concept ACKs.
ACKs for top commit:
instagibbs:
ACK a6b0c1fcc0
achow101:
re-ACK a6b0c1fcc06485ecd320727fa7534a51a20608c1
Tree-SHA512: 7a5850e8de98b439ddede2cb72de0208944f8cda67272e8b8037678738d55b7a5272375be808b0f7d15def4904430e089dafdcc037436858ff3292c5f8b75e37
By specifying the `dustrelayfee=0` option instead of the more
generic `acceptnonstdtxn=1`, we can be more specific about what
part of the transaction is non-standard and can be sure that all
other aspects follow the standard policy.
da2bc865d644f6be748c305556bdd02f02d1b161 [wallet] don't create long chains by default (glozow)
Pull request description:
Default mempool policy doesn't let you have chains longer than 25 transactions. This is locally configurable of course, but it's not really safe to assume that a chain longer than 25 transactions will propagate. Thus, the wallet should probably avoid creating such transactions by default; set `DEFAULT_WALLET_REJECT_LONG_CHAINS` to true.
Closes#9752Closes#10004
ACKs for top commit:
MarcoFalke:
re-ACK da2bc865d644f6be748c305556bdd02f02d1b161 only change is fixing typos in tests 🎏
Tree-SHA512: 65d8e4ec437fe928adf554aa7e819a52e0599b403d5310895f4e371e99bbc838219b3097c4d2f775bc870ac617ef6b4227b94291f2b376f824f14e8f2b152f31
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-
