2451a217dd test: addmultisigaddress, coverage for script size limits (furszy)
53302a0981 bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes (furszy)
9be6065cc0 test: coverage for 16-20 segwit multisig scripts (furszy)
9d9a91c4ea rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey (furszy)
0c9fedfc45 fix incorrect multisig redeem script size limit for segwit (furszy)
f7a173b578 test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' (furszy)
4f33dbd8f8 test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' (furszy)
25a81705d3 test: rpc_createmultisig, remove unnecessary checkbalances() (furszy)
b5a3289433 test: refactor, multiple cleanups in rpc_createmultisig.py (furszy)
3635d43268 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 2451a217dd
theStack:
Code-review ACK 2451a217dd
achow101:
ACK 2451a217dd
Tree-SHA512: 71794533cbd46b3a1079fb4e9d190d3ea3b615de0cbfa443466e14f05e4616ca90e12ce2bf07113515ea8113e64a560ad572bb9ea9d4835b6fb67b6ae596167f
Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed
on 'p2sh-segwit' and 'bech32' redeem scripts.
Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for
segwit output types, we don't want to enable this feature in legacy wallets for the
following reasons:
1) It introduces a compatibility-breaking change requiring 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 adds another
good reason to transition towards descriptors.
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.
Both RPC and GUI now render a useful error message instead of (silently) failing.
Replace bool with util::Result<void> to clarify that this either succeeds or returns an error message.
-BEGIN VERIFY SCRIPT-
regex_string='^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|CHAR_EQUALS_INT8|CLIENT_VERSION_BUILD|CLIENT_VERSION_IS_RELEASE|CLIENT_VERSION_MAJOR|CLIENT_VERSION_MINOR|COPYRIGHT_HOLDERS|COPYRIGHT_HOLDERS_FINAL|COPYRIGHT_HOLDERS_SUBSTITUTION|COPYRIGHT_YEAR|ENABLE_ARM_SHANI|ENABLE_AVX2|ENABLE_EXTERNAL_SIGNER|ENABLE_SSE41|ENABLE_TRACING|ENABLE_WALLET|ENABLE_X86_SHANI|ENABLE_ZMQ|HAVE_BOOST|HAVE_BUILTIN_CLZL|HAVE_BUILTIN_CLZLL|HAVE_BYTESWAP_H|HAVE_CLMUL|HAVE_CONSENSUS_LIB|HAVE_CXX20|HAVE_DECL_BE16TOH|HAVE_DECL_BE32TOH|HAVE_DECL_BE64TOH|HAVE_DECL_BSWAP_16|HAVE_DECL_BSWAP_32|HAVE_DECL_BSWAP_64|HAVE_DECL_FORK|HAVE_DECL_FREEIFADDRS|HAVE_DECL_GETIFADDRS|HAVE_DECL_HTOBE16|HAVE_DECL_HTOBE32|HAVE_DECL_HTOBE64|HAVE_DECL_HTOLE16|HAVE_DECL_HTOLE32|HAVE_DECL_HTOLE64|HAVE_DECL_LE16TOH|HAVE_DECL_LE32TOH|HAVE_DECL_LE64TOH|HAVE_DECL_PIPE2|HAVE_DECL_SETSID|HAVE_DECL_STRERROR_R|HAVE_DEFAULT_VISIBILITY_ATTRIBUTE|HAVE_DLFCN_H|HAVE_DLLEXPORT_ATTRIBUTE|HAVE_ENDIAN_H|HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR|HAVE_FDATASYNC|HAVE_GETENTROPY_RAND|HAVE_GETRANDOM|HAVE_GMTIME_R|HAVE_INTTYPES_H|HAVE_LIBADVAPI32|HAVE_LIBCOMCTL32|HAVE_LIBCOMDLG32|HAVE_LIBGDI32|HAVE_LIBIPHLPAPI|HAVE_LIBKERNEL32|HAVE_LIBOLE32|HAVE_LIBOLEAUT32|HAVE_LIBSHELL32|HAVE_LIBSHLWAPI|HAVE_LIBUSER32|HAVE_LIBUUID|HAVE_LIBWINMM|HAVE_LIBWS2_32|HAVE_MALLOC_INFO|HAVE_MALLOPT_ARENA_MAX|HAVE_MINIUPNPC_MINIUPNPC_H|HAVE_MINIUPNPC_UPNPCOMMANDS_H|HAVE_MINIUPNPC_UPNPERRORS_H|HAVE_NATPMP_H|HAVE_O_CLOEXEC|HAVE_POSIX_FALLOCATE|HAVE_PTHREAD|HAVE_PTHREAD_PRIO_INHERIT|HAVE_STDINT_H|HAVE_STDIO_H|HAVE_STDLIB_H|HAVE_STRERROR_R|HAVE_STRINGS_H|HAVE_STRING_H|HAVE_STRONG_GETAUXVAL|HAVE_SYSCTL|HAVE_SYSCTL_ARND|HAVE_SYSTEM|HAVE_SYS_ENDIAN_H|HAVE_SYS_PRCTL_H|HAVE_SYS_RESOURCES_H|HAVE_SYS_SELECT_H|HAVE_SYS_STAT_H|HAVE_SYS_SYSCTL_H|HAVE_SYS_TYPES_H|HAVE_SYS_VMMETER_H|HAVE_THREAD_LOCAL|HAVE_TIMINGSAFE_BCMP|HAVE_UNISTD_H|HAVE_VM_VM_PARAM_H|LT_OBJDIR|PACKAGE_BUGREPORT|PACKAGE_NAME|PACKAGE_STRING|PACKAGE_TARNAME|PACKAGE_URL|PACKAGE_VERSION|PTHREAD_CREATE_JOINABLE|QT_QPA_PLATFORM_ANDROID|QT_QPA_PLATFORM_COCOA|QT_QPA_PLATFORM_MINIMAL|QT_QPA_PLATFORM_WINDOWS|QT_QPA_PLATFORM_XCB|QT_STATICPLUGIN|STDC_HEADERS|STRERROR_R_CHAR_P|USE_ASM|USE_BDB|USE_DBUS|USE_NATPMP|USE_QRCODE|USE_SQLITE|USE_UPNP|_FILE_OFFSET_BITS|_LARGE_FILES)'
exclusion_files=":(exclude)src/minisketch :(exclude)src/crc32c :(exclude)src/secp256k1 :(exclude)src/crypto/sha256_arm_shani.cpp :(exclude)src/crypto/sha256_avx2.cpp :(exclude)src/crypto/sha256_sse41.cpp :(exclude)src/crypto/sha256_x86_shani.cpp"
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.cpp' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do line_number=$(awk -v my_file="$file" '/\/\/ file COPYING or https?:\/\/www.opensource.org\/licenses\/mit-license.php\./ {line = NR} /^\/\// && NR == line + 1 {while(getline && /^\/\//) line = NR} END {print line+1}' "$file"); sed -i "${line_number}i\\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
git grep --perl-regexp --files-with-matches "$regex_string" -- '*.h' $exclusion_files | xargs git grep -L "bitcoin-config.h" | while read -r file; do sed -i "/#define.*_H/a \\\\n\#if defined(HAVE_CONFIG_H)\\n#include <config/bitcoin-config.h>\\n\#endif" "$file"; done;
for file in $(git grep --files-with-matches 'bitcoin-config.h' -- '*.cpp' '*.h' $exclusion_files); do if ! grep -q --perl-regexp "$regex_string" $file; then sed -i '/HAVE_CONFIG_H/{N;N;N;d;}' $file; fi; done;
-END VERIFY SCRIPT-
The first command creates a regular expression for matching all bitcoin-config.h symbols in the following form: ^(?!//).*(AC_APPLE_UNIVERSAL_BUILD|BOOST_PROCESS_USE_STD_FS|...|_LARGE_FILES). It was generated with:
./autogen.sh && printf '^(?!//).*(%s)' $(awk '/^#undef/ {print $2}' src/config/bitcoin-config.h.in | paste -sd "|" -)
The second command holds a list of files and directories that should not be processed. These include subtree directories as well as some crypto files that already get their symbols through the makefile.
The third command checks for missing bitcoin-config headers in .cpp files and adds the header if it is missing.
The fourth command checks for missing bitcoin-config headers in .h files and adds the header if it is missing.
The fifth command checks for unneeded bitcoin-config headers in sources files and removes the header if it is unneeded.
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.
Replaces the constructor in CScriptID that converts a ScriptHash with a
function ToScriptID that does the same. This prepares for a move of
CScriptID to avoid a circular dependency.
Any identifier starting with two _, or one _ followed by a capital letter is reserved for the compiler and thus must not be used. See: https://stackoverflow.com/a/228797/7130273
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s '__pushKV' 'pushKVEnd'
s '_EraseTx' 'EraseTxNoLock'
s '_Other' 'Other'
-END VERIFY SCRIPT-
fe49f06c0e doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46 Switch hardened derivation marker to h in descriptors (Sjors Provoost)
Pull request description:
This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.
For example the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`, avoiding the need for escape characters. With this change `listdescriptors` will use `h`, so you can copy-paste the result, without having to add escape characters or switch `'` to 'h' manually.
Both markers can still be parsed.
The `hdkeypath` field in `getaddressinfo` is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.
See discussion in #15740
ACKs for top commit:
achow101:
ACK fe49f06c0e
darosior:
re-ACK fe49f06c0e
Tree-SHA512: f78bc873b24a6f7a2bf38f5dd58f2b723e35e6b10e4d65c36ec300e2d362d475eeca6e5afa04b3037ab4bee0bf8ebc93ea5fc18102a2111d3d88fc873c08dc89
9141e4395a rpc, docs: Add note for commands that supports only legacy wallets (Yusuf Sahin HAMZA)
Pull request description:
Refs #25363, apparently issue is not updated since over a month, so i decided to put the same `importaddress` note in #25368 to other rpc commands that needs this note.
Note is added for following commands:
- `importprivkey`
- `importpubkey`
- `importwallet`
- `dumpprivkey`
- `dumpwallet`
- `importmulti`
- `addmultisigaddress`
- `sethdseed`
ACKs for top commit:
achow101:
ACK 9141e4395a
Tree-SHA512: f3dc05d26577fd8dbe2bd69cb5c14ffccebacd6010402af44427b3d01be8484895dfcf33d55dfa766eadb7f9f3bae5cc4c2add3ac816a2ac60e8beb5a97527f3
7ccdd741fe test: fix importmulti/importdescriptors assertion (Jon Atack)
19d888ce40 rpc: move WALLET_FLAG_CAVEATS to the compilation unit of its caller (Jon Atack)
01df011ca2 doc: release note for wallet RPCs "warning" field deprecation (Jon Atack)
9ea8b3739a test: createwallet "warning" field deprecation test (Jon Atack)
645d7f75ac rpc: deprecate "warning" field in {create,load,unload,restore}wallet (Jon Atack)
2f4a926e95 test: add test coverage for "warnings" field in createwallet (Jon Atack)
4a1e479ca6 rpc: add "warnings" field to RPCs {create,load,unload,restore}wallet (Jon Atack)
079d8cdda8 rpc: extract wallet "warnings" fields to a util helper (Jon Atack)
f73782a903 doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack)
Pull request description:
Based on discussion and concept ACKed in #27138, add a `warnings` field to RPCs createwallet, loadwallet, unloadwallet, and restorewallet as a JSON array of strings to replace the `warning` string field in these 4 RPCs. The idea is to more gracefully handle multiple warning messages and for consistency with other wallet RPCs. Then, deprecate the latter fields, which represent all the remaining RPC `warning` fields.
The first commit f73782a903 implements https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474789198 as an alternative to #27138. One of those two could potentially be backported to our currently supported releases.
ACKs for top commit:
achow101:
ACK 7ccdd741fe
1440000bytes:
utACK 7ccdd741fe
vasild:
ACK 7ccdd741fe
pinheadmz:
re-ACK 7ccdd741fe
Tree-SHA512: 314e0a4c41fa383d95e2817bfacf359d449e460529d235c3eb902851e2f4eacbabe646d9a5a4beabc4964cdfabf6397ed8301366a58d344a2f787f83b75e9d64
Instead of storing and passing around fixed strings for the purpose of
an address, use an enum.
This also rationalizes the CAddressBookData struct, documenting all fields and
making them public, and simplifying the representation to avoid bugs like
https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114 and make
it not possible to invalid address data like change addresses with labels.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This makes it easier to handle descriptor strings manually. E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
Both markers can still be parsed. The default for new descriptors is changed to h. In normalized form h is also used. For private keys the chosen marker is preserved in a round trip.
The hdkeypath field in getaddressinfo is also impacted by this change.
-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-
65e78bda7c test: Invalid label name coverage (Aurèle Oulès)
552b51e682 refactor: Add sanity checks in LabelFromValue (Aurèle Oulès)
67e7ba8e1a rpc: Sanitize label name in various RPCs (Aurèle Oulès)
Pull request description:
The following RPCs did not sanitize the optional label name:
- importprivkey
- importaddress
- importpubkey
- importmulti
- importdescriptors
- listsinceblock
Thus is was possible to import an address with a label `*` which should not be possible.
The wildcard label is used for backwards compatibility in the `listtransactions` rpc.
I added test coverage for these RPCs.
ACKs for top commit:
ajtowns:
ACK 65e78bda7c
achow101:
ACK 65e78bda7c
furszy:
diff ACK 65e78bd
stickies-v:
re-ACK 65e78bda7c
theStack:
re-ACK 65e78bda7c
Tree-SHA512: ad99f2824d4cfae352166b76da4ca0069b7c2eccf81aaa0654be25bbb3c6e5d6b005d93960f3f4154155f80e12be2d0cebd5529922ae3d2a36ee4eed82440b31
Note is added for following rpc commands:
importprivkey, importpubkey, importwallet, dumpprivkey,
dumpwallet, importmulti, addmultisigaddress, sethdseed
Rename `BResult` class to `util::Result` and update the class interface to be
more compatible with `std::optional` and with a full-featured result class
implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for
this change is to update existing `BResult` usages now so they don't have to
change later when more features are added in #25665.
This change makes the following improvements originally implemented in #25665:
- More explicit API. Drops potentially misleading `BResult` constructor that
treats any bilingual string argument as an error. Adds `util::Error`
constructor so it is never ambiguous when a result is being assigned an error
or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return
values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same
operators and methods as `std::optional`. `BResult` had a less familiar
interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj
naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so
naming reflects code organization and it is obvious where the class is coming
from. Drops "B" from name because it is undocumented what it stands for
(bilingual?)
- Has unit tests.
This is required for removing the UniValue copy constructor.
-BEGIN VERIFY SCRIPT-
sed -i 's/return NullUniValue/return UniValue::VNULL/g' $(git grep -l NullUniValue ':(exclude)src/univalue')
-END VERIFY SCRIPT-
a2a92317ad rpc: Add warning to user about newkeypool command (Samuel Dobson)
Pull request description:
This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe.
David Harding also suggested [here](https://github.com/bitcoin/bitcoin/pull/23093#issuecomment-945350003) that the RPC help text should include a warning to the users about the interaction between newkeypool.
ACKs for top commit:
achow101:
ACK a2a92317ad
Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048
d5cab1a96d Add createmultisig and addmultisigaddress warnings release note (Samuel Dobson)
e46fc935aa Add warnings field to addmultisigaddress to warn about uncompressed keys (Samuel Dobson)
d1a9742623 Add warnings field to createmultisig to warn about uncompressed keys (Samuel Dobson)
Pull request description:
Fixes#21368
Currently, if there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. Rather than keeping this silent behaviour which may be confusing to users, we explicitly add a `warnings` field which will warn the user why their address format is different.
ACKs for top commit:
achow101:
ACK d5cab1a96d
Tree-SHA512: c2ac7f7689251bd4fcd8c26506f053921fbaf34c7a26a74e82ebc7f82cc0bd25407fd7954bf98365dcafa51fa45dcdbee6214320580ca69509690c3555e71cc0
