Conversation
WalkthroughIntroduces a new key-proof PSBT workflow (build, sign, verify) in keep-bitcoin and integrates signed PSBTs into keep-frost-net descriptor ACKs so participants prove control of contributed keys during the ACK phase. Changes
Sequence Diagram(s)mermaid Node->>KeepBitcoin: build_key_proof_psbt(session_id, share_index, account_xpub, network) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
keep-bitcoin/src/key_proof.rs (1)
116-123: IntermediateXprivvalues (master,child) are not zeroized on drop.The
child_bytesare properly wrapped inZeroizing, but theXprivstructs containing private key material are dropped without explicit zeroization since thebitcoincrate'sXprivdoesn't implementZeroize. This is a known limitation and was likely considered during the "harden key proof zeroization" commit.If the
bitcoincrate addsZeroizesupport forXprivin the future, it would be worth revisiting. For now, the sensitive window is minimal (scoped block).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-bitcoin/src/key_proof.rs` around lines 116 - 123, The intermediate Xpriv values (master, child) hold private material but aren't Zeroize-able; to minimize the sensitive window, create master and derive child inside a tightly-scoped inner block and immediately extract child.private_key.secret_bytes() into the Zeroizing-wrapped child_bytes, then let the inner block end so master and child are dropped right away (avoid binding master/child outside the block and avoid holding them beyond extraction). Reference the Xpriv construction/derive_priv calls and the child_bytes Zeroizing wrapper when implementing this scope reduction so the Xpriv instances don't outlive the extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-frost-net/src/node/descriptor.rs`:
- Around line 562-576: The initiator is waiting for ACKs from all online peers
(expected_acks) but non-contributor peers lack entries in contributions causing
our_xpub to be None and key proof generation to fail; fix this by restricting
expected_acks to only contributors (i.e., intersect the set used to build
expected_acks with the keys of contributions/expected_contributors) so peers
that are not in contributions aren’t expected to return DescriptorFinalize ACKs,
and/or make key proof generation in the DescriptorFinalize handler tolerate
missing our_xpub by treating non-contributors as optional (skip building
key_proof_psbt_bytes when our_xpub is None and don’t require their ACK). Ensure
you update places that reference expected_acks and the DescriptorFinalize flow
(symbols: expected_acks, contributions/expected_contributors, DescriptorFinalize
handler, our_xpub, key_proof_psbt_bytes, signing_share_bytes) so the initiator
only awaits ACKs from actual contributors or non-contributors are handled as
optional.
In `@keep-frost-net/src/protocol.rs`:
- Line 33: Add an assertion in the test_sign_and_verify_roundtrip test to ensure
the serialized key_proof PSBT length does not exceed MAX_KEY_PROOF_PSBT_SIZE:
after producing the signed PSBT (the variable holding the serialized key_proof
PSBT in keep-bitcoin/src/key_proof.rs), call assert!(psbt_bytes.len() <=
MAX_KEY_PROOF_PSBT_SIZE) (or an equivalent check) so the test fails if the
produced PSBT grows past the protocol constant MAX_KEY_PROOF_PSBT_SIZE;
reference the constant MAX_KEY_PROOF_PSBT_SIZE and the test function name
test_sign_and_verify_roundtrip when locating where to add this assertion.
---
Nitpick comments:
In `@keep-bitcoin/src/key_proof.rs`:
- Around line 116-123: The intermediate Xpriv values (master, child) hold
private material but aren't Zeroize-able; to minimize the sensitive window,
create master and derive child inside a tightly-scoped inner block and
immediately extract child.private_key.secret_bytes() into the Zeroizing-wrapped
child_bytes, then let the inner block end so master and child are dropped right
away (avoid binding master/child outside the block and avoid holding them beyond
extraction). Reference the Xpriv construction/derive_priv calls and the
child_bytes Zeroizing wrapper when implementing this scope reduction so the
Xpriv instances don't outlive the extraction.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
keep-bitcoin/src/key_proof.rskeep-bitcoin/src/lib.rskeep-frost-net/src/descriptor_session.rskeep-frost-net/src/node/descriptor.rskeep-frost-net/src/protocol.rskeep-frost-net/tests/multinode_test.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
keep-frost-net/src/node/descriptor.rs (1)
562-576:⚠️ Potential issue | 🟠 MajorNon-contributor receivers of
DescriptorFinalizewill return a spurious error.
finalize_descriptorbroadcasts to all online peers (lines 381–388), not just contributors. A non-contributor has no entry insession.contributions(), soour_xpubisNone, and line 563 turns that into anErr. The session cleanup and error propagation that follows is incorrect — this is an expected, benign situation for a non-contributor.Add an early exit for non-contributors before attempting key proof construction:
🐛 Proposed fix
+ // Non-contributors have no key to prove; they have already acknowledged + // the descriptor by reaching this point without a NACK. + let Some(our_xpub) = our_xpub else { + // Emit completion event if the descriptor verified cleanly. + let _ = self.event_tx.send(KfpNodeEvent::DescriptorComplete { + session_id: payload.session_id, + external_descriptor: payload.external_descriptor, + internal_descriptor: payload.internal_descriptor, + network: session_network, + }); + return Ok(()); + }; + let key_proof_psbt_bytes = { - let our_xpub = our_xpub.ok_or_else(|| { - FrostNetError::Session("Missing own xpub contribution for key proof".into()) - })?; - let net = crate::descriptor_session::parse_network(&session_network)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-frost-net/src/node/descriptor.rs` around lines 562 - 576, The code in finalize_descriptor attempts to build/sign a key proof for every receiver but does not handle non-contributors: if our_xpub is None (no entry in session.contributions()) the function currently returns an error when it should treat this as a benign case. Add an early exit right before constructing key_proof_psbt_bytes: check session.contributions() / our_xpub and if our_xpub is None (i.e., we're not a contributor) skip key proof construction and return/continue successfully (or otherwise avoid error propagation for non-contributors) so that the block using our_xpub, our_index, keep_bitcoin::build_key_proof_psbt and keep_bitcoin::sign_key_proof only runs for actual contributors.
🧹 Nitpick comments (2)
keep-bitcoin/src/key_proof.rs (2)
178-315: Good test coverage overall; consider adding a cross-network mismatch case.The suite covers determinism, session/share variations, roundtrip, tampered signature, and mainnet. One gap: no test for a PSBT signed on
Network::Testnetsubmitted for verification onNetwork::Bitcoin(differentcoin_type→ different child key → should fail). This is a cheap case to add given the other tamper tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-bitcoin/src/key_proof.rs` around lines 178 - 315, Add a test that ensures cross-network verification fails: use test_xpub to create an xpub for Network::Testnet, build a PSBT with build_key_proof_psbt(..., Network::Testnet), sign it with sign_key_proof(..., Network::Testnet), then call verify_key_proof(..., Network::Bitcoin) and assert it returns an Err; reference test helper test_xpub and the functions build_key_proof_psbt, sign_key_proof, and verify_key_proof to locate where to wire the new test.
244-249: Extract the PSBT size cap as a named constant.The test hard-codes
512which must stay in sync withMAX_KEY_PROOF_PSBT_SIZEinkeep-frost-net/src/protocol.rs. A silent drift would invalidate the guard. Define a public constant inkeep-bitcoin(e.g.pub const MAX_KEY_PROOF_PSBT_SIZE: usize = 512;) and import it in both places.♻️ Proposed change
+/// Maximum byte length of a serialised key-proof PSBT. +/// Must match `MAX_KEY_PROOF_PSBT_SIZE` in keep-frost-net. +pub const MAX_KEY_PROOF_PSBT_SIZE: usize = 512;Then in the test:
- assert!( - signed_bytes.len() <= 512, - "key proof PSBT {} bytes exceeds protocol max 512", - signed_bytes.len(), - ); + assert!( + signed_bytes.len() <= MAX_KEY_PROOF_PSBT_SIZE, + "key proof PSBT {} bytes exceeds protocol max {MAX_KEY_PROOF_PSBT_SIZE}", + signed_bytes.len(), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-bitcoin/src/key_proof.rs` around lines 244 - 249, Extract the hard-coded 512 into a shared named constant by adding a public constant pub const MAX_KEY_PROOF_PSBT_SIZE: usize = 512 in the keep-bitcoin crate (e.g. near key_proof.rs module), then replace the literal 512 in the assert in key_proof::tests (the assert! checking signed_bytes.len() <= 512) with that constant; update keep-frost-net/src/protocol.rs to import/use the same exported MAX_KEY_PROOF_PSBT_SIZE from keep-bitcoin so both crates reference the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@keep-frost-net/src/node/descriptor.rs`:
- Around line 562-576: The code in finalize_descriptor attempts to build/sign a
key proof for every receiver but does not handle non-contributors: if our_xpub
is None (no entry in session.contributions()) the function currently returns an
error when it should treat this as a benign case. Add an early exit right before
constructing key_proof_psbt_bytes: check session.contributions() / our_xpub and
if our_xpub is None (i.e., we're not a contributor) skip key proof construction
and return/continue successfully (or otherwise avoid error propagation for
non-contributors) so that the block using our_xpub, our_index,
keep_bitcoin::build_key_proof_psbt and keep_bitcoin::sign_key_proof only runs
for actual contributors.
---
Nitpick comments:
In `@keep-bitcoin/src/key_proof.rs`:
- Around line 178-315: Add a test that ensures cross-network verification fails:
use test_xpub to create an xpub for Network::Testnet, build a PSBT with
build_key_proof_psbt(..., Network::Testnet), sign it with sign_key_proof(...,
Network::Testnet), then call verify_key_proof(..., Network::Bitcoin) and assert
it returns an Err; reference test helper test_xpub and the functions
build_key_proof_psbt, sign_key_proof, and verify_key_proof to locate where to
wire the new test.
- Around line 244-249: Extract the hard-coded 512 into a shared named constant
by adding a public constant pub const MAX_KEY_PROOF_PSBT_SIZE: usize = 512 in
the keep-bitcoin crate (e.g. near key_proof.rs module), then replace the literal
512 in the assert in key_proof::tests (the assert! checking signed_bytes.len()
<= 512) with that constant; update keep-frost-net/src/protocol.rs to import/use
the same exported MAX_KEY_PROOF_PSBT_SIZE from keep-bitcoin so both crates
reference the single source of truth.
1 participant
Closes #270
Summary by CodeRabbit
New Features
APIs
Tests