wallet: remove redundant sighash calculation in Musig2 signing flow #33665

pull rkrux wants to merge 2 commits into bitcoin:master from rkrux:musig-sighash changing 2 files +38 −36
  1. rkrux commented at 7:17 am on October 21, 2025: contributor

    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)

  2. DrahtBot added the label Wallet on Oct 21, 2025
  3. DrahtBot commented at 7:17 am on October 21, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33665.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK Raimo33, optout21

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32876 (refactor: use options struct for signing and PSBT operations by Sjors)

    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.

  4. rkrux force-pushed on Oct 21, 2025
  5. rkrux force-pushed on Oct 21, 2025
  6. fanquake commented at 8:31 am on October 21, 2025: member

    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)
  7. rkrux force-pushed on Oct 21, 2025
  8. rkrux force-pushed on Oct 21, 2025
  9. rkrux commented at 5:01 pm on October 21, 2025: contributor
    Pushed the second commit to fix the fuzz test failures.
  10. rkrux marked this as ready for review on Oct 21, 2025
  11. rkrux renamed this:
    wallet: refactor to remove redundant sighash calculation
    wallet: remove redundant sighash calculation in Musig2 signing flow
    on Oct 21, 2025
  12. Raimo33 commented at 8:30 am on October 22, 2025: contributor
    Concept ACK
  13. in src/script/sign.cpp:284 in f9362dd135 outdated 
    278@@ -291,6 +279,15 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
279                 if (leaf_hash) it->second.first.insert(*leaf_hash);
280             }
281         }
282+    }
283+
284+    // Compute sighash
    optout21 commented at 6:03 am on November 1, 2025:
    Maybe this could come before the derivation-path-filling loop above. That way the early exit condition on missing sighash comes earlier, and the loop does not have to be broken into two.
    Sjors commented at 10:51 am on November 11, 2025:
    I would also prefer not breaking up this loop.
    rkrux commented at 12:12 pm on November 12, 2025:
    I recall doing it intentionally based on the response I received on a similar suggestion in the MuSig2 signing PR: #29675 (review).
  14. in src/script/sign.cpp:84 in 000c685bcd outdated 
    80@@ -81,7 +81,7 @@ std::optional<uint256> MutableTransactionSignatureCreator::ComputeSchnorrSignatu
81         execdata.m_tapleaf_hash = *leaf_hash;
82     }
83     uint256 hash;
84-    if (!SignatureHashSchnorr(hash, execdata, m_txto, nIn, nHashType, sigversion, *m_txdata, MissingDataBehavior::FAIL)) return std::nullopt;
85+    if (!SignatureHashSchnorr(hash, execdata, m_txto, nIn, static_cast<uint8_t>(nHashType), sigversion, *m_txdata, MissingDataBehavior::FAIL)) return std::nullopt;
    optout21 commented at 6:08 am on November 1, 2025:
    It seems the proper, but much higher impact solution would be to change the nHashType field to uint8_t, what do you think? Maybe create a separate issue or PR for that?
    Sjors commented at 10:44 am on November 11, 2025:
    A more thorough approach would be to introduce typedef uint8_t HashType, but it requires changes all over the codebase.
  15. optout21 commented at 6:09 am on November 1, 2025: none
    Concept ACK 000c685bcdd395d61a58ad9698563cab41e2d7d0 (more review needed)
  16. Sjors commented at 10:25 am on November 11, 2025: member
    Each commit should leave the code in a valid state, so it would make sense to move 000c685bcdd395d61a58ad9698563cab41e2d7d0 up.
  17. in src/script/sign.cpp:286 in f9362dd135 outdated 
    278@@ -291,6 +279,15 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
279                 if (leaf_hash) it->second.first.insert(*leaf_hash);
280             }
281         }
282+    }
283+
284+    // Compute sighash
285+    std::optional<uint256> sighash = creator.ComputeSchnorrSignatureHash(leaf_hash, sigversion);
286+    if (!sighash.has_value()) return false;
    Sjors commented at 10:54 am on November 11, 2025:
    Since we do an early return here, it’s nicer to make sighash (or sig_hash) not an optional, so below you don’t need dereference pointers.
    rkrux commented at 12:13 pm on November 12, 2025:
    Done
  18. wallet: static cast `nHashType` function argument from `int` to `uint8_t` 
    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.
    3a4ab8bb79
  19. wallet: refactor to remove redundant sighash calculation 
    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.
    ac4a593ff0
  20. in src/script/sign.cpp:967 in f9362dd135 outdated 
    962@@ -966,18 +963,22 @@ class DummySignatureCreator final : public BaseSignatureCreator {
963         sig.assign(64, '\000');
964         return true;
965     }
966-    std::vector<uint8_t> CreateMuSig2Nonce(const SigningProvider& provider, const CPubKey& aggregate_pubkey, const CPubKey& script_pubkey, const CPubKey& part_pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion, const SignatureData& sigdata) const override
967+    std::optional<uint256> ComputeSchnorrSignatureHash(const uint256* leaf_hash, SigVersion sigversion) const override
    Sjors commented at 10:56 am on November 11, 2025:

    Why is this added?

    It also makes the commit diff harder to read.

    rkrux commented at 12:12 pm on November 12, 2025:
    ComputeSchnorrSignatureHash function of the class BaseSignatureCreator had to be made public so that it can be called from the SignMuSig2 function that makes an implementation in the DummySignatureCreator class necessary.
  21. rkrux force-pushed on Nov 12, 2025
  22. rkrux commented at 12:13 pm on November 12, 2025: contributor

    Each commit should leave the code in a valid state, so it would make sense to move https://github.com/bitcoin/bitcoin/commit/000c685bcdd395d61a58ad9698563cab41e2d7d0 up.

    Makes sense, done.

  23. achow101 commented at 10:57 pm on November 19, 2025: member

    The original intent of this refactor to reduce seeing duplicate code

    Considering how much extra is being changed to avoid the duplication, I don’t really think this is worth it.

    but it should also help in improving performance of the wallet signing operations.

    I don’t think that’s the case. The MuSig2 functions that are all recomputing the same sighash are never actually doing so in the same call to SignMuSig2. CreateMuSig2AggregateSig exits early when there aren’t enough pubnonces or partial sigs, before the sighash is computed. CreateMuSig2PartialSig exits early when there aren’t enough pubnonces. It really ends up being that the sighash is basically computed once per SignMuSig2 call before the deduplication anyways. The only situation where it is computed multiple times is if the wallet has multiple keys in the musig, but that should rarely happen.

    This change also ends up making MuSig2’s behavior different from the other signing functions. The MuSig2 signing functions are the only ones which require the caller to provide the sighash. Arguably, for TransactionSignatureCreateor, what the sighash is is an internal implementation detail, not something that callers need to know about.

  24. rkrux commented at 2:21 pm on November 24, 2025: contributor

    The only situation where it is computed multiple times is if the wallet has multiple keys in the musig, but that should rarely happen.

    One of the reasons that made me raise the PR was this case, but as highlighted it’d occur rarely.

    Re: #33665 (comment)

    All these points make sense to me, thus I’m closing this PR.

  25. rkrux closed this on Nov 24, 2025


rkrux DrahtBot fanquake Raimo33 optout21 Sjors achow101

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-12-13 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me