| Age | Commit message (Collapse) | Author |
|---|
|
receiving in a single descriptor string
a0abcbd3822bd17a1d73c42ccd5b040a150b0501 doc: Mention multipath specifier (Ava Chow)
0019f61fc546b4d5f42eb4086f42560863fe0efb tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d605ac48f1122a836c9aa5f834957ba wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb918bc899a0637f876db31c3419aafd rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4bed9ac168d0b08def8af7485db94ef1 wallet: Move internal to be per key when importing (Ava Chow)
16922455253f47fae0466c4ec6c3adfadcfe9182 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9dca3ca5873d768b3b504cdb2ab947b rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd221501fde3efe11bdba5c6d999dbb323 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c965bbd7bcddf9656eca9cee14c3aec tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2dae4599d04c79aaacf7c5db00b2e707f descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02bc20e5c1be773443dd74d8806d953b descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b103461a98689fd5d382e8da29037f55bea descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae157f4f8226b2419d55e7dc0dfb6e4aec descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f723d62c7ec6768f7f592c09ba2047d9e descriptors: Add PubkeyProvider::Clone (Ava Chow)
Pull request description:
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.
This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.
The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes #17190
ACKs for top commit:
darosior:
re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
mjdietzx:
reACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
pythcoiner:
reACK a0abcbd
furszy:
Code review ACK a0abcbd
glozow:
light code review ACK a0abcbd3822
Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
|
|
When given a multipath descriptor, derive all of the descriptors.
The derived addresses will be returned in an object
consisting of multiple arrays. For compatibility, when given a single path
descriptor, the addresses are provided in a single array as before.
|
|
When given a descriptor which contins a multipath derivation specifier,
a vector of descriptors will be returned.
|
|
|
|
size limit
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](https://github.com/bitcoin/bitcoin/blob/ded687334031f4790ef6a36b999fb30a79dcf7b3/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
|
|
These are simple (and hopefully obviously correct) copies that can be moves
instead.
|
|
The multisig script generation process currently fails when
the user exceeds 15 keys, even when it shouldn't. The maximum
number of keys allowed for segwit redeem scripts (p2sh-segwit
and bech32) is 20 keys.
This is because the redeem script placed in the witness is not
restricted by the item size limit.
The reason behind this issue is the utilization of the legacy
p2sh redeem script restrictions on segwit ones. Redeem scripts
longer than 520 bytes are blocked from being inserted into the
keystore, which causes the signing process and the descriptor
inference process to fail.
This occurs because the multisig generation flow uses the same
keystore as the legacy spkm (FillableSigningProvider), which
contains the 520-byte limit.
|
|
In the helper `HexToPubKey`, check for three different causes of legacy
public key parsing errors (in this order):
- pubkey is not a hex string
- pubkey doesn't have a valid length (33 or 65 bytes) [NEW]
- pubkey is cryptographically invalid, i.e. not on curve
(`IsFullyValid` check)
and throw a specific error message for each one. Note that the error
code is identical for all of them (-5), so this doesn't break RPC API
compatibility.
The helper is currently used for the RPCs `createmultisig` and
`addmultisigaddress`. The length checks can be removed from the
call-sites and error message checks in the functional tests are adapted.
|
|
P2PK scripts are not PKHash destinations, they should have their own
type.
This also results in no longer showing a p2pkh address for p2pk outputs.
However for backwards compatibility, ListCoinst will still do this
conversion.
|
|
Remove standard.h from files that don't use anything in it, and include
it in files that do.
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i -e "/Deprecated alias for OMITTED, can be removed/d" src/rpc/util.h src/rpc/util.cpp
sed -i -e "s/OMITTED_NAMED_ARG/OMITTED/g" $(git grep -l "OMITTED_NAMED_ARG" src/)
-END VERIFY SCRIPT-
|
|
|
|
add a tr() descriptor example to the help deriveaddresses examples
|
|
2147483647 is the maximum positive value of a signed int32, and - currently -
the maximum value that the deriveaddresses bitcoin RPC call accepts as
derivation index due to its input validation routines.
Before this change, when the derivation index (and thus range_end) reached
std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which
is declared as int, and as such 32 bits in size on most platforms) would be
incremented at the end of the first iteration and then warp back to
-2147483648. This caused SIGABRT in bitcoind and a core dump.
This change assigns "i" an explicit size of 64 bits on every platform,
sidestepping the problem.
Fixes #26274.
|
|
|
|
createmultisig and addmultisigaddress
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's|\<get_int64\>|getInt<int64_t>|g' $(git grep -l get_int ':(exclude)src/univalue')
sed -i 's|\<get_int\>|getInt<int>|g' $(git grep -l get_int ':(exclude)src/univalue')
-END VERIFY SCRIPT-
|
|
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
