feat: Add multisig e2e benchmark test#348
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChangesAdds a new workspace-integrated Rust benchmark for Guardian multisig workflows. It supports fixture preparation, validated configuration, preflight and bootstrap operations, retries, canonicalization tracking, JSONL measurements, summaries, and documented local testnet execution. Multisig E2E Benchmark
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant BenchmarkRunner
participant MultisigClient
participant GuardianClient
CLI->>BenchmarkRunner: run configured benchmark
BenchmarkRunner->>MultisigClient: create and execute proposal
BenchmarkRunner->>GuardianClient: poll proposal delta
GuardianClient-->>BenchmarkRunner: canonicalization status
BenchmarkRunner-->>CLI: write JSONL report and summary
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new Rust workspace benchmark harness that runs an end-to-end multisig workflow against real Guardian + Miden endpoints, using two persistent 1-of-1 accounts and recording per-operation latency artifacts.
Changes:
- Adds a new workspace member crate
guardian-multisig-e2e-benchmarkwith CLI subcommands to prepare fixtures, preflight, bootstrap, run, and summarize. - Introduces a TOML-runner configuration and local README workflow, producing JSONL reports plus manifest/summary/canonicalization artifacts.
- Implements retry handling for known transient Guardian/Miden sync races and collects deferred canonicalization observations.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Registers benchmarks/multisig-e2e as a workspace member. |
| Cargo.lock | Adds lock entries for the new benchmark crate and its dependencies. |
| benchmarks/multisig-e2e/Cargo.toml | Defines the new benchmark crate, dependencies, and Tokio runtime setup. |
| benchmarks/multisig-e2e/README.md | Documents the intended real-account workflow, artifacts, and interpretation. |
| benchmarks/multisig-e2e/testnet.local.toml | Provides a sample local profile for testnet runs (ops/amount/intervals/timeouts/artifacts dir). |
| benchmarks/multisig-e2e/.gitignore | Ignores generated benchmark report artifacts under reports/. |
| benchmarks/multisig-e2e/src/main.rs | Implements the CLI entrypoint and subcommands wiring. |
| benchmarks/multisig-e2e/src/config.rs | Adds TOML config loading/validation and Miden endpoint parsing. |
| benchmarks/multisig-e2e/src/fixture.rs | Adds fixture generation/persistence for two persistent accounts (Alice/Bob). |
| benchmarks/multisig-e2e/src/runtime.rs | Builds benchmark clients, pulls/syncs accounts, and connects an authenticated Guardian observer. |
| benchmarks/multisig-e2e/src/runner.rs | Core benchmark runner: operation loop, retries, note waiting, report writing, and summary generation (with unit tests). |
| benchmarks/multisig-e2e/src/canonicalization.rs | Deferred canonicalization tracking and post-run observation drain (with unit tests). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
benchmarks/multisig-e2e/src/runtime.rs (1)
43-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
MultisigClientbuilder chain acrossfixture.rsandruntime.rs. Both files build aMultisigClientvia the identicalmiden_endpoint/guardian_endpoint/account_dir/with_secret_key/buildsequence before diverging into create-vs-load logic; the shared root cause is no common construction helper.
benchmarks/multisig-e2e/src/runtime.rs#L43-L74: extract the builder chain (lines 52-59) into a shared helper (e.g.build_client(fixture, account, config) -> Result<(MultisigClient, TempDir)>) that bothload_clientandfixture::preparecan call, keeping each caller's post-build steps (pull_account/sync_with_retryhere) separate.benchmarks/multisig-e2e/src/fixture.rs#L69-L98: replace the builder chain (lines 74-81) with the same shared helper, keepingcreate_account/push_accountas this call site's own follow-up steps.🤖 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 `@benchmarks/multisig-e2e/src/runtime.rs` around lines 43 - 74, Extract the duplicated MultisigClient construction into a shared helper, such as build_client(fixture, account, config), returning the client and TempDir while preserving the existing endpoint, account directory, secret-key, and build configuration. In benchmarks/multisig-e2e/src/runtime.rs lines 43-74, call the helper from load_client and retain pull_account and sync_with_retry there; in benchmarks/multisig-e2e/src/fixture.rs lines 69-98, call the same helper and retain create_account and push_account there.benchmarks/multisig-e2e/src/runner.rs (2)
504-514: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry classification depends on brittle substring matching of Guardian error text.
is_retryable_proposal_errorclassifies transient failures by checking for exact substrings like"conflict_pending_delta"and"There's already a pending change for this account"insideMultisigError::GuardianServer. If the Guardian server's wording changes, this silently stops retrying and previously-transient errors become hard failures.Please confirm whether
miden_multisig_client::MultisigError/the Guardian server response exposes a structured error code/kind for "pending delta conflict" that could replace this string match.🤖 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 `@benchmarks/multisig-e2e/src/runner.rs` around lines 504 - 514, Inspect miden_multisig_client::MultisigError and the Guardian server response types for a structured error code or kind representing a pending-delta conflict. Update is_retryable_proposal_error to classify GuardianServer errors using that structured value instead of matching message text, while preserving retry behavior for GuardianConnection and MidenClient errors.
646-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "required balance" formula (also in
preflight, Line 135).
config.amount.saturating_mul(config.operations.div_ceil(2))is computed identically here and inpreflight(Line 135). Extract a shared helper to avoid the two copies drifting apart if the funding formula ever changes.♻️ Proposed refactor
+fn required_balance(config: &RunConfig) -> u64 { + config.amount.saturating_mul(config.operations.div_ceil(2)) +} + fn ensure_starting_balances( clients: &[BenchClient], faucet_id: AccountId, config: &RunConfig, ) -> Result<()> { - let required = config.amount.saturating_mul(config.operations.div_ceil(2)); + let required = required_balance(config);And in
preflight(Line 135):- let required = config.amount.saturating_mul(config.operations.div_ceil(2)); + let required = required_balance(config);🤖 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 `@benchmarks/multisig-e2e/src/runner.rs` around lines 646 - 664, Extract the shared required-balance calculation from ensure_starting_balances and preflight into a helper, using the existing config.amount and config.operations inputs. Replace both inline config.amount.saturating_mul(config.operations.div_ceil(2)) expressions with that helper while preserving the current funding formula and validation behavior.
🤖 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 `@benchmarks/multisig-e2e/src/canonicalization.rs`:
- Around line 118-152: Update finish so each queued observation gets its own
deadline anchored to queued.started plus self.timeout, instead of reusing one
Instant computed at finish entry. Remove the shared deadline and pass the
per-observation deadline to observe_delta, preserving the existing sequential
processing and elapsed-time reporting.
In `@benchmarks/multisig-e2e/src/fixture.rs`:
- Around line 125-136: Update the non-Unix `restrict_permissions` implementation
to return an explicit error instead of silently succeeding, so callers cannot
persist secret keys while assuming permissions were restricted. Keep the Unix
implementation unchanged and preserve the existing `Result<()>` contract.
In `@benchmarks/multisig-e2e/src/main.rs`:
- Around line 62-66: Update the network_id derivation in the fixture setup to
use an explicit network configuration value rather than inspecting
fixture.miden_endpoint. Map each supported network setting to its corresponding
NetworkId, including Mainnet, Testnet, and Devnet, and avoid defaulting unknown
endpoints to Devnet.
In `@benchmarks/multisig-e2e/src/runtime.rs`:
- Around line 95-102: Update is_miden_sync_tip_ahead to classify the retryable
sync-tip failure using the structured MultisigError variant or error code,
rather than matching text within MidenClient messages. Preserve the existing
behavior for the specific block-to versus chain-tip condition and return false
for unrelated errors.
---
Nitpick comments:
In `@benchmarks/multisig-e2e/src/runner.rs`:
- Around line 504-514: Inspect miden_multisig_client::MultisigError and the
Guardian server response types for a structured error code or kind representing
a pending-delta conflict. Update is_retryable_proposal_error to classify
GuardianServer errors using that structured value instead of matching message
text, while preserving retry behavior for GuardianConnection and MidenClient
errors.
- Around line 646-664: Extract the shared required-balance calculation from
ensure_starting_balances and preflight into a helper, using the existing
config.amount and config.operations inputs. Replace both inline
config.amount.saturating_mul(config.operations.div_ceil(2)) expressions with
that helper while preserving the current funding formula and validation
behavior.
In `@benchmarks/multisig-e2e/src/runtime.rs`:
- Around line 43-74: Extract the duplicated MultisigClient construction into a
shared helper, such as build_client(fixture, account, config), returning the
client and TempDir while preserving the existing endpoint, account directory,
secret-key, and build configuration. In benchmarks/multisig-e2e/src/runtime.rs
lines 43-74, call the helper from load_client and retain pull_account and
sync_with_retry there; in benchmarks/multisig-e2e/src/fixture.rs lines 69-98,
call the same helper and retain create_account and push_account there.
🪄 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: a612c448-a198-468e-af1d-d887e0cd8962
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlbenchmarks/multisig-e2e/.gitignorebenchmarks/multisig-e2e/Cargo.tomlbenchmarks/multisig-e2e/README.mdbenchmarks/multisig-e2e/src/canonicalization.rsbenchmarks/multisig-e2e/src/config.rsbenchmarks/multisig-e2e/src/fixture.rsbenchmarks/multisig-e2e/src/main.rsbenchmarks/multisig-e2e/src/runner.rsbenchmarks/multisig-e2e/src/runtime.rsbenchmarks/multisig-e2e/testnet.local.toml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #348 +/- ##
=======================================
Coverage ? 78.04%
=======================================
Files ? 167
Lines ? 31904
Branches ? 0
=======================================
Hits ? 24900
Misses ? 7004
Partials ? 0 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Add multisig e2e benchmark test.
It uses real accounts and sends p2id notes between them.
Summary by CodeRabbit