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
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
In decodepsbt if an invalid amount is seen, don't calculate the fee
but still show the invalid value in the decode.
In analyze psbt, if an invalid amount is seen, set the next step to
be the creator as the creator needs to remake the transaction so that
it is valid.
773d4572a4 Mark PSBTs spending unspendable outputs as invalid in analysis (Andrew Chow)
638e40cb60 Have a PSBTAnalysis state that indicates invalid PSBT (Andrew Chow)
Pull request description:
When analyzing an unspendable PSBT, report that it is unspendable and exit analysis early.
ACKs for top commit:
Sjors:
ACK 773d457
instagibbs:
After some thought ACK 773d4572a4
Tree-SHA512: 99b0cb2fa1ea37593fc65a20effe881639d69ddeeecf5197bc87bc7f2220cbeb40f1d429d517e4d27f2e9fb563a00cd845d2b4b1ce05246a75a6cb56fb9b0ba5
fadfd844de test: Remove unused connect_nodes_bi (MarcoFalke)
fa3b9ee8b2 scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke)
faaee1e39a test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke)
1111bb91f5 test: Reformat python imports to aid scripted diff (MarcoFalke)
Pull request description:
By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side).
This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times.
Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests.
Historic background:
`connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.
ACKs for top commit:
laanwj:
ACK fadfd844de
jonasschnelli:
utACK fadfd844de - more of less a cleanup PR.
promag:
Tested ACK fadfd844de, ran extended tests.
Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24
c0b5d97103 Test that joinpsbts randomly shuffles the inputs (Andrew Chow)
6f405a1d3b Shuffle inputs and outputs after joining psbts (Andrew Chow)
Pull request description:
`joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction.
ACKs for top commit:
instagibbs:
utACK c0b5d97103
jonatack:
ACK c0b5d97103 modulo suggestions for later.
Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8
0d101a340c test: Add test for maxtxfee option (MarcoFalke)
177550101b wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b9714cb wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)
Pull request description:
Follow up to #16257, this PR makes `bumpfee` aware of `-maxtxfee`.
It also prevents dangling locked unspents when calling `fundrawtransaction` - because the previous check was after `LockCoin`.
ACKs for top commit:
MarcoFalke:
re-ACK 0d101a340c, only change is small test fixup
Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512
The new `descriptors` argument needs to be added to the Command and
ConvertParams tables to by usable as a named argument and by
bitcoin-cli.
Also update the test to use named arguments to test this.
26fe9b9909 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a2d2 Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3c33 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88734 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)
Pull request description:
This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
* Input and output scripts and keys will be filled in when known.
* P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.
This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).
ACKs for top commit:
jnewbery:
utACK 26fe9b9909
laanwj:
utACK 26fe9b9909 (will hold merging until response to promag's comments)
promag:
ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.
Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior.
This adds a descriptors argument to the utxoupdatepsbt RPC. This means:
* Input and output scripts and keys will be filled in when known
* P2SH-witness outputs will be filled in from the UTXO set when a descriptor
is provided to show they're segwit outputs.
c87fc71f7e Bugfix: test/functional/rpc_psbt: Correct test description comment (Luke Dashjr)
097c4aa379 Bugfix: test/functional/rpc_psbt: Remove check for specific error message that depends on uncertain assumptions (Luke Dashjr)
Pull request description:
When converttopsbt is called with a signed transaction, it either fails with "TX decode failed" if one or more inputs were segwit, or "Inputs must not have scriptSigs and scriptWitnesses" otherwise.
Since no effort is made by the test to ensure the inputs are segwit or not, avoid checking the exact message used.
The error code is still checked to ensure it is of the correct kind of failure.
ACKs for commit c87fc7:
instagibbs:
utACK c87fc71f7e
achow101:
utACK c87fc71f7e
Tree-SHA512: 61312b5d49aa50652902f30ba9693dfba9e5b7e6478f23becda20202d8b328ddb3e040f2199b617a68df133a5f1f8b5d68bc19d4621303f17c1963dca01bd9ef
Split up DecodePSBT, which both decodes base64 and then deserializes a
PartiallySignedTransaction, into two functions: DecodeBase64PSBT, which retains
the old behavior, and DecodeRawPSBT, which only performs the deserialization.
Add a test for base64 decoding failure.
0fb2e69815 CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change (Gregory Sanders)
b06483c96a Remove stale comment in CalculateMaximumSignedInputSize (Gregory Sanders)
Pull request description:
This is triggered anytime a fundraw type call(psbt or legacy) is used with a change output address that the wallet doesn't know how to sign for.
This regression was added in 6a34ff5335 since BnB coin selection actually cares about this.
The fix is to assume the smallest typical spend, a P2SH-P2WPKH, which is calculated using a "prototype" dummy signature flow. Future work could generalize this infrastructure to get estimated sizes of inputs for a variety of types.
I also removed a comment which I believe is stale and misleading.
Tree-SHA512: c7e2be189e524f81a7aa4454ad9370cefba715e3781f1e462c8bab77e4d27540191419029e3ebda11e3744c0703271e479dcd560d05e4d470048d9633e34da16
When converttopsbt is called with a signed transaction, it either fails with "TX decode failed" if one or more inputs were segwit, or "Inputs must not have scriptSigs and scriptWitnesses" otherwise.
Since no effort is made by the test to ensure the inputs are segwit or not, avoid checking the exact message used.
The error code is still checked to ensure it is of the correct kind of failure.
88a79cb436 fix converttopsbt permitsigdata arg, add basic test (Gregory Sanders)
Pull request description:
The final check for extraneous sigdata has a flipped boolean, resulting in incorrect behavior.
Resolves https://github.com/bitcoin/bitcoin/issues/14355
Tree-SHA512: 5157a74b8ddebd7d836fba96765c4d7ed15a73d4289817353d3566a0f6803bd4bbc3f936735c517c7a83a6cbdb4052b9c61d23f6cc4ad00a6077278cd51adbd4
6b8d86ddb8 Require a public key to be retrieved when signing a P2PKH input (Andrew Chow)
Pull request description:
If we do not have the public key for a P2PKH input, we should not continue to attempt to sign for it.
This fixes a problem where a PSBT with a P2PKH output would include invalid BIP 32 derivation paths that are missing the public key.
Tree-SHA512: 850d5e74c06833da937d5bf0348bd134180be7167b6f9b9cecbf09f75e3543fbad60d0abbc0b9afdfa51ce165aa36168849f24a7c5abf1e75f37ce8f9a13d127
Danny Lee