| Age | Commit message (Collapse) | Author |
|---|
|
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
|
