wallet: allow re-processing nonce-less MuSig2 PSBT #35155

Summary

SetMuSig2SecNonce() in src/script/signingprovider.cpp calls try_emplace on the persistent per-wallet m_musig2_secnonces map and then Assert(inserted). When walletprocesspsbt is called twice on the same nonce-less MuSig2 PSBT (either a natural retry, or the case where a counterparty strips the victim's pubnonce and returns the PSBT) the deterministic session_id = SHA256(script_pubkey || part_pubkey || sighash) collides, try_emplace returns inserted=false, and the Assert aborts bitcoind.

Impact

• Attack surface: authenticated RPC plus a wallet containing a MuSig2 participant private key.
• Trigger: either (a) user or tooling retries walletprocesspsbt assuming idempotency, or (b) a malicious MuSig2 cosigner returns the PSBT with the victim's pubnonce stripped, prompting the victim's client to re-process.
• No nonce reuse occurs. The Assert is intended to prevent nonce reuse, but regenerating a fresh secnonce (overwriting an unused old one) is cryptographically safe; it invalidates the previously-shared pubnonce, requiring a signing round restart, which is exactly the retry scenario.
• Design conflict: the guidance at src/util/check.h:125-126 recommends CHECK_NONFATAL for conditions reachable from RPC. Assert() uses std::abort() in all builds (NDEBUG is #error'd in check.h).
• Affected versions: v31.0rc1, v31.0rc2, and master. Not present in any GA release. Introduced in commit c9519c260b (PR #33636). Release-blocking for v31.0 GA.
• Severity: DoS — node abort.

Reproduction

On master (2d5ab09f0d at the time of testing):

# In regtest with wallet support:
# 1. create two descriptor wallets (musig_p0, musig_p1)
# 2. import tr(musig(priv0/<0;1>/*,pub1/<2;3>/*)) into wallet 0
# 3. fund the musig address and confirm
# 4. walletcreatefundedpsbt -> nonce-less MuSig2 PSBT
# 5. walletprocesspsbt $PSBT        # first call: complete=false, stores secnonce
# 6. walletprocesspsbt $PSBT        # second call on original PSBT: Assert(inserted), process aborts

Fix

Replace the try_emplace + Assert(inserted) in FlatSigningProvider::SetMuSig2SecNonce with insert_or_assign. Overwriting an unused secnonce is cryptographically safe: the previously returned pubnonce is invalidated, and the session must restart from the new nonce, which is the intent of the retry. Nothing else reads the old secnonce between the two calls.

Credit

Found by Claude during an automated review conducted by Anthropic; manually validated and patched by Trail of Bits. Reference: ANT-2026-05771.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

This PR and #35154 present the issues as vulnerabilities, while they are not.

We do not consider crashes in the RPCs as vulnerabilities, so please change the PR description. Including a functional test would also help with verifying the analysis performed by your LLM.

We do not consider crashes in the RPCs as vulnerabilities

Indeed, though it would be good if (even if nothing else is fixed) this is handled with CHECK_NONFATAL.

From a usability point of view, we should probably:

1. Refuse to start a second signing session unless the first one is explicitly aborted (but how?); or
2. Emit a warning that we started a second signing session (this has tripped me up in my own testing)

cc @achow101

Emit a warning that we started a second signing session (this has tripped me up in my own testing)

We should do that. We should key by the pubnonce as well in order to distinguish multiple sessions over the same transaction.

Thanks, but LLM output is not accepted if the author can not properly explain, test or could have written the change themselves.

The bottleneck in this project has always been review and testing, not writing code. Development here is intentionally conservative and slow, and reviewer attention is the scarcest resource we have. LLMs have made this worse, anyone can now prompt them and post their output as PRs. There is an infinite amount plausible looking "improvements" for LLMs to suggest and work on.

Unless we fully trust LLMs to both write and review code, humans still have to spend time understanding the proposed changes, which incurs a non-zero cost for every opened PR.

I understand that contributing to this project can be intimidating, and using LLMs may seem tempting, but it really creates more issues for this project than it solves. The best way to help this project, is to review and test changes. You can use LLMs for this, but you shouldn't solely rely on them, or just post their output.

Please reconsider whether it's something you genuinely think the project should pursue, independent of what your LLM suggested.

I'll close this for now.

Nowhere in the PR, other than the vestigial branch name, did we claim that this is a security vulnerability. As was stated in the PR, the bug was discovered by Anthropic, but was subsequently manually patched and validated by engineers at Trail of Bits. Would you rather us create a GitHub issue to track the bug?

Would you rather us create a GitHub issue to track the bug?

Yes

Nowhere in the PR, other than the vestigial branch name, did we claim that this is a security vulnerability

While the description does not literally mention the word "vulnerability", I think the language ("victim", "attack surface", "severity: DoS") implies a vulnerability being exploited.

I understand that you and the companies involved are trying to help out and this does look like a genuine bug. Thank you for that!

It is a little weird to me that Anthropic is routing issues through a for-profit company instead of reporting them to us directly, but I guess that is better than not hearing about these issues at all. We have been trying to work with Anthropic directly (e.g. access to Mythos) but have unfortunately not gotten anywhere.

I think this would have a good shot at being merged with my comments in #35155 (comment) addressed but an issue is also fine with me. Either way, thank you for your time.

Would you rather us create a GitHub issue to track the bug?

Yes

Done in https://github.com/bitcoin/bitcoin/issues/35250

I agree that it's better to open an issue, perhaps link to a branch with the suggested fix. We need to track the bug anyway.

Our signing code is not particularly elegant imo, especially when it comes to error handling. Sometimes an Assert means: I'd rather crash than continue (and potentially lose coins).

Finding the right fix might involve a wider refactor.