ea785a31f7 psbt: preserve sighash type when merging inputs (Thomas)
Pull request description:
`PSBTInput::Merge` copies every optional input field from the other input when it is absent locally, except `PSBT_IN_SIGHASH_TYPE`. So `combinepsbt` silently drops the sighash type whenever the first PSBT does not carry it, making the result depend on the argument order.
The field is what lets finalizers enforce the sighash type of existing signatures (BIP 174). When it is lost, `FinalizePSBT` falls back to the default type (`SIGHASH_ALL`, or `SIGHASH_DEFAULT` for taproot inputs), rejects signatures made with any other type as a sighash mismatch, and the PSBT can no longer be finalized. Combining a PSBT signed with `ALL|ANYONECANPAY` after a merely updated copy of the same PSBT reproduces this: `finalizepsbt` reports it as incomplete, while the reverse order finalizes and broadcasts fine.
Merge the sighash type like the other optional fields, keeping the one already present, and test both combine orders.
ACKs for top commit:
achow101:
ACK ea785a31f7
winterrdog:
Re-ACK ea785a31f7
vicjuma:
ACK ea785a31f7
rkrux:
lgtm ACK ea785a31f7
Tree-SHA512: 3487368509926c3dc0218dfab2e08273676504ad5ed4635e12e56c0484bda4cd94f4ba6f2df26ee4a902ce9a274546f727f8eca4a62eb0add3a700b2141eb272
`PSBTInput::Merge` copies every optional input field from the other
input when it is absent locally, except `PSBT_IN_SIGHASH_TYPE`. So
`combinepsbt` silently drops the sighash type whenever the first PSBT
does not carry it, making the result depend on the argument order.
The field is what lets finalizers enforce the sighash type of existing
signatures (BIP 174). When it is lost, `FinalizePSBT` falls back to the
default type (`SIGHASH_ALL`, or `SIGHASH_DEFAULT` for taproot inputs),
rejects signatures made with any other type as a sighash mismatch, and
the PSBT can no longer be finalized. Combining a PSBT signed with
`ALL|ANYONECANPAY` after a merely updated copy of the same PSBT
reproduces this: `finalizepsbt` reports it as incomplete, while the
reverse order finalizes and broadcasts fine.
Merge the sighash type like the other optional fields, keeping the one
already present, and test both combine orders.
m_tap_scripts maps a leaf script to a set of control blocks, but is serialized
one record per control block, keyed by the control block. PSBTInput::Merge
unions it by the map key, so two PSBTs that map the same control block to
different leaf scripts merge into an input serializing the 0x15 key twice.
Duplicate keys are invalid, so combinepsbt hands back a PSBT that can no longer
be decoded. Present since #22558 (v24.0).
Merge the records instead of the map entries, keeping the leaf script already
there, as BIP 174 lets the Combiner pick arbitrarily when conflicts occur. The
control blocks already present are collected once rather than searched for per
incoming record, which would be quadratic in the size of the PSBTs.
Control blocks under a leaf script that both PSBTs carry are now kept as well,
where the map level union dropped them.
The test covers conflicting and non-conflicting merges, including an incoming
leaf script whose control blocks only partly conflict, so records that do not
conflict are not dropped alongside those that do.
6d387af562 psbt: remove write-only global xpub tracking set (Thomas)
3b7051c7e3 test: check combinepsbt with conflicting global xpub origins (Thomas)
7c632c0e2a psbt: avoid duplicate global xpub keys when merging (Thomas)
Pull request description:
Global xpubs are stored in a map of key origin to set of xpubs, while the serialization writes one record per xpub, keyed by the xpub. `Merge` unions the map origin-by-origin, so when the combined PSBTs provide different key origins for the same xpub, the result serializes the same `PSBT_GLOBAL_XPUB` key twice. BIP 174 declares PSBTs with duplicate keys invalid and the deserializer rejects them, so `combinepsbt` returns a PSBT that no RPC can parse again. This affects all releases since the merge loop was added in #17034 (v23.0).
<details><summary>Reproduction on master</summary>
The PSBTs share the unsigned transaction and xpub, and differ only in the master fingerprint of the global xpub record (`00000000` vs `11111111`):
```
$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQAAAAAAAAA
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQRERERAAAA
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, global key "01043587cf00...9c85c2" already provided: iostream error
```
</details>
Deduplicate by xpub when merging, keeping the origin that is already present: BIP 174 lets the Combiner "pick arbitrarily when conflicts occur", and conflicting unknown and proprietary records are already resolved the same way. The logic is shared between `combinepsbt` and `joinpsbts` through a new `MergeGlobalXPubs` helper. The second commit adds a test that fails on master with the error above, and the last commit removes the `global_xpubs` tracking set in `Unserialize`, write-only since the generic duplicate key check introduced in #21283 (1e2d146b47) replaced the explicit one.
Note: the xpub loop in `joinpsbts` currently has no observable effect, since the collected xpubs never reach the returned PSBT. My #35516 fixes that, so this PR should land first: on its own, #35516 would make the same duplicate key issue reachable through `joinpsbts`, while with the shared helper in place it never becomes reachable. I will rebase #35516 on top afterwards.
ACKs for top commit:
Bicaru20:
tACK 6d387af562.
achow101:
ACK 6d387af562
winterrdog:
tACK 6d387af562
Tree-SHA512: e2a9e02617eeec22a9240d7cf9386ee880a5f3639b143df7de4d8ea3e7b808f8c123f0b0410ff4a22e9a564bd86b2335a5c4aa3b2281af47d111484a6f1fd108
PSBTv2 permits outputs to be added before inputs.
`UpdatePSBTOutput()` traverses output scripts with a signature creator for input index 0, so ECDSA signing or a miniscript relative-timelock check can access a missing input and abort.
Construct a standalone transaction with one input for metadata traversal.
Continue taking the output from the unsigned PSBT transaction so scripts and key origins remain associated with the correct output while traversal no longer depends on the PSBT input list.
`MutableTransactionSignatureCreator` continues to require callers to provide a valid input index.
Co-authored-by: Ava Chow <github@achow101.com>
The global xpubs are stored in a map of key origin to set of xpubs,
while the serialization writes one record per xpub, keyed by the xpub.
Merging two PSBTs origin-by-origin can store the same xpub under two
origins, which serializes as a PSBT with duplicate keys that the
deserializer (and BIP 174) reject. Notably, combinepsbt returned a
PSBT that no RPC could parse again when the combined PSBTs provide
different key origins for the same xpub.
Deduplicate by xpub when merging, keeping the origin that is already
present, and share the logic between combinepsbt and joinpsbts.
CombinePSBTs currently preserves unknown records but drops proprietary records at the global, input, and output levels because the Merge() paths never union m_proprietary.
Preserve proprietary records in PartiallySignedTransaction::Merge(), PSBTInput::Merge(), and PSBTOutput::Merge() so combine/merge keeps all PSBT key-value data.
The global unsigned tx is decomposed into separate fields inside of
PSBT, which mirrors what PSBTv2 will do. However, we still need to get
the global unsigned tx so PSBT::GetUnsignedTx is introduced to do that.
In order to also have a stable unique ID, we also introduce
PSBT::GetUniqueID to replace uses of PSBT.tx.GetHash().
PSBTInput now has the previous txid and output index, and PSBTOutput has
the amount and script. We no longer need to access the global unsigned
tx for these fields.
Additionally, we can change iterating tx.vin and tx.vout to psbtx.inputs
and psbtx.outputs.
This is in prepration for use with PSBTv2 where the global unsigned tx
will not exist.
When decomposing a transaction into a PSBTv2, the tx version and
locktime need to be stored in their respective global fields. Add those
fields and fill them when constructing.
Instead of allowing PSBTs to be default constructor, force usage of the
deserialization constructor.
CombinePSBTs, DecodeBase64PSBT, and DecodeRawPSBT are all changed to
return std::optional or util::result rather than using an output
parameter to avoid the need for a default constructor.
PSBTInput should be aware of the previous txid, output index, and
sequence numbers for inputs, extracting them from the global
unsigned tx.
PSBTOutput should be aware of the output amount and script, extracting
them from the global unsigned tx.
This prepares for PSBTv2 where these fields are serialized.
Replace the sign, finalize , bip32derivs and sighash_type arguments which
are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct.
This makes it easier to add additional options later without large code
churn, such as avoid_script_path proposed in #32857. It also makes the
use of default boolean options safer compared to positional arguments
that can easily get mixed up.
Also, apply the comment according to the dev notes.
Also, modify the dev notes to give a lambda-wrapped example.
Can be reviewed via --ignore-all-space
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
The changes made here were:
| From | To |
|-------------------|------------------|
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
The commit contains the trivial, mechanical refactors where it doesn't matter if the container can have multiple elements or not
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
Since the sighash type field is written for atypical sighash types, we
can look at that field to figure out whether the psbt contains
unnecessary transactions.
SignPSBTInput will need to report the specific things that caused an
error to callers, so change it to return a PSBTError. Additionally some
callers will now check the return value and report an error to the user.
Currently, this should not change any behavior as the things that
SignPBSTInput will error on are all first checked by its callers.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
Add TransactionError to node namespace and include it directly instead of
relying on indirect include through common/messages.h
This is a followup to a previous commit which moved the TransactionError enum.
These changes were done in a separate followup just to keep the previous commit
more minimal and easy to review.
Add separate PSBTError enum instead of reusing TransactionError enum for PSBT
operations, and drop unused error codes. The error codes returned by PSBT
operations and transaction broadcast functions mostly do not overlap, so using
an unified enum makes it harder to call any of these functions and know which
errors actually need to be handled.
Define PSBTError in the common library because PSBT functionality is
implemented in the common library and used by both the node (for rawtransaction
RPCs) and the wallet.
We make the Satisfier a base in which to store the common methods
between the Tapscript and P2WSH satisfier, and from which they both
inherit.
A field is added to SignatureData to be able to satisfy pkh() under
Tapscript context (to get the pubkey hash preimage) without wallet data.
For instance in `finalizepsbt` RPC. See also the next commits for a
functional test that exercises this.
6e9f8bb050 rpc, tests: in `utxoupdatepsbt` also look for the transaction in the txindex (ishaanam)
a5b4883fb4 rpc: extract psbt updating logic into ProcessPSBT (ishaanam)
Pull request description:
Previously the `utxoupdatepsbt` RPC, added in #13932, only updated the inputs spending segwit outputs with the `witness_utxo`, because the full transaction is needed for legacy inputs. Before this RPC looked for just the utxos in the utxo set and the mempool. This PR makes it so that if the user has txindex enabled, then the full transaction is looked for there for all inputs. If it is not found in the txindex or txindex isn't enabled, then the mempool is checked for the full transaction. If the transaction an input is spending from is still not found at that point, then the utxo set is searched and the inputs spending segwit outputs are updated with just the utxo.
ACKs for top commit:
achow101:
ACK 6e9f8bb050
Xekyo:
ACK 6e9f8bb050
Tree-SHA512: 078db3c37a1ecd5816d80a42e8bd1341e416d661f508fa5fce0f4e1249fefd7b92a0d45f44957781cb69d0953145ef096ecdd4545ada39062be27742402dac6f
Previously only the segwit utxos being spent by the psbt were looked for and
added to the psbt. Now, the full transaction corresponding to each of these
utxos (legacy and segwit) is looked for in the txindex and mempool and added
to the psbt. If txindex is disabled and the transaction is not in the mempool,
then we fall back to getting just the utxo (if segwit) from the utxo set.
