Kind 21101, 21106, 21104/21105 relay signing#63
Conversation
|
Warning Rate limit exceeded@kwsantiago has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughAdds FROST GroupCreate and NoncePrecommit commands, a persistent nonce store, hardware share-info API, and converts hardware signing to a relay-backed multi-step flow with new event kinds (21101, 21104–21106) and propagated threshold/participant parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant Handler as Nonce Precommit Handler
participant Device as Hardware Device
participant Relay as Relay
User->>Handler: cmd_frost_network_nonce_precommit(group, relay, device, count)
Handler->>Device: Request nonce generation (count)
Device-->>Handler: Return nonce commitments
Handler->>Relay: Publish Kind 21106 (commitments + session metadata)
Relay-->>Handler: Confirmation / Event ID
Handler-->>User: Confirmation + Event ID
sequenceDiagram
participant User as CLI User
participant Handler as Hardware Sign Handler
participant Device as Hardware Device
participant Relay as Relay
participant Peers as Peer Participants
User->>Handler: cmd_frost_network_sign_hardware(group, relay, device, ...)
Handler->>Device: Create local commitment / prepare signing
Handler->>Relay: Publish Kind 21104 (sign request, session id)
Handler->>Relay: Publish Kind 21105 (local commitment)
Relay-->>Peers: Broadcast request/commitments
Peers-->>Relay: Peer commitments/shares
Relay-->>Handler: Peer events received
Handler->>Device: Combine shares / finalize signature
Handler->>Relay: Publish Kind 21105 (final response)
Relay-->>Handler: Confirmation / Event IDs
Handler-->>User: Relay, Session ID, Request/Response Event IDs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @keep-cli/src/main.rs:
- Around line 2120-2121: The warning about nonce reuse is correct but no
tracking exists: add a NonceStore (e.g., struct NonceStore) that persists
pre-committed nonces and their state, implement methods is_nonce_used(&self,
nonce: &Nonce) -> bool and mark_nonce_used(&mut self, nonce: &Nonce) ->
Result<(), Error>, and call these from the signing path (where signing requests
consume a pre-committed nonce—update the function handling sign requests, e.g.,
sign_request/sign_message) to check and prevent reuse; also update the code that
prints the pre-committed list (out.info, out.warn) to reference the NonceStore
so the CLI can show which nonces are still available and which are spent, and
ensure persistence/backing storage (file or DB) so state survives restarts per
Issue #57.
- Around line 1322-1326: The loop uses a hardcoded threshold (threshold = 2u16)
which breaks groups with different signing thresholds; replace this by
retrieving the actual threshold from the group metadata or hardware signer (or
accept it as a CLI/func parameter) and use that value when comparing
peer_commitments.len(); locate and update the variable named threshold and the
while loop referencing peer_commitments so it uses the fetched/passed threshold
(e.g., group.threshold or a provided threshold_arg) instead of the fixed 2u16,
and ensure any error handling/log message reflects if the group metadata/CLI arg
is missing.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-cli/src/main.rs (1)
1-4368: Fix formatting issues flagged by CI pipeline.The CI pipeline failed with
cargo fmt -- --check. Runcargo fmtto fix formatting issues before merging.#!/bin/bash # Check which files need formatting cd keep-cli && cargo fmt -- --check
🧹 Nitpick comments (3)
keep-cli/src/main.rs (3)
1256-1270: Ephemeral keys generated per session may complicate peer verification.Each signing session generates new ephemeral
Keys, meaning the coordinator identity changes every time. While this provides unlinkability, it may make it harder for peers to verify legitimate signing requests versus spam/attacks.Consider whether a persistent coordinator identity (perhaps derived from the hardware signer) would improve the security model for relay-coordinated signing.
1336-1354: Silent handling of malformed peer commitments.When parsing peer commitments, a
participant_indexof 0 (from failed parsing) is silently skipped along with empty commitments. Consider logging warnings for malformed messages to aid debugging.🔎 Suggested improvement
if peer_idx > 0 && !peer_commitment.is_empty() && !peer_commitments.contains_key(&peer_idx) { peer_commitments.insert(peer_idx, peer_commitment.to_string()); out.success(&format!("Received commitment from participant {}", peer_idx)); + } else if peer_idx == 0 || peer_commitment.is_empty() { + // Log for debugging malformed messages + tracing::debug!("Skipping malformed commitment: idx={}, empty={}", peer_idx, peer_commitment.is_empty()); }
1959-1969: Ephemeral coordinator keys for group announcements.Similar to signing, this generates ephemeral keys. For group announcements (Kind 21101), consider whether the coordinator identity should be stable/verifiable so participants can trust the announcement source.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keep-cli/src/main.rs
🧰 Additional context used
🪛 GitHub Actions: CI
keep-cli/src/main.rs
[error] 1-1: cargo fmt -- --check failed. Rust formatting changes are needed in this file (keep-cli/src/main.rs). Run 'cargo fmt' or commit pre-commit formatting to fix.
🔇 Additional comments (4)
keep-cli/src/main.rs (4)
258-279: Command definitions look reasonable.The new
GroupCreateandNoncePrecommitsubcommands are well-structured with appropriate argument types and defaults.
649-661: Dispatch logic correctly routes to new handlers.The match arms properly extract parameters and delegate to the new handler functions.
1940-1946: Good validation of participant count.The check ensuring the number of provided npubs matches the declared participant count prevents misconfiguration.
2066-2078: The concern about message binding in pre-generated nonces is based on a misunderstanding of the FROST protocol.RFC 9591 (FROST) explicitly supports pre-generating nonces and their commitments before any signing message is chosen. The nonces themselves are random and don't need to be message-specific at generation time. Instead, the per-message binding occurs in Round 2 through binding factors computed from the message, commitment list, and group public key—not at commit time.
The code correctly follows this pattern:
frost_commit()generates fresh random nonces on the hardware device and returns only their public commitments- These commitments are stored and can be published
- Later,
frost_sign()uses those same commitments with the actual message and session ID- The hardware signer computes binding factors in Round 2 to bind the nonces to the specific message being signed
The dummy session and message values in
frost_commit()are just RPC parameters to trigger nonce generation on the hardware device—they don't affect which nonces are actually generated (those are random). The actual security binding happens whenfrost_sign()is called with the real message.The warning "Each nonce commitment can only be used once" is correct and already documented in the code.
kwsantiago
force-pushed
the
feat/frost-nostr-protocol
branch
from
ea4bdbb to
952798f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @keep-cli/src/main.rs:
- Around line 2069-2108: The group_id in cmd_frost_network_group_create is
currently derived only from name and participant_npubs which allows collisions
for groups that differ by threshold or participants; include the threshold and
participants values in the Sha256 input (e.g., update the hasher with threshold
and participants as bytes or a canonical string) before finalizing the hash so
different parameter combinations produce distinct group_id values (also consider
including relays if you want relay-specific uniqueness).
- Around line 1422-1470: The loop accepts peer commitments without checking the
response's request_id; update the event handling inside the for ev in events
loop to parse the "request_id" (e.g., content.get("request_id").and_then(|v|
v.as_str())) and ignore the event unless it equals our session_id variable, then
proceed to validate participant_index and commitment as before; ensure you check
request_id before inserting into peer_commitments or emitting the success
message so commitments from other sessions are rejected.
- Around line 1529-1588: The loop collecting signature shares accepts any
"signed" events without verifying they belong to our signing session; add a
check in the events processing (inside the for ev in events.iter() loop, before
accepting a share and inserting into peer_shares) that reads
content.get("request_id") (or the exact field name used for session id) and
compares it to our session_id (the variable holding our session/request
identifier); if it does not match, skip the event, otherwise continue the
existing participant_index/partial_signature validation and insertion into
peer_shares.
🧹 Nitpick comments (2)
keep-cli/src/main.rs (2)
1429-1433: Consider subscription-based event monitoring.The current implementation uses
fetch_eventspolling (lines 1429-1433 and 1542-1545) which could miss events if:
- The relay connection is temporarily disrupted
- Events arrive between polling intervals
- The relay applies rate limiting
For a more robust implementation, consider using
client.subscribe()with a channel receiver to get real-time event notifications. This would eliminate polling delays and improve reliability.
1346-1360: Note: Signing coordination is unencrypted.Unlike the DKG flow which encrypts Round 2 shares with NIP-44 (see lines 1939-1941), the signing coordination publishes Kind 21104 and 21105 events in plaintext. This means:
- The message being signed is visible to relay operators and observers
- Participant indices and commitments are public
- Signature shares are public before aggregation
This is acceptable if the signing group intends for the coordination to be public, but worth documenting clearly for users who may expect privacy.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keep-cli/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
keep-cli/src/main.rs (2)
keep-frost-net/src/event.rs (1)
commitment(79-99)keep-core/src/keys.rs (2)
generate(23-32)clone(95-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
keep-cli/src/main.rs (8)
270-301: LGTM on CLI command definitions.The new
GroupCreateandNoncePrecommitcommands are well-structured with appropriate help text and defaults. The use ofVec<String>forrelayandparticipant_npubproperly supports multiple values.
1126-1151: Good validation for hardware signing parameters.The logic correctly enforces that
--thresholdand--participantsare required for hardware signing and validates that2 <= threshold <= participants. This prevents common configuration errors.
1256-1318: Hardware signing setup and validation looks correct.The function now properly accepts
thresholdandparticipantsas parameters, and validates the hardware-returned share index (lines 1308-1313). This addresses the previous concern about hardcoded thresholds.
1339-1368: Sign request event structure looks correct.The Kind 21104 event includes appropriate tags (
d,request_id,message_type) and uses the session_id as the request identifier.
1370-1418: Commitment response event published correctly.The Kind 21105 event properly references the request event and includes the participant's commitment with correct tags.
2128-2176: Group announcement event structure looks correct.The Kind 21101 event includes all necessary tags (group_id, threshold, participants, per-participant p tags, and relay tags) and properly formats the content as JSON with name and description.
644-694: Dispatch logic correctly routes new commands.The handler properly forwards
thresholdandparticipantsparameters tocmd_frost_network_signand correctly dispatches the newGroupCreateandNoncePrecommitcommands to their respective handlers.
243-254: CLI arguments properly defined as optional.The new
--thresholdand--participantsarguments are correctly defined asOption<u16>and include helpful documentation. The runtime validation in the handler (lines 1130-1141) appropriately enforces these as required for hardware signing while keeping them optional for software-based signing.
kwsantiago
force-pushed
the
feat/frost-nostr-protocol
branch
2 times, most recently
from
68350b8 to
08f75b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @keep-cli/src/main.rs:
- Around line 1301-1304: The code is misformatted around NonceStore::open /
nonce_stats / out.info in main.rs; run `cargo fmt` to reformat the file, then
review and commit the resulting changes (ensure the lines involving
NonceStore::open, nonce_store.nonce_stats(group_npub) and out.info(...) are
properly formatted per rustfmt). After formatting, re-run CI to confirm the
issue is resolved.
- Around line 1575-1611: The signature-share loop handling events does not
verify the response's request_id against our session_id; before accepting a
share in the for ev in events.iter() block (where you parse ev.content into
serde_json::Value, extract status, participant_index, and partial_signature and
insert into peer_shares via peer_shares.entry(peer_idx)), add the same guard as
in the commitment phase: extract content.get("request_id").and_then(|v|
v.as_str()).unwrap_or("") and compare it to hex::encode(session_id); if it does
not match, continue and skip the event. Ensure this check runs early
(immediately after parsing content and before
status/participant_index/partial_signature validation) so only shares for the
current session are accepted.
- Around line 2134-2140: The group_id is currently derived from Sha256::new()
updated with the literal "frost-group-id-v1", name, and each participant_npubs,
which allows collisions when threshold or participants differ; update the
hashing in the group_id computation (the hasher used where group_id: [u8; 32] is
produced) to also incorporate the threshold value and the participants list (or
canonicalized participants metadata) into the hasher updates (e.g., call
hasher.update for threshold and any participants metadata) so the final group_id
uniquely reflects name, participant_npubs, threshold, and participants.
In @keep-cli/src/signer/nonce_store.rs:
- Around line 22-25: NonceStore reads/writes the JSON file without locking,
risking race conditions and nonce reuse; update NonceStore to acquire an
exclusive file lock around read/write operations (e.g., in open() and save()) by
opening the file with OpenOptions and using a cross-platform locker
(fs2::FileExt or fd-lock) before reading/parsing or writing/serializing, hold
the lock for the duration of the operation (and unlock/close afterwards), and
consider storing the file handle on the NonceStore struct while the lock is held
to ensure uniqueness and proper release.
- Around line 27-28: Formatting in NonceStore (e.g., the impl block and function
like pub fn open in nonce_store.rs) does not conform to rustfmt; run `cargo fmt`
in the repository root to reformat the file (and any others flagged at the CI
lines 28, 44, 75, 86), then commit the formatted changes so the impl NonceStore,
pub fn open, and surrounding code match the project's rustfmt rules; if you have
a rustfmt.toml, ensure it is applied before formatting.
🧹 Nitpick comments (1)
keep-cli/src/signer/hardware.rs (1)
159-189: Minor inconsistency in error messages.The error messages for
index,threshold, andparticipantsdon't include the actual out-of-range value, unlike the existingget_share_pubkeymethod (line 155) which includes the value for better debugging.🔎 Suggested improvement for consistency
let index = result["index"] .as_u64() .ok_or_else(|| anyhow!("Missing index in response"))? .try_into() - .map_err(|_| anyhow!("index out of range"))?; + .map_err(|e| anyhow!("index out of range: {}", e))?; let threshold = result["threshold"] .as_u64() .ok_or_else(|| anyhow!("Missing threshold in response"))? .try_into() - .map_err(|_| anyhow!("threshold out of range"))?; + .map_err(|e| anyhow!("threshold out of range: {}", e))?; let participants = result["participants"] .as_u64() .ok_or_else(|| anyhow!("Missing participants in response"))? .try_into() - .map_err(|_| anyhow!("participants out of range"))?; + .map_err(|e| anyhow!("participants out of range: {}", e))?;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
keep-cli/src/main.rskeep-cli/src/signer/hardware.rskeep-cli/src/signer/mod.rskeep-cli/src/signer/nonce_store.rs
🧰 Additional context used
🧬 Code graph analysis (1)
keep-cli/src/signer/hardware.rs (1)
keep-frost-net/src/session.rs (2)
threshold(152-154)participants(148-150)
🪛 GitHub Actions: CI
keep-cli/src/main.rs
[error] 1301-1301: cargo fmt --check: formatting differences detected. Please run 'cargo fmt' to fix code style issues.
[error] 2342-2342: cargo fmt --check: formatting differences detected. Please run 'cargo fmt' to fix code style issues.
keep-cli/src/signer/nonce_store.rs
[error] 28-28: cargo fmt --check: formatting differences detected. Please run 'cargo fmt' to fix code style issues.
[error] 44-44: cargo fmt --check: formatting differences detected. Please run 'cargo fmt' to fix code style issues.
[error] 75-75: cargo fmt --check: formatting differences detected. Please run 'cargo fmt' to fix code style issues.
[error] 86-86: cargo fmt --check: formatting differences detected. Please run 'cargo fmt' to fix code style issues.
🔇 Additional comments (11)
keep-cli/src/signer/mod.rs (1)
7-14: LGTM!The module declaration and re-export follow the established pattern used for other modules in this file. Clean integration of the new nonce store functionality.
keep-cli/src/signer/hardware.rs (1)
364-371: LGTM!The
ShareInfostruct is well-designed with appropriate field types matching the hardware response format. The#[allow(dead_code)]annotation is acceptable for a newly introduced API.keep-cli/src/signer/nonce_store.rs (2)
53-72: LGTM with note on idempotency.The duplicate check at lines 55-57 provides good idempotency - adding the same commitment twice is a no-op. The timestamp fallback to 0 on system clock issues is a reasonable defensive choice.
43-51: LGTM!The
save()method is appropriately private, ensuring it's called consistently as an internal implementation detail when state changes.keep-cli/src/main.rs (7)
243-254: LGTM!The new
thresholdandparticipantsCLI arguments for the Sign command are well-documented with appropriate help text indicating they're required for hardware signing.
270-301: LGTM!The
GroupCreateandNoncePrecommitcommand definitions are well-structured with clear help text. The default count of 10 nonces is a reasonable starting point.
1130-1159: Good resolution of threshold from hardware.This addresses the previous review concern about hardcoded thresholds. The code now:
- Falls back to querying the hardware signer for
thresholdandparticipantsif not provided via CLI- Validates that
2 <= threshold <= participantsThe validation at lines 1144-1149 correctly rejects invalid configurations.
1326-1334: Nonce tracking is well-integrated.The implementation correctly:
- Checks if nonce was previously used (line 1326) - prevents reuse
- Adds the nonce to the store immediately (lines 1332-1334)
- Marks it as used after signing completes (lines 1640-1643)
This addresses Issue #57's requirement to "Track nonce usage to prevent reuse."
Note: If signing fails after the nonce is generated but before completion, the nonce remains in the store as "unused." This is acceptable since the nonce wasn't actually consumed in a signature, and the hardware signer should maintain its own nonce state.
1336-1341: Good defensive validation of participant indices.The validation ensures the hardware-returned index is within the valid range
[1, participants]. Similar checks are applied to peer indices (lines 1477-1479, 1595-1597), preventing acceptance of malformed data.
1447-1455: LGTM!The 120-second timeout for collecting peer commitments is reasonable for multi-peer coordination scenarios. The timeout is properly checked in the loop with a clear error message on expiration.
2267-2279: Pre-committed nonces use dummy session/message - verify hardware compatibility.The nonces are generated using random
dummy_sessionanddummy_messagevalues (lines 2268-2269). However,frost_committypically binds the nonce to a specific signing context. When these pre-committed nonces are later used in actual signing:
- The hardware signer may expect the original session_id during
frost_sign- The commitment may be bound to the dummy message, making it invalid for the real message
This implementation publishes commitments to the relay but may not be compatible with how the hardware signer handles the nonce lifecycle. The signing flow in
cmd_frost_network_sign_hardwaregenerates fresh nonces (line 1319) rather than consuming pre-committed ones.Please verify:
- Does the hardware signer support decoupled nonce generation and signing?
- Is there a mechanism to use pre-committed nonces during
frost_signinstead of generating fresh ones?#!/bin/bash # Search for how pre-committed nonces are consumed during signing rg -n "precommit|pre_commit|PreCommit" --type rust rg -n "21106" --type rust -A 5 -B 5
…and relay-coordinated hardware signing
kwsantiago
force-pushed
the
feat/frost-nostr-protocol
branch
from
08f75b6 to
a19f059
Compare
1 participant
Closes #57, #55, #56
Summary by CodeRabbit
New Features
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.