When creating Taproot signatures, if the key being signed for is known
to be a MuSig2 aggregate key, do the MuSig2 signing algorithms.
First try to create the aggregate signature. This will fail if there are
not enough partial signatures or public nonces. If it does fail, try to
create a partial signature with all participant keys. This will fail for
those keys that we do not have the private keys for, and if there are
not enough public nonces. Lastly, if the partial signatures could not be
created, add our own public nonces for the private keys that we know, if
they do not yet exist.
Adds GetMuSig2SecNonces which returns secp256k1_musig_secnonce*, and
DeleteMuSig2Session which removes the MuSig2 secnonce from wherever it
was retrieved. FlatSigningProvider stores it as a pointer to a map of
session id to secnonce so that deletion will actually delete from the
object that actually owns the secnonces.
The session id is just a unique identifier for the caller to determine
what secnonces have been created.
`using script_verify_flags = uint32_t` allows implicit conversion to
and from int, so replace it with a class to have the compiler ensure we
use the correct type. Provide from_int and as_int to allow for explicit
conversions when desired.
Introduces the type `script_verify_flag_name` for the individual flag
name enumeration.
Previously the SCRIPT_VERIFY_* flags were specified as either uint32_t,
unsigned int, or unsigned. This converts them to a common type alias in
preparation for changing the underlying type.
Moves FormatScriptFlags logic into GetScriptFlagNames which returns a
vector of strings. For completeness, also has GetScriptFlagNames report
on any bits that do not match a known script flag.
While reviewing the MuSig2 descriptors PR 31244, I realized that the enum
`DeriveType` here logically refers to the derive type for ranged descriptors.
This became evident to me while going through the implementations of `IsRange`
& `IsHardened` functions of `BIP32PubkeyProvider`, and the `ParsePubkeyInner`
function. Initially I got confused by reading `IsRange` translating to
`!= DeriveType::NO`, but later realised it specifically referred to the presence
of ranged derivations. I propose explicitly mentioning "RANGED" in the values
of the `DeriveType` enum would make it easier to parse the descriptors code.
This enum is used in one file only - `script/descriptors.cpp`. That's why I
explicitly passed it as the argument in the `sed` commands in the script below.
-BEGIN VERIFY SCRIPT-
sed -i 's/HARDENED\b/HARDENED_RANGED/g' src/script/descriptor.cpp
sed -i 's/\bNO\b/NON_RANGED/g' src/script/descriptor.cpp
-END VERIFY SCRIPT-
The current `prevector` size of 28 bytes (chosen to fill the `sizeof(CScript)` aligned size) was introduced in 2015 (https://github.com/bitcoin/bitcoin/pull/6914) before SegWit and TapRoot.
However, the increasingly common `P2WSH` and `P2TR` scripts are both 34 bytes, and are forced to use heap (re)allocation rather than efficient inline storage.
The core trade-off of this change is to eliminate heap allocations for common 34-36 byte scripts at the cost of increasing the base memory footprint of all `CScript` objects by 8 bytes (while still respecting peak memory usage defined by `-dbcache`).
Increasing the `prevector` size allows these scripts to be stored inline, avoiding extra heap allocations, reducing potential memory fragmentation, and improving performance during cache flushes. Massif analysis confirms a lower stable memory usage after flushing, suggesting the elimination of heap allocations outweighs the larger base size for common workloads.
Due to memory alignment, increasing the `prevector` size to 36 bytes doesn't change the overall `sizeof(CScript)` compared to an increase to 34 bytes, allowing us to include `P2PK` scripts as well at no additional memory cost.
Performance benchmarks for AssumeUTXO load and flush show:
* Small dbcache (450MB): ~1-3% performance improvement (despite more frequent flushes)
* Large dbcache (4500MB): ~6-8% performance improvement due to fewer heap allocations (and basically the number of flushes)
* Very large dbcache (4500MB): ~5-6% performance improvement due to fewer heap allocations (and memory limit not being reached, so there's no memory penalty)
Full IBD and reindex-chainstate with larger `dbcache` values also show an overall ~3-4% speedup.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf miniscript: Make `operator""_mst` `consteval` (Pieter Wuille)
14052162b19ac22f465f7db7880a6ab5d588a98c Revert "miniscript: make operator_mst consteval" (Hennadii Stepanov)
Pull request description:
Same as https://github.com/bitcoin/bitcoin/pull/28657, but without the refactoring required to work around [fixed](https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353) MSVC bugs.
The second commit has been taken from https://github.com/bitcoin/bitcoin/pull/29167.
ACKs for top commit:
sipa:
ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf
hodlinator:
re-ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf
Tree-SHA512: 8b531f9d6c450a8a5218865da05ffb5093d09ce2c0bee9874c0160795c4b1713928730d894ea3cd0b12b133346971ae3a00ed2fe8d9fd8a50b67a74ef81fde98
This reverts commit 63317103c9f2b0635558da814567bb79c17ae851.
operator""_mst has been manually adjusted according to commit
faf21625652fd0d4bbf9b86fd9ebedb5857505ea
When parsing a descriptor, it is useful to be able to check whether a
string begins with a substring without consuming that substring as
another function such as Func() will be used later which requires that
substring to be present at the beginning.
Specifically, for MuSig2, this modified Const will be used to determine
whether a an expression begins with "musig(" before a subsequent
Func("musig", ...) is used.
Instead of GetPrivKey returning a key and having the caller fill the
FlatSigningProvider, have GetPrivKey take the FlatSigningProvider and
fill it by itself. This will be necessary for descriptors such as
musig() where there are private keys that need to be added to the
FlatSigningProvider but do not directly appear in any resulting scripts.
GetPrivKey is now changed to void as the caller no longer cares whether
it succeeds or fails.
Instead of having ExpandHelper fill in the origins in the
FlatSigningProvider output, have GetPubKey do it by itself. This reduces
the extra variables needed in order to track and set origins in
ExpandHelper.
Also changes GetPubKey to return a std::optional<CPubKey> rather than
using a bool and output parameters.
Legacy wallets should only import keys to the keypool if they came in a
single key descriptor. Instead of relying on assumptions about the
descriptor based on how many pubkeys show up after expanding the
descriptor, explicitly mark descriptors as being single key type and use
that for the check.
ff0194a7ce9dabf1b31b64ca584e45840dce8141 miniscript: convert non-critical asserts to CHECK_NONFATAL (Antoine Poinsot)
Pull request description:
The Miniscript code contains assertions to prevent ending up in an insane state or prevent UB, but also to enforce logical invariants. For the latter it is not necessary to crash the program if they are broken. Raising an exception suffices, especially as this code is often called through the RPC interface which can in turn handle the exception and the user can report it to developers.
This revives #28678 from Pieter Wuille.
ACKs for top commit:
hodlinator:
ACK ff0194a7ce9dabf1b31b64ca584e45840dce8141
TheCharlatan:
ACK ff0194a7ce9dabf1b31b64ca584e45840dce8141
brunoerg:
code review ACK ff0194a7ce9dabf1b31b64ca584e45840dce8141
Tree-SHA512: 8ed8f7b494e46ecf7cdebe75120cd0ffe543b6bc289bf882dac631fe2ec2cae590d5f7bc2316e52db085791694b136dffbc71c40c1e16886fa53ab00bd8cabd0
* Range-for avoids ++i/i++ debate and decreases linecount.
* seen_multipath is only used if multipath_segment_index hasn't already been set. Rename it to seen_substitutes to better describe what it does, now that the context implies its involved in multipath.
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong
error code into this 1 place. Every other situation where the value stack
lacks the required number of arguments for the op-code,
SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
Also in this commit:
- Fix script_tests to adjust to the corrected error message
- Fix p2p_invalid_tx functional test to produce the desired error message
ffff4a293ad878494e12f8f00108cc99ee2b713e bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97bcb1790a7cd4363a13fda7c80c3dd90 refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b40c97375af0722f32f7575bca3af819 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179c062b7ca92d120455ce02a9f4e9e19 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e6e80e3da1ab6448b6212244bafa5d3 scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c03db00a2be3f703aa7e5eec4312bd2e refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717ec18d64b7ff7bba71b8f0c202ba31d test: Fix broken span_tests (MarcoFalke)
fadf02ef8bf96ad5b3b8e34fd425b31b555f4371 refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be17fa9e7c91188710e6a04939ceab11 refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293ad878494e12f8f00108cc99ee2b713e
theuni:
ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
21e9d39a3725cd6107b742f0cb97f65b3640201b docs: add release notes for 31603 (brunoerg)
a8b548d75d9a376c9bb66e06bb918c876416d615 test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62cf5d3ea9b98d5a76e4e54cac07bc3c test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea16a04844c83e56fd6deaa1f0dc0a7e descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef6c02ecbaa0cf448567355b6b86b510 test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
darosior:
re-ACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
sipa:
utACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
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-
