Hardening#176
Merged
Merged
Conversation
|
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 (11)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ 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). (2)
🧰 Additional context used🧠 Learnings (10)📓 Common learnings📚 Learning: 2026-04-29T16:47:26.051ZApplied to files:
📚 Learning: 2026-04-28T14:49:17.416ZApplied to files:
📚 Learning: 2026-05-20T18:08:12.109ZApplied to files:
📚 Learning: 2026-04-30T20:31:37.077ZApplied to files:
📚 Learning: 2026-05-11T14:42:47.309ZApplied to files:
📚 Learning: 2026-02-12T17:49:15.841ZApplied to files:
📚 Learning: 2026-03-03T15:27:38.961ZApplied to files:
📚 Learning: 2026-05-13T18:55:29.338ZApplied to files:
📚 Learning: 2026-05-11T14:28:55.018ZApplied to files:
🔇 Additional comments (12)
📝 WalkthroughWalkthroughWiden session identifiers from u64 to u128 across DKG/crypto/messages/state/bench/tests; add router ingress limits, metrics, and gRPC per-connection tuning; tighten PRE local-share verification and JWT leeway; update workspace dependency declarations and docs. ChangesRuntime, networking, docs, and metrics
Session ID migration and crypto/message contracts
Sign/nonce hardening, PRE verification, JWT change, and misc fixes
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/orbis-node/src/dkg/session_state.rs (1)
739-759:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck for an existing session before enforcing
MAX_DKG_SESSIONS.When the map is full and this
session_idalready exists, this returnsLimitReachedinstead ofAlreadyExists. The contract above this method says callers may ignoreAlreadyExistsbut must clear PSS claims onLimitReached, so the current order can tear down a live claim for an already-running session.[suggested fix]
Patch
let mut states = self.states.write().await; + // A duplicate create for an existing session should stay idempotent even + // when the manager is already at capacity. + if states.contains_key(&session_id) { + tracing::debug!( + session_id = session_id, + "DKG session already exists for session_id" + ); + return CreateSessionOutcome::AlreadyExists; + } + // Enforce maximum concurrent session limit to prevent resource exhaustion if states.len() >= MAX_DKG_SESSIONS { tracing::warn!( session_id = session_id, active_sessions = states.len(), @@ ); return CreateSessionOutcome::LimitReached; } - - // Check if session already exists to avoid overwriting existing state - if states.contains_key(&session_id) { - tracing::debug!( - session_id = session_id, - "DKG session already exists for session_id" - ); - return CreateSessionOutcome::AlreadyExists; - }🤖 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/session_state.rs` around lines 739 - 759, The current code checks MAX_DKG_SESSIONS before verifying if the session_id already exists, causing an existing session to be reported as LimitReached; change the ordering in the create session path so you first check states.contains_key(&session_id) and return CreateSessionOutcome::AlreadyExists if present, and only then enforce the MAX_DKG_SESSIONS limit (using states.len() >= MAX_DKG_SESSIONS to return CreateSessionOutcome::LimitReached); update the block around states, session_id, MAX_DKG_SESSIONS, CreateSessionOutcome::AlreadyExists and CreateSessionOutcome::LimitReached accordingly.
🧹 Nitpick comments (2)
bin/orbis-node/src/sign/messages.rs (1)
33-68: ⚡ Quick winAdd one
> u64::MAXround-trip test for the signed statements.These are canonical payloads that end up under signature, but the migration here is only exercised by type changes. A remaining narrow cast would stay invisible until a large live
session_idhits reshare or refresh signing. One serialize/deserialize test using something like(u64::MAX as u128) + 1for both statements would lock down the new wire contract.🤖 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/sign/messages.rs` around lines 33 - 68, Add a unit test that round-trips a value larger than u64::MAX for the session_id to lock the wire contract: construct a RingReshareUpdateStatement (and its RingReshareUpdateContext if used) and a RefreshHealthCheckStatement with session_id set to (u64::MAX as u128) + 1, serialize them with the same canonical serializer used by the codebase (e.g., serde_json or bincode), then deserialize and assert equality of the original and deserialized structs; place the test alongside existing message tests and reference the types RingReshareUpdateStatement, RingReshareUpdateContext, and RefreshHealthCheckStatement so future narrow casts are caught.bin/orbis-node/src/dkg/PROTOCOL_FLOW.md (1)
1-22: 💤 Low valueConsider documenting the session ID data type.
The protocol flow references
session_id(line 21) but does not mention that it uses a 128-bit identifier. Since this PR migrates session IDs tou128and affects serialization size, adding a brief note in the "Overview" or as a comment near line 21 would help readers understand the protocol's data model.🤖 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/PROTOCOL_FLOW.md` around lines 1 - 22, Add a short note near the StartDkgRequest/session_id mention explaining that session_id is now a 128-bit unsigned integer (u128) and that this increases its serialized size; update the "Overview" or the comment next to `session_id` in the `start_dkg` flow to state: session_id: u128 (128-bit identifier) and note any implications for serialization/transport size so readers understand the data model change introduced by the PR.
🤖 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/helpers/launch.rs`:
- Around line 68-79: The two CLI args grpc_concurrency_limit_per_connection and
grpc_max_concurrent_streams currently accept 0; add clap value parsers with a
positive range to validate inputs and fail fast. For
grpc_concurrency_limit_per_connection use value_parser =
clap::value_parser!(usize).range(1..) and for grpc_max_concurrent_streams use
value_parser = clap::value_parser!(u32).range(1..), placing these attributes on
the respective struct fields so passing 0 will be rejected.
---
Outside diff comments:
In `@bin/orbis-node/src/dkg/session_state.rs`:
- Around line 739-759: The current code checks MAX_DKG_SESSIONS before verifying
if the session_id already exists, causing an existing session to be reported as
LimitReached; change the ordering in the create session path so you first check
states.contains_key(&session_id) and return CreateSessionOutcome::AlreadyExists
if present, and only then enforce the MAX_DKG_SESSIONS limit (using states.len()
>= MAX_DKG_SESSIONS to return CreateSessionOutcome::LimitReached); update the
block around states, session_id, MAX_DKG_SESSIONS,
CreateSessionOutcome::AlreadyExists and CreateSessionOutcome::LimitReached
accordingly.
---
Nitpick comments:
In `@bin/orbis-node/src/dkg/PROTOCOL_FLOW.md`:
- Around line 1-22: Add a short note near the StartDkgRequest/session_id mention
explaining that session_id is now a 128-bit unsigned integer (u128) and that
this increases its serialized size; update the "Overview" or the comment next to
`session_id` in the `start_dkg` flow to state: session_id: u128 (128-bit
identifier) and note any implications for serialization/transport size so
readers understand the data model change introduced by the PR.
In `@bin/orbis-node/src/sign/messages.rs`:
- Around line 33-68: Add a unit test that round-trips a value larger than
u64::MAX for the session_id to lock the wire contract: construct a
RingReshareUpdateStatement (and its RingReshareUpdateContext if used) and a
RefreshHealthCheckStatement with session_id set to (u64::MAX as u128) + 1,
serialize them with the same canonical serializer used by the codebase (e.g.,
serde_json or bincode), then deserialize and assert equality of the original and
deserialized structs; place the test alongside existing message tests and
reference the types RingReshareUpdateStatement, RingReshareUpdateContext, and
RefreshHealthCheckStatement so future narrow casts are caught.
🪄 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: 4e0ce70b-54f3-4bae-a3d5-65f1285cc3e4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
Cargo.tomlREADME.mdbin/cli-tool/Cargo.tomlbin/orbis-node/Cargo.tomlbin/orbis-node/README.mdbin/orbis-node/src/constants.rsbin/orbis-node/src/dkg/PROTOCOL_FLOW.mdbin/orbis-node/src/dkg/coordinator/message_handlers/commitment.rsbin/orbis-node/src/dkg/coordinator/message_handlers/session_init.rsbin/orbis-node/src/dkg/coordinator/message_handlers/share.rsbin/orbis-node/src/dkg/coordinator/mod.rsbin/orbis-node/src/dkg/coordinator/network.rsbin/orbis-node/src/dkg/coordinator/phases/mod.rsbin/orbis-node/src/dkg/coordinator/phases/phase1.rsbin/orbis-node/src/dkg/coordinator/phases/phase2.rsbin/orbis-node/src/dkg/coordinator/phases/phase4.rsbin/orbis-node/src/dkg/coordinator/refresh_health_check.rsbin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rsbin/orbis-node/src/dkg/coordinator/reshare/cleanup.rsbin/orbis-node/src/dkg/coordinator/reshare/selection.rsbin/orbis-node/src/dkg/coordinator/ring_storage.rsbin/orbis-node/src/dkg/helpers.rsbin/orbis-node/src/dkg/messages.rsbin/orbis-node/src/dkg/service.rsbin/orbis-node/src/dkg/session_state.rsbin/orbis-node/src/dkg/tests/dkg.rsbin/orbis-node/src/dkg/tests/refresh.rsbin/orbis-node/src/dkg/tests/reshare.rsbin/orbis-node/src/helpers/create_routers.rsbin/orbis-node/src/helpers/helpers.rsbin/orbis-node/src/helpers/launch.rsbin/orbis-node/src/main.rsbin/orbis-node/src/pre/tests.rsbin/orbis-node/src/sign/messages.rsbin/orbis-node/src/sign/tests.rsbin/orbis-node/src/store_secret/service.rscrates/authn/Cargo.tomlcrates/crypto/benches/bls12_381/dkg.rscrates/crypto/benches/bls12_381/pre.rscrates/crypto/benches/bls12_381/sign.rscrates/crypto/benches/decaf377/dkg.rscrates/crypto/benches/decaf377/pre.rscrates/crypto/benches/decaf377/sign.rscrates/crypto/benches/dkg_benchmarks.rscrates/crypto/src/bls12_381/dkg.rscrates/crypto/src/bls12_381/tests/deserialization_prop_tests.rscrates/crypto/src/bls12_381/tests/pre_tests.rscrates/crypto/src/bls12_381/tests/sign_tests.rscrates/crypto/src/decaf377/dkg.rscrates/crypto/src/decaf377/tests/deserialization_prop_tests.rscrates/crypto/src/decaf377/tests/pre_tests.rscrates/crypto/src/decaf377/tests/sign_tests.rscrates/crypto/src/dkg_tests.rscrates/crypto/src/lifecycle_tests.rscrates/crypto/src/test_helpers/test_helper.rscrates/crypto/src/trait.rscrates/network/README.mdcrates/network/src/iroh/router.rscrates/network/src/iroh/tests.rscrates/network/src/lib.rscrates/network/src/metrics.rscrates/network/src/trait.rscrates/proto/Cargo.tomldocker/docker-compose-integration-test.ymldocker/docker-compose-metrics-network-test.ymldocker/docker-compose.3-node.ymldocker/grafana/dashboards/orbis.json
📜 Review details
⏰ 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 context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 152
File: crates/bulletin/src/sourcehub/mod.rs:12-14
Timestamp: 2026-05-11T14:28:55.018Z
Learning: In `crates/bulletin/src/sourcehub/mod.rs` (Rust), the `DEFAULT_THRESHOLD_SIGNATURE_SCHEME` constant references `crypto::THRESHOLD_SIGNATURE_SCHEME`. All builds of orbis-rs are expected to enable at least one crypto feature (bls12-381, decaf377, or a future equivalent), so this constant should NOT be feature-gated at the bulletin crate level. The feature enforcement is handled by the `crypto` crate itself. Avoid flagging missing cfg gates on this constant.
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 147
File: bin/orbis-node/src/store_secret/service.rs:0-0
Timestamp: 2026-04-29T16:47:26.051Z
Learning: In `bin/orbis-node/src/store_secret/service.rs`, the pre-store `verify_encryption` check (which previously passed ring key, effective key, and policy metadata into `validate_encrypted_document`) was intentionally removed as part of PR `#147` (closes issue `#139`). The rationale: running this verification before storing requires exposing ring key and policy metadata to the validation function, which leaks privacy. Since a caller could always store cryptographically junk data regardless of this check, removing it does not weaken the protocol's security invariants — it only improves privacy. Proof field validation (shared_point/challenge/response) is deferred to downstream consumers.
📚 Learning: 2026-05-13T18:55:29.338Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 161
File: bin/orbis-node/src/dkg/coordinator/refresh_health_check.rs:147-163
Timestamp: 2026-05-13T18:55:29.338Z
Learning: In `bin/orbis-node/src/dkg/coordinator/refresh_health_check.rs`, the partial-broadcast / local-rollback split-brain scenario in `run_selector` was explicitly considered and accepted as a pragmatic trade-off. The refresh health check is a diagnostic layer on top of the actual key ceremony; the underlying key material is persisted independently. Adding 2PC or compensating rollback broadcasts would just shift the same class of partial-failure risk to the new coordination round, creating an infinite nesting problem. The design intentionally accepts potential transient inconsistency in the health-check staging state.
Applied to files:
crates/crypto/src/bls12_381/tests/pre_tests.rsbin/orbis-node/src/dkg/coordinator/phases/phase1.rsbin/orbis-node/src/dkg/coordinator/network.rsbin/orbis-node/src/dkg/coordinator/reshare/selection.rsbin/orbis-node/src/dkg/tests/refresh.rsbin/orbis-node/src/dkg/coordinator/refresh_health_check.rs
📚 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/bls12_381/tests/pre_tests.rscrates/crypto/src/dkg_tests.rscrates/crypto/src/decaf377/tests/pre_tests.rscrates/crypto/src/bls12_381/tests/sign_tests.rscrates/crypto/src/lifecycle_tests.rscrates/crypto/src/decaf377/tests/sign_tests.rscrates/crypto/src/decaf377/dkg.rscrates/crypto/src/decaf377/tests/deserialization_prop_tests.rscrates/crypto/src/bls12_381/tests/deserialization_prop_tests.rscrates/crypto/src/bls12_381/dkg.rscrates/crypto/src/trait.rscrates/crypto/src/test_helpers/test_helper.rs
📚 Learning: 2026-03-03T15:27:38.961Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 87
File: bin/orbis-node/src/tests/mod.rs:3-3
Timestamp: 2026-03-03T15:27:38.961Z
Learning: In the orbis-rs repository, integration tests (including Docker-based tests in bin/orbis-node/src/tests/integration.rs) should run by default during `cargo test` without requiring a feature flag.
Applied to files:
docker/docker-compose-integration-test.ymldocker/docker-compose-metrics-network-test.ymlbin/orbis-node/README.mdbin/orbis-node/Cargo.toml
📚 Learning: 2026-04-28T20:40:09.735Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 145
File: .github/workflows/rust.yml:40-40
Timestamp: 2026-04-28T20:40:09.735Z
Learning: In the sourcenetwork/orbis-rs repository, the SourceHub Docker image is intentionally pulled using the mutable `dev` tag (e.g., `ghcr.io/sourcenetwork/sourcehub:dev`) in CI workflows and docker-compose files. The maintainer (JesseAbram) prefers this over pinning to a specific SHA or release tag because it forces active tracking and manual updates of the upstream SourceHub image. Do not flag this as a reproducibility issue.
Applied to files:
docker/docker-compose-integration-test.ymldocker/docker-compose-metrics-network-test.yml
📚 Learning: 2026-04-30T20:31:37.077Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 148
File: bin/orbis-node/src/dkg/coordinator/phases.rs:689-695
Timestamp: 2026-04-30T20:31:37.077Z
Learning: In `bin/orbis-node/src/dkg/coordinator/phases.rs`, `wait_for_reshare_bulletin_finalized` intentionally gates PSS-claim release only on `next_peer_ids` and `new_threshold` being `None`. This is by design: an upcoming PR (related to PR `#148`) will make the threshold signature the trigger that clears those fields on the bulletin, so this poll condition will be sufficient once that change lands. Do not flag the absence of explicit signature-metadata verification in this function as a bug.
Applied to files:
bin/orbis-node/src/dkg/coordinator/phases/phase2.rsbin/orbis-node/src/dkg/coordinator/phases/phase1.rsbin/orbis-node/src/sign/tests.rsbin/orbis-node/src/dkg/coordinator/phases/phase4.rscrates/crypto/benches/bls12_381/sign.rsbin/orbis-node/src/dkg/coordinator/reshare/cleanup.rscrates/crypto/src/bls12_381/tests/sign_tests.rsbin/orbis-node/src/dkg/tests/reshare.rsbin/orbis-node/src/sign/messages.rsbin/orbis-node/src/pre/tests.rsbin/orbis-node/src/dkg/PROTOCOL_FLOW.mdbin/orbis-node/src/dkg/coordinator/phases/mod.rsbin/orbis-node/src/dkg/coordinator/message_handlers/share.rsbin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rsbin/orbis-node/src/dkg/helpers.rsbin/orbis-node/src/dkg/coordinator/reshare/selection.rsbin/orbis-node/src/dkg/coordinator/mod.rsbin/orbis-node/src/dkg/coordinator/refresh_health_check.rsbin/orbis-node/src/dkg/tests/dkg.rsbin/orbis-node/src/dkg/session_state.rs
📚 Learning: 2026-05-11T14:42:47.309Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 152
File: bin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rs:274-282
Timestamp: 2026-05-11T14:42:47.309Z
Learning: In `bin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rs`, `block_number_nonce` in `RingPayload` is intentionally passed through unchanged in the signed `RingReshareUpdateStatement`. SourceHub (see sourcenetwork/sourcehub#110) rotates `block_number_nonce` atomically server-side when applying the threshold-signature bulletin update (together with `new_peer_ids`/`new_threshold` → `peer_ids`/`threshold`). A replay of an old signature is benign by design: if the prior update failed, the nodes were already in the ready state awaiting the chain update, so re-submitting the same signature has no harmful effect.
Applied to files:
bin/orbis-node/src/dkg/coordinator/phases/phase2.rsbin/orbis-node/src/dkg/coordinator/ring_storage.rsbin/orbis-node/src/dkg/coordinator/message_handlers/commitment.rsbin/orbis-node/src/dkg/coordinator/message_handlers/session_init.rsbin/orbis-node/src/store_secret/service.rsbin/orbis-node/src/dkg/coordinator/phases/phase1.rsbin/orbis-node/src/sign/tests.rsbin/orbis-node/README.mdcrates/crypto/src/decaf377/tests/pre_tests.rscrates/crypto/benches/decaf377/pre.rsbin/orbis-node/src/dkg/coordinator/phases/phase4.rsbin/orbis-node/src/dkg/service.rscrates/crypto/benches/bls12_381/sign.rsbin/orbis-node/src/dkg/coordinator/reshare/cleanup.rscrates/crypto/src/bls12_381/tests/sign_tests.rscrates/crypto/benches/decaf377/sign.rscrates/crypto/benches/bls12_381/dkg.rsbin/orbis-node/src/dkg/tests/reshare.rscrates/crypto/src/lifecycle_tests.rsbin/orbis-node/src/sign/messages.rscrates/crypto/benches/bls12_381/pre.rscrates/crypto/src/decaf377/tests/sign_tests.rscrates/crypto/src/decaf377/dkg.rscrates/crypto/benches/decaf377/dkg.rscrates/crypto/benches/dkg_benchmarks.rsbin/orbis-node/src/pre/tests.rsbin/orbis-node/src/dkg/PROTOCOL_FLOW.mdcrates/crypto/src/bls12_381/dkg.rsbin/orbis-node/src/dkg/coordinator/network.rsbin/orbis-node/src/dkg/coordinator/phases/mod.rsbin/orbis-node/src/dkg/messages.rscrates/crypto/src/trait.rsbin/orbis-node/src/dkg/coordinator/message_handlers/share.rsbin/orbis-node/src/helpers/helpers.rsbin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rsbin/orbis-node/src/dkg/helpers.rsbin/orbis-node/src/dkg/coordinator/reshare/selection.rsbin/orbis-node/src/dkg/coordinator/mod.rsbin/orbis-node/src/dkg/tests/refresh.rsbin/orbis-node/src/dkg/coordinator/refresh_health_check.rsbin/orbis-node/src/dkg/tests/dkg.rscrates/crypto/src/test_helpers/test_helper.rsbin/orbis-node/src/dkg/session_state.rs
📚 Learning: 2026-04-28T14:49:17.416Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 142
File: bin/orbis-node/src/dkg/helpers.rs:126-145
Timestamp: 2026-04-28T14:49:17.416Z
Learning: In `bin/orbis-node/src/dkg/helpers.rs`, `load_reshare_ring_payload` intentionally prefers the local `RingIndex` bulletin_post_id over the wire-provided one for old-committee nodes. This is by design: local `RingIndex` and `RingShareBundle` are part of the validity boundary. An old-committee member with a stale or missing local post ID is considered unhealthy; falling back to the wire-supplied post ID would mask stale-state drift rather than surface it. Pure receivers (nodes not in the old committee, with no local index entry) are the only case where the wire `bulletin_post_id` is used as a fallback, which is explicitly allowed.
Applied to files:
bin/orbis-node/src/dkg/coordinator/phases/phase2.rsbin/orbis-node/src/dkg/coordinator/ring_storage.rsbin/orbis-node/src/dkg/coordinator/message_handlers/commitment.rsbin/orbis-node/src/dkg/coordinator/message_handlers/session_init.rsbin/orbis-node/src/dkg/coordinator/phases/phase1.rscrates/network/README.mdbin/orbis-node/src/sign/tests.rsbin/orbis-node/README.mdbin/orbis-node/src/dkg/service.rsbin/orbis-node/src/dkg/coordinator/reshare/cleanup.rsbin/orbis-node/src/dkg/tests/reshare.rsbin/orbis-node/src/sign/messages.rsbin/orbis-node/src/pre/tests.rsbin/orbis-node/src/dkg/PROTOCOL_FLOW.mdbin/orbis-node/src/dkg/coordinator/network.rsbin/orbis-node/src/dkg/coordinator/phases/mod.rsbin/orbis-node/src/dkg/messages.rsbin/orbis-node/src/dkg/coordinator/message_handlers/share.rsbin/orbis-node/src/helpers/helpers.rsbin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rsbin/orbis-node/src/dkg/helpers.rsbin/orbis-node/src/dkg/coordinator/reshare/selection.rsbin/orbis-node/src/dkg/coordinator/mod.rsbin/orbis-node/src/dkg/coordinator/refresh_health_check.rsbin/orbis-node/src/dkg/tests/dkg.rscrates/crypto/src/test_helpers/test_helper.rsbin/orbis-node/src/dkg/session_state.rs
📚 Learning: 2026-04-29T16:47:26.051Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 147
File: bin/orbis-node/src/store_secret/service.rs:0-0
Timestamp: 2026-04-29T16:47:26.051Z
Learning: In `bin/orbis-node/src/store_secret/service.rs`, the pre-store `verify_encryption` check (which previously passed ring key, effective key, and policy metadata into `validate_encrypted_document`) was intentionally removed as part of PR `#147` (closes issue `#139`). The rationale: running this verification before storing requires exposing ring key and policy metadata to the validation function, which leaks privacy. Since a caller could always store cryptographically junk data regardless of this check, removing it does not weaken the protocol's security invariants — it only improves privacy. Proof field validation (shared_point/challenge/response) is deferred to downstream consumers.
Applied to files:
bin/orbis-node/src/store_secret/service.rsbin/orbis-node/README.mdREADME.mdbin/orbis-node/src/dkg/tests/reshare.rsbin/orbis-node/src/sign/messages.rsbin/orbis-node/src/dkg/PROTOCOL_FLOW.mdbin/orbis-node/src/dkg/helpers.rsbin/orbis-node/src/dkg/session_state.rs
📚 Learning: 2026-05-11T14:28:55.018Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 152
File: crates/bulletin/src/sourcehub/mod.rs:12-14
Timestamp: 2026-05-11T14:28:55.018Z
Learning: In `crates/bulletin/src/sourcehub/mod.rs` (Rust), the `DEFAULT_THRESHOLD_SIGNATURE_SCHEME` constant references `crypto::THRESHOLD_SIGNATURE_SCHEME`. All builds of orbis-rs are expected to enable at least one crypto feature (bls12-381, decaf377, or a future equivalent), so this constant should NOT be feature-gated at the bulletin crate level. The feature enforcement is handled by the `crypto` crate itself. Avoid flagging missing cfg gates on this constant.
Applied to files:
bin/orbis-node/README.mdcrates/crypto/benches/bls12_381/sign.rscrates/crypto/src/bls12_381/tests/sign_tests.rscrates/crypto/benches/decaf377/sign.rscrates/crypto/benches/bls12_381/pre.rscrates/crypto/src/decaf377/tests/sign_tests.rsbin/orbis-node/src/constants.rscrates/crypto/benches/decaf377/dkg.rsbin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rs
📚 Learning: 2026-05-14T20:15:59.261Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 162
File: bin/cli-tool/src/commands.rs:93-94
Timestamp: 2026-05-14T20:15:59.261Z
Learning: `bin/cli-tool` is a test CLI tool in the `sourcenetwork/orbis-rs` repository. Using `.expect()` (panics) instead of proper error propagation is intentional and acceptable throughout this crate, as it is not production code.
Applied to files:
bin/orbis-node/README.md
📚 Learning: 2026-05-20T18:08:12.109Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 171
File: crates/bulletin/src/sourcehub/mod.rs:238-264
Timestamp: 2026-05-20T18:08:12.109Z
Learning: In `crates/bulletin/src/sourcehub/mod.rs`, the `block_number_nonce` field in `ring_finalized_canonical_hash` does NOT need to be manually incremented. The nonce is read directly from the on-chain bulletin post and is updated by a separate blockchain upon reshare finalization. When `ring_finalized_canonical_hash` is called for a subsequent reshare, the nonce is already current on-chain, so the local `finalized` ring struct should use `..ring` spread without modifying `block_number_nonce`.
Applied to files:
bin/orbis-node/src/dkg/coordinator/reshare/cleanup.rsbin/orbis-node/src/dkg/tests/reshare.rsbin/orbis-node/src/sign/messages.rscrates/crypto/src/trait.rsbin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rsbin/orbis-node/src/dkg/helpers.rs
📚 Learning: 2026-05-20T18:07:01.094Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 171
File: crates/bulletin/src/dummy/mod.rs:152-173
Timestamp: 2026-05-20T18:07:01.094Z
Learning: `DummyBulletin` in `crates/bulletin/src/dummy/mod.rs` is only used for unit tests, not production. Production-level consistency concerns (e.g., nonce mirroring, exact trait contract fidelity) should not be flagged as issues for this type.
Applied to files:
crates/crypto/src/decaf377/tests/deserialization_prop_tests.rscrates/crypto/src/bls12_381/tests/deserialization_prop_tests.rsbin/orbis-node/src/dkg/tests/dkg.rs
🔇 Additional comments (61)
README.md (1)
333-343: LGTM!bin/orbis-node/README.md (3)
17-21: LGTM!
54-79: LGTM!
83-83: LGTM!crates/network/README.md (1)
49-49: LGTM!Also applies to: 80-80, 93-102, 114-114
docker/docker-compose-integration-test.yml (1)
9-11: LGTM!docker/docker-compose-metrics-network-test.yml (1)
3-5: LGTM!docker/docker-compose.3-node.yml (1)
1-3: LGTM!docker/grafana/dashboards/orbis.json (1)
1269-1314: LGTM!Cargo.toml (2)
51-51: LGTM!
54-58: Verify MSRV compatibility for the tonic/prost 0.14.x upgrade before mergeThis workspace declares
workspace.package.rust-version = "1.70". Check that theCargo.lock-resolved versions ofprost,tonic,tonic-build,tonic-prost, andtonic-prost-buildhave crates.ioversion.rust_versionmetadata<= 1.70; if any require a newer compiler, bump the workspace MSRV or pin compatible crate versions.bin/cli-tool/Cargo.toml (1)
12-12: LGTM!bin/orbis-node/Cargo.toml (1)
17-17: LGTM!crates/authn/Cargo.toml (1)
16-16: LGTM!Also applies to: 19-19
crates/proto/Cargo.toml (1)
10-12: LGTM!Also applies to: 15-16
bin/orbis-node/src/constants.rs (1)
175-195: LGTM!bin/orbis-node/src/helpers/create_routers.rs (1)
2-4: LGTM!Also applies to: 13-19, 40-40, 80-80, 134-134
bin/orbis-node/src/main.rs (1)
78-79: LGTM!Also applies to: 360-363, 408-409
crates/network/src/trait.rs (1)
11-27: LGTM!Also applies to: 128-132
crates/network/src/lib.rs (1)
19-19: LGTM!crates/network/src/iroh/router.rs (1)
9-21: LGTM!Also applies to: 46-46, 64-75, 77-100, 103-113, 115-155, 157-178, 180-208, 210-267, 269-289
crates/network/src/metrics.rs (1)
88-95: LGTM!Also applies to: 111-111, 187-195
crates/network/src/iroh/tests.rs (1)
13-13: LGTM!Also applies to: 19-28, 297-376
bin/orbis-node/src/dkg/coordinator/message_handlers/commitment.rs (1)
10-10: LGTM!bin/orbis-node/src/dkg/coordinator/message_handlers/session_init.rs (1)
13-13: LGTM!bin/orbis-node/src/dkg/coordinator/message_handlers/share.rs (1)
11-11: LGTM!Also applies to: 129-129, 149-149, 165-165
bin/orbis-node/src/dkg/coordinator/mod.rs (1)
55-55: LGTM!Also applies to: 67-67, 475-475, 504-504, 512-512, 532-532, 550-550, 561-561, 570-574, 581-581, 588-588
bin/orbis-node/src/dkg/coordinator/network.rs (1)
30-30: LGTM!Also applies to: 59-59, 99-99, 412-412, 511-511
bin/orbis-node/src/dkg/coordinator/phases/mod.rs (1)
39-39: LGTM!Also applies to: 53-53, 66-66, 106-106, 147-147, 183-183, 234-234
bin/orbis-node/src/dkg/coordinator/phases/phase1.rs (1)
5-5: LGTM!Also applies to: 145-145
bin/orbis-node/src/dkg/coordinator/phases/phase2.rs (1)
5-5: LGTM!bin/orbis-node/src/dkg/coordinator/phases/phase4.rs (1)
5-5: LGTM!Also applies to: 14-14
bin/orbis-node/src/dkg/coordinator/refresh_health_check.rs (1)
35-35: LGTM!Also applies to: 177-177, 242-242, 256-256, 287-287, 440-440, 478-478, 503-503
bin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rs (1)
42-42: LGTM!Also applies to: 198-198, 284-284, 333-333
bin/orbis-node/src/dkg/coordinator/reshare/cleanup.rs (1)
13-13: LGTM!Also applies to: 29-29
bin/orbis-node/src/dkg/coordinator/reshare/selection.rs (1)
5-5: LGTM!Also applies to: 74-74, 96-96, 147-147, 164-164, 303-303, 368-368
bin/orbis-node/src/dkg/coordinator/ring_storage.rs (1)
16-16: LGTM!bin/orbis-node/src/dkg/service.rs (1)
130-130: LGTM!bin/orbis-node/src/store_secret/service.rs (1)
193-193: LGTM!bin/orbis-node/src/dkg/PROTOCOL_FLOW.md (1)
205-207: LGTM!crates/crypto/benches/dkg_benchmarks.rs (1)
41-58: LGTM!crates/crypto/benches/bls12_381/dkg.rs (1)
11-17: LGTM!crates/crypto/benches/bls12_381/pre.rs (1)
18-29: LGTM!crates/crypto/benches/bls12_381/sign.rs (1)
16-23: LGTM!crates/crypto/benches/decaf377/dkg.rs (1)
11-17: LGTM!crates/crypto/benches/decaf377/pre.rs (1)
16-29: LGTM!crates/crypto/benches/decaf377/sign.rs (1)
16-23: LGTM!crates/crypto/src/dkg_tests.rs (1)
51-55: LGTM!crates/crypto/src/lifecycle_tests.rs (1)
147-153: LGTM!Also applies to: 281-281
crates/crypto/src/test_helpers/test_helper.rs (1)
49-49: LGTM!Also applies to: 70-73, 223-223, 262-262, 289-289, 303-303, 320-320, 354-354, 382-383, 387-390, 438-438, 473-473, 490-493, 595-595, 613-616, 707-707, 726-729
crates/crypto/src/bls12_381/tests/deserialization_prop_tests.rs (1)
52-52: LGTM!Also applies to: 114-115
crates/crypto/src/bls12_381/tests/pre_tests.rs (1)
27-27: LGTM!crates/crypto/src/bls12_381/tests/sign_tests.rs (1)
29-29: LGTM!Also applies to: 76-76
crates/crypto/src/decaf377/tests/deserialization_prop_tests.rs (1)
50-50: LGTM!Also applies to: 124-125
crates/crypto/src/decaf377/tests/pre_tests.rs (1)
25-25: LGTM!crates/crypto/src/decaf377/tests/sign_tests.rs (1)
26-26: LGTM!Also applies to: 60-60
bin/orbis-node/src/dkg/tests/dkg.rs (1)
831-831: LGTM!Also applies to: 950-950, 994-994, 1042-1042, 1123-1123, 1158-1158, 1185-1185, 1212-1213, 1245-1245, 1281-1281, 1327-1327, 1368-1368, 1370-1370, 1380-1380, 1384-1384, 1392-1392, 1401-1401, 1429-1429
bin/orbis-node/src/dkg/tests/refresh.rs (1)
400-400: LGTM!Also applies to: 498-498, 529-529
bin/orbis-node/src/dkg/tests/reshare.rs (1)
468-468: LGTM!Also applies to: 547-547
bin/orbis-node/src/pre/tests.rs (1)
319-319: LGTM!bin/orbis-node/src/sign/tests.rs (1)
211-211: LGTM!
Comment on lines
+68
to
+79
| /// Maximum in-flight gRPC requests per client connection. | ||
| #[arg( | ||
| long, | ||
| default_value_t = crate::constants::GRPC_CONCURRENCY_LIMIT_PER_CONNECTION | ||
| )] | ||
| pub grpc_concurrency_limit_per_connection: usize, | ||
| /// Maximum concurrent HTTP/2 streams per gRPC client connection. | ||
| #[arg( | ||
| long, | ||
| default_value_t = crate::constants::GRPC_MAX_CONCURRENT_STREAMS | ||
| )] | ||
| pub grpc_max_concurrent_streams: u32, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm the new args are defined without an existing range guard.
rg -n -C3 'grpc_concurrency_limit_per_connection|grpc_max_concurrent_streams|value_parser|range\(1\.\.\)' bin/orbis-node/src/helpers/launch.rs
# 2) Trace where these fields are consumed in server setup paths.
rg -n -C3 'grpc_concurrency_limit_per_connection|grpc_max_concurrent_streams|concurrency_limit_per_connection|http2_max_concurrent_streams' bin/orbis-node/srcRepository: sourcenetwork/orbis-rs
Length of output: 1947
Add positive range validation for the new gRPC limit CLI flags
These clap args have defaults but no value_parser/range guard, so passing 0 is currently allowed; constrain both to 1.. to fail fast on invalid configs.
Proposed fix
#[arg(
long,
+ value_parser = clap::value_parser!(usize).range(1..),
default_value_t = crate::constants::GRPC_CONCURRENCY_LIMIT_PER_CONNECTION
)]
pub grpc_concurrency_limit_per_connection: usize,
@@
#[arg(
long,
+ value_parser = clap::value_parser!(u32).range(1..),
default_value_t = crate::constants::GRPC_MAX_CONCURRENT_STREAMS
)]
pub grpc_max_concurrent_streams: u32,🤖 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/helpers/launch.rs` around lines 68 - 79, The two CLI args
grpc_concurrency_limit_per_connection and grpc_max_concurrent_streams currently
accept 0; add clap value parsers with a positive range to validate inputs and
fail fast. For grpc_concurrency_limit_per_connection use value_parser =
clap::value_parser!(usize).range(1..) and for grpc_max_concurrent_streams use
value_parser = clap::value_parser!(u32).range(1..), placing these attributes on
the respective struct fields so passing 0 will be rejected.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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 `@crates/crypto/src/decaf377/tests/sign_tests.rs`:
- Around line 136-157: The tests hash_to_sign_scalar_is_deterministic and
hash_to_sign_scalar_separates_domains currently compare Result<Fr> values;
change them to assert the calls succeed and compare the underlying Fr values
instead (e.g., call hash_to_sign_scalar(...).unwrap() or use an assert_ok
helper) so you validate the produced scalar rather than matching errors — update
both occurrences of hash_to_sign_scalar(FROST_BINDING_DOMAIN, transcript),
hash_to_sign_scalar(FROST_CHALLENGE_DOMAIN, transcript), and
hash_to_sign_scalar(SIGN_DERIVATION_DOMAIN, transcript) to unwrap or assert
success before asserting equality/inequality.
🪄 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: 05b3664c-6a59-4eb0-94fb-d06c14ee17bc
📒 Files selected for processing (4)
bin/orbis-node/src/pre/coordinator/initiator.rsbin/orbis-node/src/pre/tests.rscrates/crypto/src/decaf377/sign.rscrates/crypto/src/decaf377/tests/sign_tests.rs
📜 Review details
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 152
File: crates/bulletin/src/sourcehub/mod.rs:12-14
Timestamp: 2026-05-11T14:28:55.018Z
Learning: In `crates/bulletin/src/sourcehub/mod.rs` (Rust), the `DEFAULT_THRESHOLD_SIGNATURE_SCHEME` constant references `crypto::THRESHOLD_SIGNATURE_SCHEME`. All builds of orbis-rs are expected to enable at least one crypto feature (bls12-381, decaf377, or a future equivalent), so this constant should NOT be feature-gated at the bulletin crate level. The feature enforcement is handled by the `crypto` crate itself. Avoid flagging missing cfg gates on this constant.
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 147
File: bin/orbis-node/src/store_secret/service.rs:0-0
Timestamp: 2026-04-29T16:47:26.051Z
Learning: In `bin/orbis-node/src/store_secret/service.rs`, the pre-store `verify_encryption` check (which previously passed ring key, effective key, and policy metadata into `validate_encrypted_document`) was intentionally removed as part of PR `#147` (closes issue `#139`). The rationale: running this verification before storing requires exposing ring key and policy metadata to the validation function, which leaks privacy. Since a caller could always store cryptographically junk data regardless of this check, removing it does not weaken the protocol's security invariants — it only improves privacy. Proof field validation (shared_point/challenge/response) is deferred to downstream consumers.
📚 Learning: 2026-04-29T16:47:26.051Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 147
File: bin/orbis-node/src/store_secret/service.rs:0-0
Timestamp: 2026-04-29T16:47:26.051Z
Learning: In `bin/orbis-node/src/store_secret/service.rs`, the pre-store `verify_encryption` check (which previously passed ring key, effective key, and policy metadata into `validate_encrypted_document`) was intentionally removed as part of PR `#147` (closes issue `#139`). The rationale: running this verification before storing requires exposing ring key and policy metadata to the validation function, which leaks privacy. Since a caller could always store cryptographically junk data regardless of this check, removing it does not weaken the protocol's security invariants — it only improves privacy. Proof field validation (shared_point/challenge/response) is deferred to downstream consumers.
Applied to files:
bin/orbis-node/src/pre/coordinator/initiator.rscrates/crypto/src/decaf377/tests/sign_tests.rsbin/orbis-node/src/pre/tests.rscrates/crypto/src/decaf377/sign.rs
📚 Learning: 2026-04-30T20:31:37.077Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 148
File: bin/orbis-node/src/dkg/coordinator/phases.rs:689-695
Timestamp: 2026-04-30T20:31:37.077Z
Learning: In `bin/orbis-node/src/dkg/coordinator/phases.rs`, `wait_for_reshare_bulletin_finalized` intentionally gates PSS-claim release only on `next_peer_ids` and `new_threshold` being `None`. This is by design: an upcoming PR (related to PR `#148`) will make the threshold signature the trigger that clears those fields on the bulletin, so this poll condition will be sufficient once that change lands. Do not flag the absence of explicit signature-metadata verification in this function as a bug.
Applied to files:
bin/orbis-node/src/pre/coordinator/initiator.rsbin/orbis-node/src/pre/tests.rs
📚 Learning: 2026-05-11T14:42:47.309Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 152
File: bin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rs:274-282
Timestamp: 2026-05-11T14:42:47.309Z
Learning: In `bin/orbis-node/src/dkg/coordinator/reshare/bulletin_update.rs`, `block_number_nonce` in `RingPayload` is intentionally passed through unchanged in the signed `RingReshareUpdateStatement`. SourceHub (see sourcenetwork/sourcehub#110) rotates `block_number_nonce` atomically server-side when applying the threshold-signature bulletin update (together with `new_peer_ids`/`new_threshold` → `peer_ids`/`threshold`). A replay of an old signature is benign by design: if the prior update failed, the nodes were already in the ready state awaiting the chain update, so re-submitting the same signature has no harmful effect.
Applied to files:
bin/orbis-node/src/pre/coordinator/initiator.rscrates/crypto/src/decaf377/tests/sign_tests.rsbin/orbis-node/src/pre/tests.rs
📚 Learning: 2026-04-28T14:49:17.416Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 142
File: bin/orbis-node/src/dkg/helpers.rs:126-145
Timestamp: 2026-04-28T14:49:17.416Z
Learning: In `bin/orbis-node/src/dkg/helpers.rs`, `load_reshare_ring_payload` intentionally prefers the local `RingIndex` bulletin_post_id over the wire-provided one for old-committee nodes. This is by design: local `RingIndex` and `RingShareBundle` are part of the validity boundary. An old-committee member with a stale or missing local post ID is considered unhealthy; falling back to the wire-supplied post ID would mask stale-state drift rather than surface it. Pure receivers (nodes not in the old committee, with no local index entry) are the only case where the wire `bulletin_post_id` is used as a fallback, which is explicitly allowed.
Applied to files:
bin/orbis-node/src/pre/coordinator/initiator.rsbin/orbis-node/src/pre/tests.rs
📚 Learning: 2026-05-11T14:28:55.018Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 152
File: crates/bulletin/src/sourcehub/mod.rs:12-14
Timestamp: 2026-05-11T14:28:55.018Z
Learning: In `crates/bulletin/src/sourcehub/mod.rs` (Rust), the `DEFAULT_THRESHOLD_SIGNATURE_SCHEME` constant references `crypto::THRESHOLD_SIGNATURE_SCHEME`. All builds of orbis-rs are expected to enable at least one crypto feature (bls12-381, decaf377, or a future equivalent), so this constant should NOT be feature-gated at the bulletin crate level. The feature enforcement is handled by the `crypto` crate itself. Avoid flagging missing cfg gates on this constant.
Applied to files:
crates/crypto/src/decaf377/tests/sign_tests.rsbin/orbis-node/src/pre/tests.rscrates/crypto/src/decaf377/sign.rs
📚 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/decaf377/tests/sign_tests.rscrates/crypto/src/decaf377/sign.rs
📚 Learning: 2026-05-13T18:55:29.338Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 161
File: bin/orbis-node/src/dkg/coordinator/refresh_health_check.rs:147-163
Timestamp: 2026-05-13T18:55:29.338Z
Learning: In `bin/orbis-node/src/dkg/coordinator/refresh_health_check.rs`, the partial-broadcast / local-rollback split-brain scenario in `run_selector` was explicitly considered and accepted as a pragmatic trade-off. The refresh health check is a diagnostic layer on top of the actual key ceremony; the underlying key material is persisted independently. Adding 2PC or compensating rollback broadcasts would just shift the same class of partial-failure risk to the new coordination round, creating an infinite nesting problem. The design intentionally accepts potential transient inconsistency in the health-check staging state.
Applied to files:
bin/orbis-node/src/pre/tests.rs
📚 Learning: 2026-03-03T15:27:38.961Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 87
File: bin/orbis-node/src/tests/mod.rs:3-3
Timestamp: 2026-03-03T15:27:38.961Z
Learning: In the orbis-rs repository, integration tests (including Docker-based tests in bin/orbis-node/src/tests/integration.rs) should run by default during `cargo test` without requiring a feature flag.
Applied to files:
bin/orbis-node/src/pre/tests.rs
📚 Learning: 2026-05-20T18:07:01.094Z
Learnt from: JesseAbram
Repo: sourcenetwork/orbis-rs PR: 171
File: crates/bulletin/src/dummy/mod.rs:152-173
Timestamp: 2026-05-20T18:07:01.094Z
Learning: `DummyBulletin` in `crates/bulletin/src/dummy/mod.rs` is only used for unit tests, not production. Production-level consistency concerns (e.g., nonce mirroring, exact trait contract fidelity) should not be flagged as issues for this type.
Applied to files:
bin/orbis-node/src/pre/tests.rs
🔇 Additional comments (15)
bin/orbis-node/src/pre/coordinator/initiator.rs (1)
203-225: LGTM!bin/orbis-node/src/pre/tests.rs (3)
14-16: LGTM!Also applies to: 23-23, 29-29
316-347: LGTM!
1335-1435: LGTM!crates/crypto/src/decaf377/sign.rs (8)
22-25: LGTM!Also applies to: 34-34, 161-162
166-183: LGTM!
214-228: LGTM!
230-240: LGTM!
378-402: LGTM!
442-458: LGTM!
520-532: LGTM!
535-551: LGTM!crates/crypto/src/decaf377/tests/sign_tests.rs (3)
2-5: LGTM!
28-31: LGTM!
62-65: LGTM!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.