Add commitment phase to dkg#210
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (5)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2026-03-03T15:27:28.357ZApplied to files:
📚 Learning: 2026-06-19T15:58:32.988ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughFresh DKG sessions now broadcast commitment hashes before revealing commitments. The coordinator records hashes, buffers or validates commitments, advances through new state-machine events and commands, and validates refresh/reshare keys. CLI retries, CI matrix checks, PRE response handling, and reporting timeouts are also updated. ChangesFresh DKG commit-reveal flow
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant DKGNode
participant Coordinator
participant Peer
participant SessionStateManager
DKGNode->>Coordinator: initiate Phase 0
Coordinator->>Peer: CommitmentHash
Peer->>Coordinator: CommitmentHash
Coordinator->>SessionStateManager: record hash
Peer->>Coordinator: Commitment
Coordinator->>SessionStateManager: validate or buffer commitment
Coordinator->>Coordinator: transition to RevealCommitment
Possibly related PRs
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 (2)
bin/orbis-node/src/dkg/v0/tests/commit_reveal.rs (1)
192-227: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a coordinator-level equivocation test.
CommitmentHashRecordOutcome::Mismatchis already covered insession_state.rs, buthandle_commitment_hash_messagestill needs a test that two different commitment hashes from the samefrom_node_idreturnDkgError::CommitmentVerificationFailed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/orbis-node/src/dkg/v0/tests/commit_reveal.rs` around lines 192 - 227, Add a coordinator-level test alongside commitment_hash_is_rejected_for_non_fresh_session that sends two different commitment hashes from the same sender through handle_message, using a fresh session and valid peer mapping. Assert the first message succeeds, the second returns DkgError::CommitmentVerificationFailed, and clean up the test database.bin/orbis-node/src/dkg/v0/coordinator/phases/phase0.rs (1)
11-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider guarding against re-entry once Phase 0 has advanced.
initiate_phase1_commitmentsguards against duplicate invocation once the session has moved past a given phase; this function has no equivalent guard, so a second call after the session already advanced (e.g. toPhase1Commitments) would regressstate.phaseback toPhase0CommitmentHashesviaupdate_phase, which would then make phase1.rs's newPhase0CommitmentHashesguard incorrectly reject a legitimate reveal. No caller in the current files can trigger this today (service.rs'ssession_existscheck prevents re-invocation), but this leaves the function exposed if called from a future path.♻️ Suggested guard
pub async fn initiate_phase0_commitment_hashes<D>( coord: &DkgCoordinator<D>, session_id: u128, peer_ids: &[String], ) -> Result<()> where D: CoordinatorDkg, { + let already_advanced = coord + .app_state + .dkg_session_state + .with_state(&session_id, |state| { + !matches!(state.phase, DkgPhase::Initializing | DkgPhase::Phase0CommitmentHashes) + }) + .await + .ok_or_else(|| session_not_found(session_id))?; + if already_advanced { + tracing::debug!(session_id, "Phase 0 start requested after hashes already complete; ignoring duplicate request"); + return Ok(()); + } + let (commitment_hash, node_id, threshold) = coord🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/orbis-node/src/dkg/v0/coordinator/phases/phase0.rs` around lines 11 - 44, Add a phase re-entry guard to the phase-0 coordinator flow before generating or broadcasting commitments: reject sessions whose phase has already advanced beyond the permitted initial phase, matching the protection used by initiate_phase1_commitments. Ensure update_phase is only reached for valid Fresh sessions so repeated calls cannot regress state.phase from a later phase back to Phase0CommitmentHashes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/orbis-node/src/dkg/v0/coordinator/message_handlers/commitment_hash.rs`:
- Around line 67-86: Change the pending commitment replay in the commitment-hash
handler to swallow and log errors instead of propagating them with .await?,
matching the sibling replay pattern in commitment.rs. Preserve the surrounding
flow so drive_event(DkgEvent::CommitmentHashRecorded, ...) always executes after
the hash is recorded, even when handle_commitment_message fails.
In `@bin/orbis-node/src/dkg/v0/service.rs`:
- Around line 296-301: Update the comment above the self_included guard in the
DKG initiation flow to refer to Phase 0 instead of Phase 1, matching
initiate_phase0_commitment_hashes and the existing log message.
---
Nitpick comments:
In `@bin/orbis-node/src/dkg/v0/coordinator/phases/phase0.rs`:
- Around line 11-44: Add a phase re-entry guard to the phase-0 coordinator flow
before generating or broadcasting commitments: reject sessions whose phase has
already advanced beyond the permitted initial phase, matching the protection
used by initiate_phase1_commitments. Ensure update_phase is only reached for
valid Fresh sessions so repeated calls cannot regress state.phase from a later
phase back to Phase0CommitmentHashes.
In `@bin/orbis-node/src/dkg/v0/tests/commit_reveal.rs`:
- Around line 192-227: Add a coordinator-level test alongside
commitment_hash_is_rejected_for_non_fresh_session that sends two different
commitment hashes from the same sender through handle_message, using a fresh
session and valid peer mapping. Assert the first message succeeds, the second
returns DkgError::CommitmentVerificationFailed, and clean up the test database.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: adef1668-ba7e-4806-adcb-740835c1eef3
📒 Files selected for processing (24)
bin/orbis-node/src/dkg/v0/coordinator/inbound.rsbin/orbis-node/src/dkg/v0/coordinator/message_handlers/commitment.rsbin/orbis-node/src/dkg/v0/coordinator/message_handlers/commitment_hash.rsbin/orbis-node/src/dkg/v0/coordinator/message_handlers/mod.rsbin/orbis-node/src/dkg/v0/coordinator/message_handlers/session_init.rsbin/orbis-node/src/dkg/v0/coordinator/mod.rsbin/orbis-node/src/dkg/v0/coordinator/network.rsbin/orbis-node/src/dkg/v0/coordinator/phases/mod.rsbin/orbis-node/src/dkg/v0/coordinator/phases/phase0.rsbin/orbis-node/src/dkg/v0/coordinator/phases/phase1.rsbin/orbis-node/src/dkg/v0/coordinator/phases/phase4.rsbin/orbis-node/src/dkg/v0/coordinator/state_machine.rsbin/orbis-node/src/dkg/v0/helpers.rsbin/orbis-node/src/dkg/v0/messages.rsbin/orbis-node/src/dkg/v0/service.rsbin/orbis-node/src/dkg/v0/session_state.rsbin/orbis-node/src/dkg/v0/tests/commit_reveal.rsbin/orbis-node/src/dkg/v0/tests/mod.rsbin/orbis-node/src/dkg/v0/tests/refresh.rscrates/crypto/src/bls12_381/common.rscrates/crypto/src/bls12_381/tests/dkg_tests.rscrates/crypto/src/decaf377/common.rscrates/crypto/src/decaf377/tests/dkg_tests.rscrates/crypto/src/trait.rs
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-12T17:49:15.841Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 48
File: crates/crypto/src/helpers.rs:26-33
Timestamp: 2026-02-12T17:49:15.841Z
Learning: In the decaf377 module usage, calling Fr::rand() without importing UniformRand is valid: the method is available directly on the type. During reviews of Rust code in crates/crypto/src, prefer using Fr::rand() without a UniformRand import when possible, and only import UniformRand if a method requires the trait explicitly. This helps distinguish library-specific extension behavior (e.g., decaf377) from other crates (e.g., ark_bls12_381) where trait imports may be necessary.
Applied to files:
crates/crypto/src/trait.rscrates/crypto/src/decaf377/tests/dkg_tests.rscrates/crypto/src/bls12_381/tests/dkg_tests.rscrates/crypto/src/decaf377/common.rscrates/crypto/src/bls12_381/common.rs
🔇 Additional comments (27)
bin/orbis-node/src/dkg/v0/coordinator/phases/phase4.rs (3)
188-205: Validation approach and documentation are solid.The end-to-end guard catching residual public-key drift before staging is a valuable addition. The comment at lines 188-191 clearly explains why the check is needed (individual commitment checks can't catch all drift). Re-serializing
staged_pkintoring_pk_bytesat line 200 ensures the stored bytes match the validated key.The concerns above (format consistency and orphaned bundle) should be verified, but the design intent is correct.
188-200: 🩺 Stability & AvailabilityNo issue: the ring key encoding and refresh staging already line up.
public_key_matches_storage_keycomparesG1Affine::to_string()against the stored ring key, andbuild_refresh_ring_bundleonly constructs an in-memory bundle; the refresh candidate is staged only after this guard passes.> Likely an incorrect or invalid review comment.
121-130: 🩺 Stability & AvailabilityNo issue: the Reshare guard can’t be bypassed here.
SessionKind::ring_key()returnsSomeforReshare, andbuild_refresh_ring_bundleonly stages an in-memory bundle before the health-check path.> Likely an incorrect or invalid review comment.bin/orbis-node/src/dkg/v0/tests/commit_reveal.rs (1)
18-78: LGTM!Also applies to: 80-95, 97-126, 128-166, 168-190, 192-227
bin/orbis-node/src/dkg/v0/tests/mod.rs (1)
3-3: LGTM!bin/orbis-node/src/dkg/v0/tests/refresh.rs (1)
429-432: LGTM!Also applies to: 441-442, 459-463, 473-477
bin/orbis-node/src/dkg/v0/messages.rs (1)
145-145: LGTM!crates/crypto/src/bls12_381/common.rs (1)
248-251: LGTM!crates/crypto/src/decaf377/common.rs (1)
159-164: LGTM!bin/orbis-node/src/dkg/v0/session_state.rs (2)
151-151: LGTM!Also applies to: 160-191, 324-325, 367-367, 1727-1776
1109-1157: 🗄️ Data Integrity & IntegrationNo action needed.
coordinator/phases/mod.rsalready mapsstate.commit_reveal.received_hashes.len()tocommitment_hashes_received,state.commit_reveal.own_hash_broadcast_completetohash_broadcast_complete, andtotal_nodestostate.node.total_nodes().> Likely an incorrect or invalid review comment.crates/crypto/src/bls12_381/tests/dkg_tests.rs (1)
6-8: LGTM!Also applies to: 42-54
crates/crypto/src/decaf377/tests/dkg_tests.rs (1)
6-8: LGTM!Also applies to: 40-52
bin/orbis-node/src/dkg/v0/helpers.rs (1)
41-41: LGTM!Also applies to: 52-55, 92-104, 903-920
crates/crypto/src/trait.rs (1)
476-482: LGTM!bin/orbis-node/src/dkg/v0/coordinator/mod.rs (1)
377-385: LGTM!bin/orbis-node/src/dkg/v0/coordinator/phases/mod.rs (1)
3-4: LGTM!Also applies to: 19-24, 77-78, 109-117, 179-183, 199-201
bin/orbis-node/src/dkg/v0/coordinator/phases/phase1.rs (1)
50-67: LGTM!bin/orbis-node/src/dkg/v0/coordinator/state_machine.rs (2)
8-8: LGTM!Also applies to: 30-30, 65-119, 146-151, 205-261
51-52: 🎯 Functional CorrectnessNo change needed
SessionSnapshotalready pullscommitment_hashes_receivedfromstate.commit_reveal.received_hashes.len()andhash_broadcast_completefromstate.commit_reveal.own_hash_broadcast_completeinbin/orbis-node/src/dkg/v0/coordinator/phases/mod.rs.> Likely an incorrect or invalid review comment.bin/orbis-node/src/dkg/v0/coordinator/message_handlers/commitment_hash.rs (1)
1-103: LGTM!bin/orbis-node/src/dkg/v0/coordinator/message_handlers/commitment.rs (2)
53-64: LGTM!Also applies to: 98-179
75-97: 🎯 Functional CorrectnessPhase1 uses the same serialized commitment bytes The reveal path sends the cached
commitment_bytesfromserialize_commitment_coefficients(...), so it matches the bytes hashed in Phase0.> Likely an incorrect or invalid review comment.bin/orbis-node/src/dkg/v0/coordinator/inbound.rs (1)
34-42: LGTM!Also applies to: 220-232, 406-417
bin/orbis-node/src/dkg/v0/coordinator/network.rs (1)
135-137: LGTM!bin/orbis-node/src/dkg/v0/coordinator/message_handlers/mod.rs (1)
7-8: LGTM!Also applies to: 22-24, 41-49
bin/orbis-node/src/dkg/v0/coordinator/message_handlers/session_init.rs (1)
642-673: 🎯 Functional CorrectnessNo double Phase 0 on the initiator
service.rsskips the local peer when sendingSessionInit, so the initiator only entersinitiate_phase0_commitment_hashesthrough the local start path once.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/rust.yml:
- Line 39: Update both actions/checkout@v4 steps in the clippy and build jobs to
set persist-credentials to false, preventing the GitHub token from being stored
in the local Git configuration.
- Around line 28-45: Add a workflow-level permissions block for the Rust CI
workflow, granting only contents: read, so the clippy job and other read-only
checks use a least-privilege GITHUB_TOKEN.
- Around line 48-49: Remove the trailing line-continuation backslash from the
runs-on value in the workflow, preserving the folded YAML value so the runner
label contains a normal space between the dynamic identifier and spot
configuration.
In `@bin/cli-tool/src/commands.rs`:
- Around line 79-80: Update the map_err error message in the
create_authenticated_request call to reference create_authenticated_request
instead of create_dkg_jwt, preserving the existing error propagation and
formatting.
- Around line 51-54: Update is_retryable_dkg_start_error to classify retryable
failures by transient gRPC status codes, including Unavailable, Unknown, and
DeadlineExceeded, rather than inspecting the message for “timed out”; return
false for other codes and remove the brittle message-text check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87a65151-6f12-4983-879f-5fe9d3222448
📒 Files selected for processing (2)
.github/workflows/rust.ymlbin/cli-tool/src/commands.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: clippy (decaf377, --no-default-features --features=decaf377,redb,integration-test,fault-injection)
- GitHub Check: build (decaf377, --no-default-features --features=decaf377,redb,integration-test,fault-injection)
- GitHub Check: lint
- GitHub Check: build (bls12-381, --features=integration-test,fault-injection)
- GitHub Check: clippy (bls12-381, --features=integration-test,fault-injection)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-04-28T20:40:06.849Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 145
File: .github/workflows/rust.yml:40-40
Timestamp: 2026-04-28T20:40:06.849Z
Learning: In sourcenetwork/orbis-rs, when CI workflows reference the SourceHub Docker image using the mutable `dev` tag (e.g., `ghcr.io/sourcenetwork/sourcehub:dev`), treat this as an intentional choice. Do not flag it as a reproducibility/release-pinning issue; the `dev` tag is deliberately used to continuously track upstream SourceHub and require manual updates when needed.
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2026-05-14T20:15:52.222Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 162
File: bin/cli-tool/src/commands.rs:93-94
Timestamp: 2026-05-14T20:15:52.222Z
Learning: In the test CLI crate under bin/cli-tool (e.g., bin/cli-tool/src/**/*.rs), reviewers should treat `.expect()`/panicking error handling as acceptable when it’s used for convenience in test tooling rather than production behavior. Do not require full error propagation for these cases; if you see `.expect(...)`, ensure the panic message is meaningful and that the code is clearly part of this non-production CLI tool (not core library/production paths).
Applied to files:
bin/cli-tool/src/commands.rs
🪛 zizmor (1.26.1)
.github/workflows/rust.yml
[warning] 39-39: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 59-59: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 28-45: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🔇 Additional comments (2)
.github/workflows/rust.yml (2)
23-26: LGTM!
36-37: 📐 Maintainability & Code Quality | ⚡ Quick winVerify decaf377 matrix intentionally omits sourcehub and iroh features.
The bls12-381 entry uses default features (which include
authz-sourcehub,bulletin-sourcehub,network/irohperbin/orbis-node/Cargo.toml) plusintegration-test,fault-injection. The decaf377 entry uses--no-default-featuresand only enablesdecaf377,redb,integration-test,fault-injection, omittingauthz-sourcehub,bulletin-sourcehub, andiroh. If those crates have decaf377-specific code paths, this gap leaves them untested. If the omission is intentional (e.g., those features don't support decaf377), please confirm.Also applies to: 56-57
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/orbis-node/src/pre/v0/coordinator/network.rs`:
- Around line 105-129: Update the response-handling method’s documentation to
state that a reencryption response may be returned even when it is not stored.
Bind the result of store_response_for_version before matching it, and add
diagnostic logging for ResponseStoreOutcome::MissingRequest while preserving its
existing Ok(Some(response)) behavior; leave Stored and UnexpectedOrDuplicatePeer
handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64922857-881d-44e1-99ce-9efc2861a018
📒 Files selected for processing (2)
.github/workflows/rust.ymlbin/orbis-node/src/pre/v0/coordinator/network.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/rust.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: build (decaf377, --no-default-features --features=decaf377,redb,integration-test,fault-injection)
- GitHub Check: clippy (decaf377, --no-default-features --features=decaf377,redb,integration-test,fault-injection)
- GitHub Check: clippy (bls12-381, --features=integration-test,fault-injection)
- GitHub Check: lint
- GitHub Check: build (bls12-381, --features=integration-test,fault-injection)
🔇 Additional comments (1)
bin/orbis-node/src/pre/v0/coordinator/network.rs (1)
3-3: LGTM!
1 participant
No description provided.