It is base58, so shouldn't be qualified with STR_HEX. Similarly,
signmessagewithprivkey also declares the argument as a STR.
This fix is motivated by the OpenRPC dump, where fields tagged with STR_HEX are
described with a restricting regex that would make its correct usage a
violation against the unpatched schema.
436921eb46 test: check joinpsbts preserves global xpubs and proprietary fields (Thomas)
011094b282 rpc: preserve global xpubs and proprietary fields in joinpsbts (Thomas)
Pull request description:
`joinpsbts` collects the global xpubs of all the joined PSBTs into `merged_psbt`, but returns a separately constructed `shuffled_psbt` into which only the inputs, outputs, and unknown fields are copied. The collected `PSBT_GLOBAL_XPUB` records are silently dropped, and `PSBT_GLOBAL_PROPRIETARY` records are not collected at all.
The xpub collection was added in #17034, which was written against a `joinpsbts` that still returned `merged_psbt`, but was merged after #16512 had introduced the `shuffled_psbt` rebuild, so the collected xpubs have never reached the result.
Shuffle the inputs and outputs of `merged_psbt` in place instead of rebuilding a new PSBT, so that all global data is preserved, and union the global proprietary records in the merge loop, matching the `combinepsbt` behavior from #34893.
ACKs for top commit:
jpk68:
ACK 436921eb46
achow101:
ACK 436921eb46
winterrdog:
tACK 436921eb46
Tree-SHA512: d9de34c25aecc29b6b4fb80d6584fa919cc5ff9b7ef2f4d8ce35c4043fe7638fefb8af10448f2cd14021f5d25e149f0efc8798c5b8c9bc8b5582c6152010e891
joinpsbts collects the global xpubs of all the joined PSBTs into
merged_psbt, but returns a separately constructed shuffled_psbt into
which only the inputs, outputs, and unknown fields are copied. The
collected PSBT_GLOBAL_XPUB records are silently dropped, and
PSBT_GLOBAL_PROPRIETARY records are not collected at all.
The xpub collection was added in #17034, which was written against a
joinpsbts that still returned merged_psbt, but was merged after #16512
had introduced the shuffled_psbt rebuild, so the collected xpubs have
never reached the result.
Shuffle the inputs and outputs of merged_psbt in place instead of
rebuilding a new PSBT, so that all global data is preserved, and union
the global proprietary records in the merge loop, matching the
combinepsbt behavior from #34893.
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
25bed560be test: add forward-compat functional test for txindex (sedited)
703304ed8c doc: add release notes for txindex disk usage and downgrading (Andrew Toth)
8e5320a2d2 tests: cover txindex hash prefix collisions and legacy fallback (Andrew Toth)
b75efa19ba txindex: skip bloom filters and legacy lookups for new databases (Andrew Toth)
004d7c098c txindex: hash key prefixes and pack block positions (Andrew Toth)
5a255970fd refactor: move txindex db constants and legacy key to txindex_key.h (Andrew Toth)
327660134c txindex: pass the full block to DB::WriteTxs (Andrew Toth)
42771e7998 txindex: use a new block locator for downgrade safety (Andrew Toth)
4b08baed72 txindex: return optional tx and block hash from FindTx (Andrew Toth)
Pull request description:
The current txindex uses the full 32-byte txid as keys, which takes up about 66 GB of disk space today on mainnet. Using a 5-byte key prefix instead drops the disk usage to 26 GB - cutting the size to less than half.
Using the full 32-bytes is unnecessary since a 5-byte salted siphash will produce collisions in about 1 in 1.1 trillion. Some collisions will occur, but the penalty is just an extra disk read, deserialization and hash.
The tx position can be appended to the key instead of used as a value, and a LevelDB iterator can seek to the prefix and then scan for the correct tx. This is an almost identical approach to `txospenderindex`.
Also instead of storing the file position of the block, we can store only the sequence of the connected block and offset of the transaction in the block. This can be packed into a 6-byte key suffix using 3-byte representations of the sequence and offset in the block. The block file can be recovered by the CBlockIndex that is already in memory. The sequence is mapped to the block hash in the db, so we can lookup the block hash to find the CBlockIndex during reads.
If a tx is not found with this method, we fallback to looking up the legacy entry. With this method a user with an existing db can opt to erase the `indexes/txindex` folder and reindex, or keep the current index and new entries will be appended with the smaller footprint.
The time to index was faster on my machine with this method, 1h19m vs current 1h50m.
Lookups are roughly the same, around 0.2ms per lookup with `getrawtransaction`.
When testing on mainnet, I got 894,549 2-way collisions, 395 3-way collision, and 1 4-way collision that worst case could cause an extra 3 false positives when reading.
ACKs for top commit:
l0rinc:
diff reACK 25bed560be
sedited:
ACK 25bed560be
ajtowns:
ACK 25bed560be
Tree-SHA512: a25c79ca7e722e2f372b65f5fc11c8b194ad49f2240b4881c7e606306aabbd3604aede3f1c33606b467486affac3a3f503638f513c896935cebbc02709cb60d8
c9a70f9338 script: qa: Improve Key::Fingerprint type safety (David Gumberg)
Pull request description:
Extracted from pseudoramdom's work in #35436:
Instead of using c style arrays for key fingerprints, use `std::array`'s whose length can always reasoned about at compile time and for most operations the compiler enforces the size being correct.
```cpp
using KeyFingerprint = std::array<unsigned char, 4>;
```
```diff
- unsigned char vchFingerprint[4];
+ KeyFingerprint fingerprint;
```
This allows the replacement of a lot of raw `memcpy` + trust-me-bro lengths, with the assignment operator:
```cpp
- memcpy(ret.vchFingerprint, vchFingerprint, 4);
+ ret.fingerprint = fingerprint;
```
This commit also adds two helper functions for
- Retrieving the [fingerprint of a key identifier](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#user-content-Key_identifiers) (`CKeyID`)
- Retrieving the fingerprint of the key identifier of an XPUB.
ACKs for top commit:
w0xlt:
ACK c9a70f9338
sedited:
ACK c9a70f9338
pseudoramdom:
Code review ACK w/ some minor nits c9a70f9338
polespinasa:
ACK c9a70f9338
Tree-SHA512: 3ee76742c0bc317dfbc12a6731afdcc40495db6e4d5d94880d0a721990d36cb3e4d374ccc96079ba1f8ad3f88581ee5a609bfe259c0ea7cd28cade373aac1b38
29b124416e doc: add release notes for 32800 (Musa Haruna)
5d25a0c28d rpc: add `vsize_adjusted` field to getrawtransaction output for mempool transactions (Musa Haruna)
eaef8d3111 rpc: add `vsize_adjusted` and `vsize_bip141` field to mempool-related RPCs (Musa Haruna)
Pull request description:
### Motivation and Problem
`CTxMemPoolEntry::GetTxSize()` returns the larger of two values: the BIP 141 virtual size (vsize) and the "sigop-adjusted size." This sigop-adjusted size is used by mempool validation and mining algorithms as a safeguard to prevent overfilling blocks with transactions that approach both the weight and signature operation (sigop) limits in a way that could harm block space efficiency.
In the current implementation, the sigop-adjusted size is reported as the "vsize" in RPCs that provide mempool transaction data, such as `getmempoolentry`, `getrawmempool`, `testmempoolaccept`, and `submitpackage`. However, the documentation for these RPCs typically describes this value simply as the "virtual transaction size as defined in BIP 141," without acknowledging the sigop adjustment. Since the reported size may differ from the pure BIP 141 definition, this confuses people as in this [tweet](https://x.com/mononautical/status/1646166180145577990?s=20), discrepancy can be misleading, as the reported size may differ from the pure BIP 141 definition.
### Proposed Solution
To resolve this, all mempool-related RPCs now return two separate fields:
**vsize_adjusted:** the sigop-adjusted size, i.e. max(BIP 141 vsize, sigop-adjusted size), which reflects the value previously returned under the vsize label and continues to drive mempool acceptance and block template scoring.
**vsize_bip141:** the pure BIP 141 virtual size, strictly `ceil(weight/4)`, matching the consensus definition is now reported here in `vsize_bip141` field. `vsize` field in now marked as DEPRECATED and users are advised to use the new `vsize_bip141` field for pure virtual size instead.
This means that clients that depends on mempool policy size reported vsize will use `vsize_adjusted`, while `vsize` is now purely BIP 141.
Additionally, this PR updates the relevant RPC help text to clearly document the distinction between these two sizes, and adds supporting documentation `doc/policy/feerates-and-vsize.md` to better explain fee rates, virtual size calculations, sigop adjustments, and the mempool policy heuristics.
A new field, vsize_adjusted, has also been added to the getrawtransaction RPC result when input information (transaction is in the mempool) is available. Exposing this value provides users with more precise insight into how the transaction’s sigops impact its effective size for policy and fee estimation.
Note: This picks up work from the closed [#27591](https://github.com/bitcoin/bitcoin/pull/27591)
Fixes [#32775](https://github.com/bitcoin/bitcoin/issues/32775)
ACKs for top commit:
achow101:
ACK 29b124416e
hodlinator:
re-ACK 29b124416e
ismaelsadeeq:
Code review ACK 29b124416e
sedited:
ACK 29b124416e
Tree-SHA512: 9322ab1a2f7561b4221fb2bbe9f822c402f845c52a93de14008c1e5bc33e5c6f19ebc647ab6615b7c6be137c76cff6a33c6920818a78813de5632eb88c96a876
7e19ce200b rpc: Fix descriptorprocesspsbt internal bug on invalid signatures (b-l-u-e)
Pull request description:
Fixes#32849
descriptorprocesspsbt crashes with an internal bug assertion when given a PSBT whose inputs looked “finalized” that is non-empty final_script_sig / witness but whose signatures did not verify like invalid Schnorr bytes with unusual sighash combinations such as SIGHASH_SINGLE | ANYONECANPAY.
Completeness was inferred with PSBTInputSigned, which only checks that final fields are present, not that they pass script verification. That let the RPC treat the PSBT as complete and call FinalizeAndExtractPSBT, where a failing invariant surfaced as CHECK_NONFATAL instead of a normal incomplete outcome.
This PR determines complete the same way as the wallet path: after ProcessPSBT, it recomputes PrecomputedTransactionData and sets complete only if every input passes PSBTInputSignedAndVerified.
Invalid signatures then yield complete: false and no hex, without going through the finalize path that asserted.
ACKs for top commit:
achow101:
ACK 7e19ce200b
rkrux:
lgtm re-ACK 7e19ce200b
Tree-SHA512: c534fb5bd3401ee3ac0bc27eaedaaaaf23e5e8510ed9b96a747bb0e41e2bd3d2031b6b08e933e139bb426e97bd05fab6e91397205f4be1db9d17630eea49a8bf
Extend the `getrawtransaction` RPC to include a new field `vsize_adjusted` when the transaction is in the mempool.
The `vsize_adjusted` field provides the mempool's accounting size for the transaction based on its sigop cost,
which can exceed its serialized vsize under `-bytespersigop` policies.
Test coverage is added to verify the correct calculation and exposure of the `vsize_adjusted` field via `mempool_sigoplimit.py`.
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.
Add prevout, elision_mode, and vin_inner_elision options
to TxDocOptions, and use them to describe getrawtransaction verbosity=2
with structured metadata instead of raw ELISION entries. Extract
verbosity=1 block fields into a local verbosity_1_block variable so
verbosity=2 can reuse them via ElideGroup.
When vin_inner_elision is set, vin fields are elided while keeping
prevout visible. With elision_mode == ElisionMode::Silent, every other
top level field is hidden, so the only rendered output comes from the
vin not elided.
Loosen elision_has_description() to accept any group with at least one
field that is not SKIP.
6d86184a8b rpc: combinerawtransaction now rejects unmergeable transactions (Adam Andrews)
Pull request description:
Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.
fixes #25980
ACKs for top commit:
nervana21:
tACK 6d86184a8b
achow101:
ACK 6d86184a8b
rkrux:
ACK [6d86184](6d86184a8b)
Tree-SHA512: 5caf983c5ab618a000f40b9ad698439d3e6217ec2dc593740443f47d90f8804a895f3054dd29bbcecdb48a61992b0d0afda7ec89591d768a44918648bbb6e20d
Use PSBTInputSignedAndVerified instead of PSBTInputSigned to properly
validate signatures before marking PSBT as complete
Signed-off-by: b-l-u-e <8102260+blue@users.noreply.github.com>
walletcreatefundedpsbt, createpsbt, converttopsbt, and psbtbumpfee take
a psbt_version argument to set the version of the PSBT that they
produce. The default psbt_version is 2.
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.
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.
Previously, combinerawtransaction would silently return the first tx when
asked to combine unrelated txs. Now, it will check tx mergeability and
throws a descriptive error if tx cannot be merged.
The `CChain::Contains()` method dereferences its input without checking,
potentially resulting in nullptr-dereference if invoked with `nullptr`.
To avoid this possibility, its input is changed to a reference instead.
Call sites are adapted accoringly, extra nullptr-check is added as
needed.
Introduce `CoinsViewEmpty` as an explicit no-op `CCoinsView` implementation, and define its singleton accessor out of line in `coins.cpp` to avoid `-Wunique-object-duplication` in shared-library builds.`
Use it at call sites that intentionally want a no-op backend instead of constructing anonymous placeholder views.
`CCoinsViewTest` and `CoinsViewBottom` now inherit defaults from `CoinsViewEmpty` (e.g. the unused `EstimateSize()`, which now returns 0).
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Prior to this commit, decodepsbt_inputs would call TxDoc during initialization
which lives in another TLU. TxDoc relies on CURRENCY_UNIT to be initialized when
it may not have been (note this is different from the TLU containing
decodepsbt_inputs which also has a CURRENCY_UNIT). Fix this by lazy initializing
decodepsbt_inputs.
Prevent the issue from occurring in the future by also doing the same for
decodepsbt_outputs and getblock_vin.
Since this class defines the functionality of the RPC method, not
just its help text, this better reflects reality.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/\bRPCHelpMan\b/RPCMethod/g' $(git grep -l RPCHelpMan src/)
-END VERIFY SCRIPT-
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on
it.
Fix this issue by introducing a new print_elision option and using it
in decodepsbt.
This change will ensure that RPCResult::MatchesType is properly run.
Also, this clarifies the RPC output minimally:
```diff
--- a/decodepsbt
+++ b/decodepsbt
@@ -35,7 +35,7 @@ Result:
"inputs" : [ (json array)
{ (json object)
"non_witness_utxo" : { (json object, optional) Decoded network transaction for non-witness UTXOs
- ...
+ ... The layout is the same as the output of decoderawtransaction.
},
"witness_utxo" : { (json object, optional) Transaction output for witness UTXOs
"amount" : n, (numeric) The value in BTC
```
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>
060bb55508 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba5 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db5416 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 060bb55508🌛
achow101:
ACK 060bb55508
rkrux:
lgtm ACK 060bb55508
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
