b63428ac9ce2c903670409b3e47b9f6730917ae8 rpc: refactor: use more (Maybe)Arg<std::string_view> (stickies-v)
037830ca0dbb6ede9f9d72691c756f4bae6c97e2 refactor: increase string_view usage (stickies-v)
b3bf18f0bac0ffe18206ee20642e11264ba0c99d rpc: refactor: use string_view in Arg/MaybeArg (stickies-v)
Pull request description:
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](d127b25199 (diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a32374R32))) with raw pointers being implemented.
This PR aims to improve on this by returning a trivially copyable `std::string_view` (`Arg`) or `std::optional<std::string_view>` (`MaybeArg`), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using `std::is_trivially_copyable_v` instead of defining the types manually.
In cases where functions currently take a `const std::string&` and it would be too much work / touching consensus logic to update them (`signmessage.cpp`), a `std::string` copy is made (which was already happening anyway).
The last 2 commits increase usage of the `{Arg,MaybeArg}<std::string_view>` helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.
ACKs for top commit:
maflcko:
re-ACK b63428ac9ce2c903670409b3e47b9f6730917ae8 🎉
achow101:
ACK b63428ac9ce2c903670409b3e47b9f6730917ae8
pablomartin4btc:
re-ACK [b63428a](b63428ac9c)
w0xlt:
reACK b63428ac9c
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
faa9d10c84bc6b465cbca266468990cc716b4300 refactor: Construct g_verify_flag_names on first use (MarcoFalke)
Pull request description:
The current usage of the `g_verify_flag_names` map seems fine and I can not see a static initialization order fiasco here.
However, it seems brittle to hope this remains the case in the future. Also, it triggers a msan false-positive in the fuzz CI task. (C.f https://github.com/bitcoin-core/qa-assets/actions/runs/18352815555/job/52413137315?pr=241#step:7:5245)
So just apply the "Construct on first use" idiom.
ACKs for top commit:
kevkevinpal:
ACK [faa9d10](faa9d10c84)
ajtowns:
ACK faa9d10c84bc6b465cbca266468990cc716b4300
janb84:
lgtm ACK faa9d10c84bc6b465cbca266468990cc716b4300
stickies-v:
ACK faa9d10c84bc6b465cbca266468990cc716b4300
Tree-SHA512: 6685dfc91c99a8245722e07fac99a7a6d58586c30964be7ccd74a176dfbf00c6255c8594621e2909640763924f51d3efd4ce65ed65eaeeb1d05c2fd01fe63604
ac599c4a9cb3b2d424932d3fd91f9eed17426827 test: Test MuSig2 in the wallet (Ava Chow)
68ef954c4c59802a6810a462eaa8dd61728ba820 wallet: Keep secnonces in DescriptorScriptPubKeyMan (Ava Chow)
4a273edda0ec10f0c5ae5d94b9925fa334d1c6e6 sign: Create MuSig2 signatures for known MuSig2 aggregate keys (Ava Chow)
258db938899409c8ee1cef04e16ba1795ea0038d sign: Add CreateMuSig2AggregateSig (Ava Chow)
bf69442b3f5004dc3df5a1b1d752114ba68fa5f4 sign: Add CreateMuSig2PartialSig (Ava Chow)
512b17fc56eac3a2e2b9ba489b5423d098cce0db sign: Add CreateMuSig2Nonce (Ava Chow)
82ea67c607cde6187d7082429d27b927dc21c0c6 musig: Add MuSig2AggregatePubkeys variant that validates the aggregate (Ava Chow)
d99a081679e16668458512aba2fd13a3e1bdb09f psbt: MuSig2 data in Fill/FromSignatureData (Ava Chow)
4d8b4f53363f013ed3972997f0b05b9c19e9db9d signingprovider: Add musig2 secnonces (Ava Chow)
c06a1dc86ff2347538e95041ab7b97af25342958 Add MuSig2SecNonce class for secure allocation of musig nonces (Ava Chow)
9baff05e494443cd82708490f384aa3034ad43bd sign: Include taproot output key's KeyOriginInfo in sigdata (Ava Chow)
4b24bfeab9d6732aae3e69efd33105792ef1198f pubkey: Return tweaks from BIP32 derivation (Ava Chow)
f14876213aad0e67088b75cae24323db9f2576d8 musig: Move synthetic xpub construction to its own function (Ava Chow)
fb8720f1e09f4e41802f07be53fb220d6f6c127f sign: Refactor Schnorr sighash computation out of CreateSchnorrSig (Ava Chow)
a4cfddda644f1fc9a815b2d16c997716cd63554a tests: Clarify why musig derivation adds a pubkey and xpub (Ava Chow)
39a63bf2e7e38dd3f30b5d1a8f6b2fff0e380d12 descriptors: Add a doxygen comment for has_hardened output_parameter (Ava Chow)
2320184d0ea87279558a8e6cbb3bccf5ba1bb781 descriptors: Fix meaning of any_key_parsed (Ava Chow)
Pull request description:
This PR implements MuSig2 signing so that the wallet can receive and spend from imported `musig(0` descriptors.
The libsecp musig module is enabled so that it can be used for all of the MuSig2 cryptography.
Secnonces are handled in a separate class which holds the libsecp secnonce object in a `secure_unique_ptr`. Since secnonces must not be used, this class has no serialization and will only live in memory. A restart of the software will require a restart of the MuSig2 signing process.
ACKs for top commit:
fjahr:
tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
rkrux:
lgtm tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
theStack:
Code-review ACK ac599c4a9cb3b2d424932d3fd91f9eed17426827 🗝️
Tree-SHA512: 626b9adc42ed2403e2f4405321eb9ce009a829c07d968e95ab288fe4940b195b0af35ca279a4a7fa51af76e55382bad6f63a23bca14a84140559b3c667e7041e
Modernizes interface by not forcing users to deal with raw pointers,
without adding copying overhead. Generalizes the logic of whether
we return by value or by optional/pointer.
In cases where functions take a `const std::string&` and it would
be too much work to update them, a string copy is made (which was
already happening anyway).
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.
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.
