From 64897008ab6643c1cb698fcc6b9abb771f9e4540 Mon Sep 17 00:00:00 2001 From: Salvatore Ingala <6681844+bigspider@users.noreply.github.com> Date: Mon, 9 Dec 2024 13:58:57 +0100 Subject: [PATCH] Nits from PR review --- doc/bitcoin.md | 2 +- doc/musig.md | 6 +++--- src/common/wallet.c | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/doc/bitcoin.md b/doc/bitcoin.md index 77d5531fa..099ef316b 100644 --- a/doc/bitcoin.md +++ b/doc/bitcoin.md @@ -233,7 +233,7 @@ For a registered wallet, the hmac must be correct. For a default wallet, `hmac` must be equal to 32 bytes `0`. -The data yielded has with a ``, represented as a bitcoin-style varint; the rest of the message depends on the value of ``, as specified in the following subsections. +The data yielded begins with ``, represented as a bitcoin-style varint; the rest of the message depends on the value of ``, as specified in the following subsections. ##### If `` is at most 65535 diff --git a/doc/musig.md b/doc/musig.md index f3356aa89..f2dbdbdaf 100644 --- a/doc/musig.md +++ b/doc/musig.md @@ -73,7 +73,7 @@ Storing the session in persistent memory only at the end of Phase 1, and deletin #### Security of synthetic randomness -Generating $rand_{i, j}$ synthetically is not a problem, since the $rand\_root$ value is kept secret and never leaves the device. This ensures that all the values produced for different $i$ and $j$ not predictable for an attacker. +Generating $rand_{i, j}$ synthetically is not a problem, since the $rand\_root$ value is kept secret and never leaves the device. This ensures that all the values produced for different $i$ and $j$ are not predictable for an attacker. #### Malleability of the PSBT If the optional parameters are passed to the _NonceGen_ function, they will depend on the transaction data present in the PSBT. Therefore, there is no guarantee that they will be unchanged the next time the PSBT is provided. @@ -82,6 +82,6 @@ However, that does not constitute a security risk, as those parameters are only ### Generalization to multiple PSBT signing sessions -The approach described above assumes that no attempt to sign a PSBT containing for a wallet policy containing `musig()` keys is initiated while a session is already in progress. +The approach described above assumes that no attempt to sign a PSBT for a wallet policy containing `musig()` keys is initiated while a session is already in progress. -It is possible to generalize this to an arbitrary number of parallel signing sessions. Each session could be identified by a `psbt_session_id` computed by hashing together the transaction hashes, \ No newline at end of file +In order to generalize this to an arbitrary number of parallel signing sessions, one can identify each signing session with a `psbt_session_id`. Such `psbt_session_id` should deterministically depend on the transaction being signed (ignoring all the other PSBT fields), and the wallet policy being signed. In praticular, the computed `psbt_session_id` should be identical between Round 1 and Round 2 of the protocol. Note that malicious collisions of the `psbt_session_id` (for example by tampering with some details of the PSBT, like the SIGHASH flags) _are_ possible, but they do not constitute a security risk. diff --git a/src/common/wallet.c b/src/common/wallet.c index 8954af963..1f9434dd9 100644 --- a/src/common/wallet.c +++ b/src/common/wallet.c @@ -511,9 +511,9 @@ static int parse_keyexpr(buffer_t *in_buf, if (n_musig_keys < 2) { return WITH_ERROR(-1, "musig must have at least 2 key indexes"); } - if (n_musig_keys > MAX_PUBKEYS_PER_MUSIG) { - return WITH_ERROR(-1, "Too many keys in musig"); - } + + // sanity check; the loop above should never exit with too many keys + LEDGER_ASSERT(n_musig_keys <= MAX_PUBKEYS_PER_MUSIG, "Too many keys in musig"); // allocate musig structures