ee045b61efc1479c1866b786661ef39a863677d0 rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c3729d4e054ac4260b344a75ad4b7239b3 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06709212934c521c513bb2e1a521a31f psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f80e998f7402433572b695f589f7f42 psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337ad75390a1f8810d6715f3634ed07e98 wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49b2e709a2b00af92ca76e9f30e47aec psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a11825694856a2643e9600fa537182fbb597c107 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4ada5b64c8113450ed736a2581c97518 wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923b10f0690de9dcd34f17fbb8d6de5eb psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd815325713d64b9daa61c2f93061d27bd47d tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d6123e0056656e406cd9f35aac6f71df4b psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
rkrux:
re-ACK ee045b61efc1479c1866b786661ef39a863677d0
fjahr:
Code review ACK ee045b61efc1479c1866b786661ef39a863677d0
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
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.
e261eb8d50c7192260a449e653453e63f59dbeed tests: Add BIP 373 test vectors (Ava Chow)
26370c68d09ddd6c8d24ef3b62e7b87a09e6dcaa rpc: Include MuSig2 fields in decodepsbt (Ava Chow)
ff3d460898489d2c509492a9a11b1a336e6ec662 psbt: Implement un/ser of musig2 fields (Ava Chow)
Pull request description:
Implements un/serialization of MuSig2 PSBT fields and prepares PSBT to be able to sign for MuSig2 inputs.
Split from #29675
ACKs for top commit:
fjahr:
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed
theStack:
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed
rkrux:
tACK e261eb8d50c7192260a449e653453e63f59dbeed
Tree-SHA512: bb852ad074978847ac4dc656332025e2d4d1025d4283537b89618c7cadd61a8ecd2eff24779b8a014bc8d7b431125060449768192fa05ad0577f29e3c64b2374
32dcec269bf33f7be28245d88a1d8f2889cc39ae rpc: update RPC help of `createpsbt` (rkrux)
931117a46f5854d487e13b2b1446b621409c8371 rpc: update the doc for `data` field in `outputs` argument (rkrux)
8134a6b5d40568dcf32fdb21163cb1792efddc27 rpc: add cli example for `walletcreatefundedpsbt` RPC (rkrux)
Pull request description:
### add cli example for `walletcreatefundedpsbt` and `createpsbt` RPCs
The only example present earlier was one that creates an OP_RETURN output. This
lack of examples has discouraged me earlier to use this RPC. Adding an example
that creates PSBT sending bitcoin to address, a scenario that is much more common.
### rpc: update the doc for `data` field in `outputs` argument
It was not evident to me that this field creates an `OP_RETURN` output until
I read the code and tried it out. Thus, making the doc explicitly mention it.
This affects docs of the following RPCs:
`bumpfee`, `psbtbumpfee`, `send`, `walletcreatefundedpsbt`, `createpsbt`,
and `createrawtransaction`
ACKs for top commit:
sipa:
utACK 32dcec269bf33f7be28245d88a1d8f2889cc39ae
1440000bytes:
utACK 32dcec269b
achow101:
ACK 32dcec269bf33f7be28245d88a1d8f2889cc39ae
ryanofsky:
Concept ACK 32dcec269bf33f7be28245d88a1d8f2889cc39ae. These seem like helpful clarifications, but I did not look into the details
Tree-SHA512: f994488ba7d52d00960fc52064bb419cf548e29822fe23d6ee0452fdf514dd93f089145eddb32b8086a7918cf8cf33a4c3f16bfcb7948f3c9d5afd95e8d3a1cb
Update the example wherein the PSBT sends bitcoin to an address instead
of creating an OP_RETURN output. Also, update the RPC description to
reflect the fact that the created transaction is unsigned.
This affects docs of the following RPCs:
`bumpfee`, `psbtbumpfee`, `send`, `walletcreatefundedpsbt`, `createpsbt`,
and `createrawtransaction`
It was not evident to me that this field creates an `OP_RETURN` output until
I read the code and tried it out. Thus, making the doc explicitly mention it.
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-
37a5c5d83664c31d83fc649d3c8c858bd5f10f21 doc: update descriptors.md for getdescriptoractivity (James O'Beirne)
ee3ce6a4f4d35afe7fcab16eff419a6788b02170 test: rpc: add no address case for getdescriptoractivity (James O'Beirne)
811f76f3a511d20750046319b390e225a1151caa rpc: add getdescriptoractivity (James O'Beirne)
25fe087de59e967ce968d35ed77138325eb9a9fa rpc: move-only: move ScriptPubKeyDoc to utils (James O'Beirne)
Pull request description:
The RPC command `scanblocks` provides a useful way to get a set of blockhashes that have activity relevant to a set of descriptors (`relevant_blocks`). However actually extracting the activity from those blocks is left as an exercise to the end user.
This process involves not only generating the (potentially ranged) set of scripts for the descriptor set on the client side (maybe via `deriveaddresses`), but then the user must retrieve each block's contents one-by-one using `getblock <hash>`, which is transmitted over a network link. And that's all before they perform the actual search over block content. There's even more work required to incorporate unconfirmed transactions.
This PR introduces an RPC `getdescriptoractivity` that [dovetails](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-08-16#1046393;) with `scanblocks` output, handling the process described above. Users specify the blockhashes (perhaps from `relevant_blocks`) and a set of descriptors; they are then given all spend/receive activity in that set of blocks.
This is a very useful tool when implementing lightweight wallets that want neither to require a third-party indexer like electrs, nor the overhead of creating and managing watch-only wallets in Core. This allows Core to be more easily used in a "stateless" manner by wallets, with potentially many nodes interchangeably acting as backends.
### Example usage
```
% ./src/bitcoin-cli scanblocks start \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' \
857263
{
"from_height": 857263,
"to_height": 858263,
"relevant_blocks": [
"00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"
],
"completed": true
}
% ./src/bitcoin-cli getdescriptoractivity \
'["00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88", "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb"]' \
'["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]'
{
"activity": [
{
"type": "receive",
"amount": 0.00002900,
"blockhash": "00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a60900a5a88",
"height": 857907,
"txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"vout": 254,
"output_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
},
{
"type": "spend",
"amount": 0.00002900,
"blockhash": "00000000000000000001c5291ed6a40c06d3db5c8fb738567654b24a14b24ecb",
"height": 858260,
"spend_txid": "7f61d1b248d4ee46376f9c6df272f63fbb0c17039381fb23ca5d90473b823c36",
"spend_vin": 0,
"prevout_txid": "c9d34f202c1f66d80cae76f305350f5fdde910b97cf6ae6bf79f5bcf2a337d06",
"prevout_vout": 254,
"prevout_spk": {
"asm": "1 7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"desc": "rawtr(7e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b)#yewcd80j",
"hex": "51207e02f613a8d427f5f55ff62bddc47ccfb394953e57fdcb9a8add58af3124698b",
"address": "bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t",
"type": "witness_v1_taproot"
}
}
]
}
```
ACKs for top commit:
instagibbs:
reACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
achow101:
ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
tdb3:
Code review and light retest ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
rkrux:
re-ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21
Tree-SHA512: 04aa51e329c6c2ed72464b9886281d5ebd7511a8a8e184ea81249033a4dad535a12829b1010afc2da79b344ea8b5ab8ed47e426d0bf2eb78ab395d20b1da8dbb
This avoids low-level log errors that are supposed to only occur when
there is an actual problem with the block on disk missing unexpectedly,
but not in the case where the block and/or undo data are expected not to be there.
It changes behavior such that in the first case (block index indicates
data is available but retrieving it fails) an error is thrown.
It also adjusts a functional tests that tried to simulate not
having undo data (but having block data) by deleting the undo file.
This situation should occur reality because block and undo data are pruned together.
Instead, test this situation with a block that hasn't been connected.
fa5755b0a8536b844fdccfecf386c1baab24f1c9 doc: rpc: Use "output script" consistently (2/2) (MarcoFalke)
Pull request description:
Small follow-up to https://github.com/bitcoin/bitcoin/pull/30408 to fixup the RPCs that were forgotten.
ACKs for top commit:
theStack:
lgtm ACK fa5755b0a8536b844fdccfecf386c1baab24f1c9
Tree-SHA512: f1fc0aabb59017da216d6fe0f08a2274336d04db332ad6ce3d9608cd6f03667be1c76423f24a489ac8e7d536011a129dca752ab64b4621b7bc1d4d53f68602e4
75648cea5a9032b3d388cbebacb94d908e08924e test: add P2A ProduceSignature coverage (Greg Sanders)
7998ce6b20fba62c022228355907b612ba6692e1 Add release note for P2A output feature (Greg Sanders)
71c9b02a04742eeecab14aae4697b1a3eb51ff7f test: add P2A coverage for decodescript (Greg Sanders)
1349e9ec1558484f2912a2444c410170fcec8745 test: Add anchor mempool acceptance test (Greg Sanders)
9d892099378b2ad5f52220403bdeae43c61d6955 policy: stop 3rd party wtxid malleability of anchor spend (Greg Sanders)
b60aaf8b239978947d2b0e3f56e7d8a4092d7570 policy: make anchor spend standard (Greg Sanders)
455fca86cfada1823aa28615b5683f9dc73dbb9a policy: Add OP_1 <0x4e73> as a standard output type (Greg Sanders)
Pull request description:
This is a sub-feature taken out of the original proposal for ephemeral anchors #30239
This PR makes *spending* of `OP_1 <0x4e73>` (i.e. `bc1pfeessrawgf`) standard. Creation of this output type is already standard.
Any future witness output types are considered relay-standard to create, but not to spend. This preserves upgrade hooks, such as a completely new output type for a softfork such as BIP341. It also gives us a bit of room to use a new output type for policy uses.
This particular sized witness program has no other known use-cases (https://bitcoin.stackexchange.com/a/110664/17078), s it affords insufficient cryptographic security for a secure commitment to data, such as a script or a public key. This makes this type of output "keyless", or unauthenticated.
As a witness program, the `scriptSig` of the input MUST be blank, by BIP141. This helps ensure txid-stability of the spending transaction, which may be required for smart contracting wallets. If we do not use segwit, a miner can simply insert an `OP_NOP` in the `scriptSig` without effecting the result of program execution.
An additional relay restriction is to disallow non-empty witness data, which an adversary may use to penalize the "honest" transactor when RBF'ing the transaction due to the incremental fee requirement of RBF rules.
The intended use-case for this output type is to "anchor" the transaction with a spending child to bring exogenous CPFP fees into the transaction package, encouraging the inclusion of the package in a block. The minimal size of creation and spending of this output makes it an attractive contrast to outputs like `p2sh(OP_TRUE)` and `p2wsh(OP_TRUE)` which
are significantly larger in vbyte terms.
Combined with TRUC transactions which limits the size of child transactions significantly, this is an attractive option for presigned transactions that need to be fee-bumped after the fact.
ACKs for top commit:
sdaftuar:
utACK 75648cea5a9032b3d388cbebacb94d908e08924e
theStack:
re-ACK 75648cea5a9032b3d388cbebacb94d908e08924e
ismaelsadeeq:
re-ACK 75648cea5a9032b3d388cbebacb94d908e08924e via [diff](e7ce6dc070..75648cea5a)
glozow:
ACK 75648cea5a9032b3d388cbebacb94d908e08924e
tdb3:
ACK 75648cea5a9032b3d388cbebacb94d908e08924e
Tree-SHA512: d529de23d20857e6cdb40fa611d0446b49989eaafed06c28280e8fd1897f1ed8d89a4eabbec1bbf8df3d319910066c3dbbba5a70a87ff0b2967d5205db32ad1e
These outputs are called anchors, and allow
key-less anchor spends which are vsize-minimized
versus keyed anchors which require larger outputs
when creating and inputs when spending.
29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b rpc: doc: use "output script" terminology consistently in "asm"/"hex" results (Sebastian Falbesoner)
Pull request description:
The wording "public key script" was likely chosen as a human-readable form of the technical term `scriptPubKey`, but it doesn't seem to be really widespread. Replace it by the more (probably most?) common term "output script" instead. Note that the argument for the `decodescript` RPC is not necessarily an output script (it could e.g. be also a redeem script), so in this case we just stay generic and use "script".
See also the draft BIP "Terminology for Transaction Components" (https://github.com/murchandamus/bips/blob/2022-04-tx-terminology/bip-tx-terminology.mediawiki) from murchandamus which suggests to use "output script" as well.
Affects the help text of the following RPCs:
- decodepsbt
- decoderawtransaction
- decodescript
- getblock (if verbosity=3)
- getrawtransaction (if verbosity=2,3)
- gettxout
ACKs for top commit:
maflcko:
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b
achow101:
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b
BrandonOdiwuor:
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b
tdb3:
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b
Tree-SHA512: 62eb92d42bc44e36dc3090df7b248a123868a74af253d2046de02086e688bf6ff98307b927ba2fee3d599f85e073aeb8eca90ed15105ca63b648b6796cfa340b
The wording "public key script" was likely chosen as a human-readable form of
the technical term `scriptPubKey`, but it doesn't seem to be really widespread.
Replace it by the more common term "output script" instead. Note that the
argument for the `decodescript` RPC is not necessarily an output script (it
could e.g. be also a redeem script), so in this case we just stay generic and
use "script".
See also the draft BIP "Terminology for Transaction Components"
(https://github.com/murchandamus/bips/blob/2022-04-tx-terminology/bip-tx-terminology.mediawiki)
which suggests to use "output script" as well.
Affects the help text of the following RPCs:
- decodepsbt
- decoderawtransaction
- decodescript
- getblock (if verbosity=3)
- getrawtransaction (if verbosity=2,3)
- gettxout
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
TheCharlatan:
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
hebasto:
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
2451a217dd2c21b6d2f2b2699ceddd0bf9073019 test: addmultisigaddress, coverage for script size limits (furszy)
53302a09817e5b799d345dfea432546a55a9d727 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes (furszy)
9be6065cc03f2408f290a332b203eef9c9cebf24 test: coverage for 16-20 segwit multisig scripts (furszy)
9d9a91c4ea6b3bb32ef4131bca86f1d6683fc901 rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey (furszy)
0c9fedfc45fa7cbd6801ca5fd756863ec9a6911c fix incorrect multisig redeem script size limit for segwit (furszy)
f7a173b5785cda460470df9a74a0e0f94d7f9a18 test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' (furszy)
4f33dbd8f8c0e29f37b04e6af6d2c7905ecceaf6 test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' (furszy)
25a81705d376e8c96dad45436ae3fca975b3daf5 test: rpc_createmultisig, remove unnecessary checkbalances() (furszy)
b5a328943362cfac6e90fd4e1b167c357d53b7d4 test: refactor, multiple cleanups in rpc_createmultisig.py (furszy)
3635d432681847313c098f9827483372a840e70f test: rpc_createmultisig, remove manual wallet initialization (furszy)
Pull request description:
Fixing https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674830104 and more.
Currently, redeem scripts longer than 520 bytes, which are technically valid under segwit rules, have flaws in the following processes:
1) The multisig creation process fails to deduce the output descriptor, resulting in the generation of an incorrect descriptor. Additionally, the accompanying user warning is also inaccurate.
2) The `signrawtransactionwithkey` RPC command fail to sign them.
3) The legacy wallet `addmultisigaddress` wrongly discards them.
The issue arises because most of these flows are utilizing the legacy spkm keystore, which imposes
the [p2sh max redeem script size rule](ded6873340/src/script/signingprovider.cpp (L160)) on all scripts. Which blocks segwit redeem scripts longer than
the max element size in all the previously mentioned processes (`createmultisig`, `addmultisigaddress`, and
`signrawtransactionwithkey`).
This PR fixes the problem, enabling the creation of multisig output descriptors involving more than 15 keys and
allowing the signing of these scripts, along with other post-segwit redeem scripts that surpass the 520-byte
p2sh limit.
Important note:
Instead of adding support for these longer redeem scripts in the legacy wallet, an "unsupported operation"
error has been added. The reasons behind this decision are:
1) The introduction of this feature brings about a compatibility-breaking change that requires downgrade
protection; older wallets would be unable to interact with these "new" legacy wallets.
2) Considering the ongoing deprecation of the legacy spkm, this issue provides another compelling
reason to transition towards descriptors.
Testing notes:
To easily verify each of the fixes, I decoupled the tests into standalone commits. So they can be
cherry-picked on top of master. Where `rpc_createmultisig.py` (with and without the `--legacy-wallet`
arg) will fail without the bugs fixes commits.
Extra note:
The initial commits improves the `rpc_createmultisig.py` test in many ways. I found this test very
antiquated, screaming for an update and cleanup.
ACKs for top commit:
pinheadmz:
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
theStack:
Code-review ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
achow101:
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
Tree-SHA512: 71794533cbd46b3a1079fb4e9d190d3ea3b615de0cbfa443466e14f05e4616ca90e12ce2bf07113515ea8113e64a560ad572bb9ea9d4835b6fb67b6ae596167f
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.
The process currently fails to sign redeem scripts that are longer than
520 bytes. Even when it shouldn't. The 520 bytes redeem scripts limit
is a legacy p2sh rule, and not a segwit limitation.
Segwit redeem scripts are not restricted by the script item size limit.
The reason why this occurs, is the usage of the same keystore used by
the legacy spkm. Which contains blockage for any redeem scripts longer
than the script item size limit.
fa46cc22bc696e6845915ae91d6b68e36bf4c242 Remove deprecated -rpcserialversion (MarcoFalke)
Pull request description:
The flag is problematic for many reasons:
* It is deprecated
* It is a global flag, requiring a restart to change, as opposed to a flag that can be set on each RPC invocation
* It may be hidden in config files by accident, hard to debug, causing LND crashes and bugs, see https://github.com/bitcoin/bitcoin/issues/28730#issuecomment-1780940868
* It makes performance improvements harder to implement: https://github.com/bitcoin/bitcoin/pull/17529#issuecomment-556082818
Fix all issues by removing it.
If there is a use-case, likely a per-RPC flag can be added, if needed.
ACKs for top commit:
ajtowns:
crACK fa46cc22bc696e6845915ae91d6b68e36bf4c242
TheCharlatan:
lgtm ACK fa46cc22bc696e6845915ae91d6b68e36bf4c242
Tree-SHA512: 96ba1c60356ce93954fe5c2a59045771c6d1516ad0d9dc436ef1800a1f1b0153f0d5fb78ca99d53ad54ba25fbce36962bdf1d4325aceedfc8154a61347a6a915
1bce12acd3e271a7c88d9400b4e3a5645bc8a911 test: add test for `descriptorprocesspsbt` RPC (ishaanam)
fb2a3a70e860aa87fb7a21f6554ed9f3ce901e2d rpc: add descriptorprocesspsbt rpc (ishaanam)
Pull request description:
This PR implements an RPC called `descriptorprocesspsbt`. This RPC is based off of `walletprocesspsbt`, but instead of interacting with the wallet to update, sign and finalize a psbt, it instead accepts an array of output descriptors and uses that information along with information from the mempool, txindex, and the utxo set to do so. `utxoupdatepsbt` also updates a psbt in this manner, but doesn't sign or finalize it. Because of this overlap, a helper function that is added in this PR is called by both `utxoupdatepsbt` and `descriptorprocesspsbt`. Whether or not the helper function signs a psbt is dictated by if the HidingSigningProvider passed to it contains any private information. There is also a test added in this PR for this new RPC that uses p2wsh, p2wpkh, and legacy outputs.
Edit: see https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1228830963
ACKs for top commit:
achow101:
re-ACK 1bce12acd3e271a7c88d9400b4e3a5645bc8a911
instagibbs:
reACK 1bce12acd3
Tree-SHA512: e1d0334739943e71f2ee68b4db7637ebe725da62e7aa4be071f71c7196d2a5970a31ece96d91e372d34454cde8509e95ab0eebd2c8edb94f7d5a781a84f8fc5d
This is a commit in preparation for the next few commits. The functions
are moved to methods to avoid their re-declaration for the purpose of
passing in BlockManager options.
The functions that were now moved into the BlockManager should no longer
use the params as an argument, but instead use the member variable.
In the moved ReadBlockFromDisk and UndoReadFromDisk, change
the function signature to accept a reference to a CBlockIndex instead of
a raw pointer. The pointer is expected to be non-null, so reflect that
in the type.
To allow for the move of functions to BlockManager methods all call
sites require an instantiated BlockManager, or a callback to one.
6e9f8bb050785dbc754b6bb493aad9139908ef98 rpc, tests: in `utxoupdatepsbt` also look for the transaction in the txindex (ishaanam)
a5b4883fb43d01bfef1244df62c64bf8691ca63a 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 6e9f8bb050785dbc754b6bb493aad9139908ef98
Xekyo:
ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98
Tree-SHA512: 078db3c37a1ecd5816d80a42e8bd1341e416d661f508fa5fce0f4e1249fefd7b92a0d45f44957781cb69d0953145ef096ecdd4545ada39062be27742402dac6f
