fix(contracts): skip cleanup_pubkey_mapping when signer count does n…

[zeljkoX]

Calling exec.cleanup_pubkey_mapping after update_signers_and_threshold
unconditionally produces a STARK-invalid proof when the storage map has
exactly one entry (e.g. add-signer from a 1-signer account). The proc's
internal loop guard already short-circuits in this case, so the body never
ran — but entering the proc at all perturbs the prover trace enough to fail
node-side STARK verification with constraint mismatch: quotient * vanishing != folded constraints.

Gate the call on init_num_of_approvers > new_num_of_approvers so cleanup
only runs when the signer set actually shrinks. Add-signer and threshold-only
updates skip the call entirely; remove-signer behavior is unchanged.
Applied to both Falcon and ECDSA multisig MASM. The existing add-signer test
is extended to call prove_next_block() so MockChain exercises proof
generation and would catch this class of regression.

TODO: Open PR against https://github.com/OpenZeppelin/miden-confidential-contracts and verify with @onurinanc

Summary by CodeRabbit

• Refactor

  • Optimized multisig contract efficiency by making public key mapping cleanup conditional, now executing only when approver counts differ, with improvements applied to both standard and ECDSA contract variants

• Tests

  • Strengthened signer management tests by improving transaction handling and chain state progression to ensure proper validation after threshold updates

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 630e8a55-b058-4b1c-81cd-a118923b0c1b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

Both multisig and multisig_ecdsa contracts now conditionally execute pubkey map cleanup based on approver count comparison. The test is updated to make the mock chain mutable and properly advance block state after transaction execution before validating storage changes.

Changes

Conditional Cleanup in Multisig Contracts

Layer / File(s)	Summary
Conditional cleanup logic crates/contracts/masm/auth/multisig.masm, crates/contracts/masm/auth/multisig_ecdsa.masm	Both contracts replace unconditional cleanup_pubkey_mapping invocation with conditional logic: load init_num_of_approvers and new_num_of_approvers, derive a boolean via u32assert2 + u32lt, and only execute cleanup when true; otherwise discard stack values.
Test chain state advancement crates/contracts/tests/auth/multisig.rs	mock_chain is declared mutable; after executing the signer update transaction, the test registers it, proves the next block, then applies the account delta and validates storage state.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops through contract code,
Where cleanup now takes the conditional road,
Only cleaning when numbers require the call,
The test advances the chain through it all! 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: making cleanup_pubkey_mapping conditional based on signer count reduction.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-add-signer

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR fixes a STARK proof verification failure in the multisig contracts by avoiding entering cleanup_pubkey_mapping unless the signer set actually shrinks, and strengthens the regression test to exercise block proving.

Changes:

• Gate cleanup_pubkey_mapping behind a init_num_of_approvers > new_num_of_approvers check in both Falcon and ECDSA multisig MASM.
• Extend the “add signer from single signer” test to add the executed tx to MockChain and call prove_next_block() to ensure proof generation is covered.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
crates/contracts/tests/auth/multisig.rs	Extends add-signer test to prove the next block (covers proof generation path).
crates/contracts/masm/auth/multisig.masm	Conditionally calls cleanup_pubkey_mapping only when approver count decreases.
crates/contracts/masm/auth/multisig_ecdsa.masm	Same conditional cleanup gating as Falcon variant for parity.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.37%. Comparing base (7bff98c) to head (acb52f9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #259      +/-   ##
==========================================
+ Coverage   75.28%   76.37%   +1.09%     
==========================================
  Files         131      136       +5     
  Lines       23314    24888    +1574     
==========================================
+ Hits        17551    19009    +1458     
- Misses       5763     5879     +116     

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 850d590...acb52f9. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.