In the MuSig2 signing flow, the same sighash ends up being recalculated for all the participant public keys involved in the MuSig aggregate keys. This happens for all the three parts of the signing flow: nonce, partial sig, and aggregate sig.
The original intent of this refactor to reduce seeing duplicate code, but it should also help in improving performance of the wallet signing operations.
This was highlighted during the review of the MuSig2 signing PR: #29675 (review)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33665.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | Raimo33 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
https://github.com/bitcoin/bitcoin/actions/runs/18676072881/job/53246386885?pr=33665#step:9:3427:
0Run script_sign with args ['/home/admin/actions-runner/_work/_temp/build/bin/fuzz', '-runs=1', PosixPath('/home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/script_sign')]INFO: Running with entropic power schedule (0xFF, 100).
1INFO: Seed: 2902865681
2INFO: Loaded 1 modules (615865 inline 8-bit counters): 615865 [0x56e10e6f9bb8, 0x56e10e790171),
3INFO: Loaded 1 PC tables (615865 PCs): 615865 [0x56e10e790178,0x56e10f0f5d08),
4INFO: 4186 files found in /home/admin/actions-runner/_work/_temp/ci/scratch/qa-assets/fuzz_corpora/script_sign
5INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 938660 bytes
6INFO: seed corpus: files: 4186 min: 9b max: 938660b total: 57347949b rss: 105Mb
7[#1024](/bitcoin-bitcoin/1024/) pulse cov: 12906 ft: 62748 corp: 979/178Kb exec/s: 512 rss: 162Mb
8/home/admin/actions-runner/_work/_temp/src/script/sign.cpp:84:60: runtime error: implicit conversion from type 'int' of value -2147483648 (32-bit, signed) to type 'uint8_t' (aka 'unsigned char') changed the value to 0 (8-bit, unsigned)
9 [#0](/bitcoin-bitcoin/0/) 0x56e10bf84088 in MutableTransactionSignatureCreator::ComputeSchnorrSignatureHash(uint256 const*, SigVersion) const /home/admin/actions-runner/_work/_temp/build/src/./script/sign.cpp:84:60
10 [#1](/bitcoin-bitcoin/1/) 0x56e10bf9f6d7 in SignMuSig2(BaseSignatureCreator const&, SignatureData&, SigningProvider const&, std::vector<unsigned char, std::allocator<unsigned char>>&, XOnlyPubKey const&, uint256 const*, uint256 const*, SigVersion) /home/admin/actions-runner/_work/_temp/build/src/./script/sign.cpp:285:46
11 [#2](/bitcoin-bitcoin/2/) 0x56e10bf8bdd9 in SignTaproot(SigningProvider const&, BaseSignatureCreator const&, WitnessV1Taproot const&, SignatureData&, std::vector<std::vector<unsigned char, std::allocator<unsigned char>>, std::allocator<std::vector<unsigned char, std::allocator<unsigned char>>>>&)::$_0::operator()(XOnlyPubKey const&, uint256 const*) const /home/admin/actions-runner/_work/_temp/build/src/./script/sign.cpp:581:17
12 [#3](/bitcoin-bitcoin/3/) 0x56e10bf8bdd9 in SignTaproot(SigningProvider const&, BaseSignatureCreator const&, WitnessV1Taproot const&, SignatureData&, std::vector<std::vector<unsigned char, std::allocator<unsigned char>>, std::allocator<std::vector<unsigned char, std::allocator<unsigned char>>>>&) /home/admin/actions-runner/_work/_temp/build/src/./script/sign.cpp:587:13
13 [#4](/bitcoin-bitcoin/4/) 0x56e10bf8bdd9 in SignStep(SigningProvider const&, BaseSignatureCreator const&, CScript const&, std::vector<std::vector<unsigned char, std::allocator<unsigned char>>, std::allocator<std::vector<unsigned char, std::allocator<unsigned char>>>>&, TxoutType&, SigVersion, SignatureData&) /home/admin/actions-runner/_work/_temp/build/src/./script/sign.cpp:703:16
14 [#5](/bitcoin-bitcoin/5/) 0x56e10bf86e4c in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) /home/admin/actions-runner/_work/_temp/build/src/./script/sign.cpp:734:19
15 [#6](/bitcoin-bitcoin/6/) 0x56e10bf956de in SignTransaction(CMutableTransaction&, SigningProvider const*, std::map<COutPoint, Coin, std::less<COutPoint>, std::allocator<std::pair<COutPoint const, Coin>>> const&, int, std::map<int, bilingual_str, std::less<int>, std::allocator<std::pair<int const, bilingual_str>>>&) /home/admin/actions-runner/_work/_temp/build/src/./script/sign.cpp:1050:13
16 [#7](/bitcoin-bitcoin/7/) 0x56e10b9c040e in script_sign_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/./test/fuzz/script_sign.cpp:135:19
17 [#8](/bitcoin-bitcoin/8/) 0x56e10bc78e9e in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
18 [#9](/bitcoin-bitcoin/9/) 0x56e10bc78e9e in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5
19 [#10](/bitcoin-bitcoin/10/) 0x56e10bc78e9e in LLVMFuzzerTestOneInput /home/admin/actions-runner/_work/_temp/build/src/test/fuzz/util/./test/fuzz/fuzz.cpp:216:5
20 [#11](/bitcoin-bitcoin/11/) 0x56e10b28521f in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1ab321f) (BuildId: bc4e3a0200c7b1d70a19e1981b37dcb57d3b1225)
21 [#12](/bitcoin-bitcoin/12/) 0x56e10b284829 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1ab2829) (BuildId: bc4e3a0200c7b1d70a19e1981b37dcb57d3b1225)
22 [#13](/bitcoin-bitcoin/13/) 0x56e10b286592 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1ab4592) (BuildId: bc4e3a0200c7b1d70a19e1981b37dcb57d3b1225)
23 [#14](/bitcoin-bitcoin/14/) 0x56e10b286ab0 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1ab4ab0) (BuildId: bc4e3a0200c7b1d70a19e1981b37dcb57d3b1225)
24 [#15](/bitcoin-bitcoin/15/) 0x56e10b273135 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1aa1135) (BuildId: bc4e3a0200c7b1d70a19e1981b37dcb57d3b1225)
25 [#16](/bitcoin-bitcoin/16/) 0x56e10b29f4f6 in main (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1acd4f6) (BuildId: bc4e3a0200c7b1d70a19e1981b37dcb57d3b1225)
26 [#17](/bitcoin-bitcoin/17/) 0x7318303981c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
27 [#18](/bitcoin-bitcoin/18/) 0x73183039828a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
28 [#19](/bitcoin-bitcoin/19/) 0x56e10b267704 in _start (/home/admin/actions-runner/_work/_temp/build/bin/fuzz+0x1a95704) (BuildId: bc4e3a0200c7b1d70a19e1981b37dcb57d3b1225)
In the MuSig2 signing flow, the same sighash ends up being
recalculated for all the participant public keys involved
in the MuSig aggregate keys. This happens for all the
three parts of the signing flow: nonce, partial sig, and
aggregate sig.
The original intent of this refactor to reduce seeing
duplicate code, but it should also help in improving
performance of the wallet signing operations.
This was highlighted during the review of the MuSig2
signing PR 29675.
This is done to fix failures in the fuzz tests that occur because of implicit
conversion of `nHashType` from `int` to `uint8_t` when ultimately passed down
to the `SignatureHashSchnorr` function. Narrowing the type for this variable
at this stage fixes the fuzz tests failures.
