Skip to content

Comments

feat: Custom Mutisig Pallet #352

Merged
czareko merged 30 commits intotestnet/planckfrom
feat/custom_multisig
Jan 27, 2026
Merged

feat: Custom Mutisig Pallet #352
czareko merged 30 commits intotestnet/planckfrom
feat/custom_multisig

Conversation

@czareko
Copy link
Collaborator

@czareko czareko commented Jan 16, 2026

@czareko czareko marked this pull request as ready for review January 22, 2026 01:10
@czareko czareko marked this pull request as draft January 22, 2026 04:26
@czareko czareko marked this pull request as ready for review January 26, 2026 04:46
@czareko czareko requested review from illuzen and n13 January 26, 2026 04:46
@n13
Copy link
Collaborator

n13 commented Jan 27, 2026

One thin - apparently we auto clean expired proposals on propose.

This risks timing out when too many proposals are expired locking up the entire pallet.

So I would put a reasonable limit, like at most clean up 5 expired proposals on each propose call, then we never have this issue.

Edit: I see it's capped by 200.

pub const MaxActiveProposals: u32 = 100; // Max active proposals per multisig
pub const MaxTotalProposalsInStorage: u32 = 200; // Max total in storage (Active + Executed + Cancelled)

Aren't proposals automatically deleted on Cancel and execute?

If so, then cancelled and executed states are never stored, also we only need max active, the other value is then not needed.

Which would then cap the removal process at 99, which I guess is fine, although if you manage to fill it up to 100, no new proposals are possible, and therefore nothing gets cleaned up either.

@n13
Copy link
Collaborator

n13 commented Jan 27, 2026

Review of PR #352: Custom Multisig Pallet

Target Branch: testnet/planck
Spec: Multisig Pallet - Requirements Specification

Summary

This PR implements the Custom Multisig Pallet as per the specification. The implementation generally aligns well with the requirements, covering the core lifecycle, economic security models (burned fees, locked deposits), and storage limits.

However, there is a Critical Issue regarding threshold-1 multisigs and a few observations for configuration and testing.

Critical Findings

1. Missing Auto-Execution for Threshold-1 Proposals

The specification states in the propose API requirements:

"If threshold = 1, auto-executes and immediately removes proposal"

The docstring for propose in src/lib.rs also claims this behavior:

"If threshold = 1, auto-executes and immediately removes proposal"

Issue: The implementation in propose (lines 501-673) does NOT implement this logic. It creates the proposal, adds the proposer's approval, stores it, and returns. It never checks if threshold == 1 to trigger do_execute.

Impact:

  • 1-of-N Multisigs: Requires a second transaction (approve) from another signer to execute, defeating the purpose of 1-of-N.
  • 1-of-1 Multisigs: The proposer is automatically added to approvals during propose. The approve function checks !proposal.approvals.contains(&approver). Since the proposer is already in the list, they cannot call approve. Since there are no other signers, the proposal is permanently stuck in storage until it expires.

Recommendation:
Add the check at the end of propose:

// ... inside propose ...
Proposals::<T>::insert(&multisig_address, proposal_id, proposal.clone());
// ... update counters ...

// Check for auto-execution
if multisig_data.threshold == 1 {
    Self::do_execute(multisig_address, proposal_id, proposal)?;
}

Self::deposit_event(Event::ProposalCreated { ... });
Ok(())

Observations & Suggestions

1. Fee Configuration

In runtime/src/configs/mod.rs:

  • MultisigFee (Creation): 100 * MILLI_UNIT (0.1 UNIT)
  • ProposalFee (Action): 1000 * MILLI_UNIT (1 UNIT)

Observation: The Proposal Fee is 10x higher than the Multisig Creation Fee. Typically, creating a permanent entity (Multisig) is more expensive than using it. While the spec mentions ProposalFee is the "Primary revenue" and "Spam prevention", ensure this balance is intended. A cheap creation fee might encourage spamming empty multisig accounts (though MultisigDeposit helps mitigate this).

2. Test Coverage

  • Threshold-1 Test: There is no test case for a Multisig with threshold = 1. Adding a test case for create_multisig(threshold=1) -> propose() would have caught the stuck proposal issue.
  • Filibuster Protection: The tests for per_signer_proposal_limit_enforced are good and verify the spec requirement.

3. Spec Compliance Checklist

Requirement Status Notes
Multisig Account
Deterministic Address Uses sorted signers + nonce + TrailingZeroInput
Immutable Config Signers/Threshold set on creation
Unique Identification Global nonce ensures uniqueness
Proposal Lifecycle
Full Call Storage Stores BoundedVec<u8>
Auto-Execution Missing for Threshold=1
Auto-Cleanup (Active) propose cleans expired proposals
Economics
Burned Fees Uses withdraw (burned in typical config)
Locked Deposits Uses reserve/unreserve
Security
Reentrancy Protection do_execute uses CEI pattern correctly
Spam Prevention Limits on signers, proposals, call size enforced
Filibuster Protection Per-signer limits enforced

Conclusion

The PR is high quality but must not be merged until the Threshold-1 execution bug is fixed, as it renders 1-of-1 multisigs unusable.

@czareko
Copy link
Collaborator Author

czareko commented Jan 27, 2026

@n13 , Threshold=1 case fixed, MaxActiveProposals - removed

Copy link
Collaborator

@n13 n13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG

@czareko czareko merged commit 1c2b978 into testnet/planck Jan 27, 2026
4 checks passed
czareko added a commit that referenced this pull request Feb 17, 2026
* feat: qp-header for Planck release (#338)

* no circuit padding hasher for block header

* *use custom hasher for header that encodes the pre-image in a felt aligned manner.

* *bespoke header hasher

* *patch bug with hash header fall back

* *replace custom poseidon header hasher on
generic header with a fork of header that has a custom
hasher that overrides default on the header trait.

* *rmv commented out impl of prior hash method

* Update primitives/header/src/lib.rs

Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>

* fixed tests

* Use inherent struct method

* Update Cargo.toml

---------

Co-authored-by: Ethan <tylercemer@gmail.com>
Co-authored-by: illuzen <illuzen@users.noreply.github.com>
Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>

* burn high-security fee instead of sending to treasury (#357)

* Continuous Mining (#358)

* new block trigger

* dedupe logic, remove unnecessary field

* simplify again

* fmt

* clippy

* fmt

* feat: Vesting and MerkleAirdrop removed (#360)

* feat: Merkle Airdrop - removed

* feat: Vesting pallet - removed

* fix: Clippy for header

* Enable wormhole addresses in genesis (#359)

* generate transfer proofs for genesis endowment

* fmt

* fix balances tests

* fmt

* add recover funds call (#361)

* add recover funds call

* add unit tests

* fix up remaining tests

* cargo fmt

* fix benchmarks, update weights

* fix merge error

* feat: Custom Mutisig Pallet  (#352)

* feat: Merkle Airdrop - removed

* feat: Vesting pallet - removed

* poc: First multisig version

* fix: Taplo

* fix: Execution for expired & address simplified fallback

* draft: Historical proposals - paginaged endpoint

* draft: Historical proposals - from events only

* ref: Events renamed + Deposits logic simplified

* feat: GracePeriod param removed

* fix: Reentrancy

* feat: History cleaning redesigned

* fix: Expiry - additional validation

* feat: Proposal nonce

* feat: Dynamic weights

* feat: Multisig deposit fee

* feat: MaxExpiry param

* feat: Fees to Treasury

* feat: History removable only by signers

* fix: Weights

* feat: Fees burned

* feat: Filibuster protection

* feat: Proposals auto cleaning

* feat: Proposal id - nonce instead of hash

* feat: Calls - production whitelist

* feat: Remove call whitelisting

* fix: Test fix after balances pallet update

* fix: Review cleaning

* fix: Multisig - auto-cleaning expanded (#364)

* fix: Multisig - auto-cleaning expanded

* fix: Weights related to storage size

* QUIC miner (#363)

* quic implementation

* refactor for readability

* simplify

* miner initiates, multiple miners supported

* simplify loop further

* short job counters

* simplify new_full

* gracefully handle invalid seals

* remove unused and log misbehaving miners

* emoji

* fmt

* taplo

* improve readability, logs, documentation

* feat: High-Security Integration for Multisig Pallet (#368)

* feat: Multisig + HS integrated

* feat: Weights update

* fix: Benchmarks + README

* feat: HS trait defined in primitives

* fix: Taplo

* fix: Sell order tracking

* feat: Deterministic address + simplified cleaning

* fix: Deposits for multisig

* fix: Dynamic weight + benchmarks refactor

* feat: Execute - separated

* feat: Multisig - benchamarks refactor

* fix: Benchmarks - corrected HS multisig cost

* feat: Dynamic cleaning methods

* feat: Approve dissolve - two variants

* feat: Approval with negative weight

* Switch to new plonky2  (#367)

* No pad hasher header (#327)

* no circuit padding hasher for block header

* *use custom hasher for header that encodes the pre-image in a felt aligned manner.

* *bespoke header hasher

* *patch bug with hash header fall back

* *replace custom poseidon header hasher on
generic header with a fork of header that has a custom
hasher that overrides default on the header trait.

* *rmv commented out impl of prior hash method

* Update primitives/header/src/lib.rs

Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>

* fixed tests

* Use inherent struct method

* Update Cargo.toml

---------

Co-authored-by: Ethan <tylercemer@gmail.com>
Co-authored-by: illuzen <illuzen@users.noreply.github.com>

* Verify header in the wormhole proof (#295)

* Use canonical balances pallet and add support for assets in wormhole (#333)

* Use canonical balances pallet, add assets support to wormhole

* Ignore old tests

* Remove tests

* Override native asset id

* Use poseidon hasher

* Use poseidon storage hasher

* Passing wormhole proof tests

* Update binaries

* Update binaries

* Update zk-circuits crates

* Use crates.io dep versions

* Use `ToFelts` trait in the wormhole pallet (#347)

* Apply ToFelts changes to wormhole

* Fix checks

* Passing tests

* Revert unit test line

* Rename explicit AccountId

* Add ValidateUnsigned impl to wormhole (#353)

* feat: aggregated proof verification in wormhole (#351)

* Aggregated proofs verification wormhole

* clippy

* check block hash in agg proof

* feat: quantized funding amounts (#354)

* feat/quantized_wormhole_funding_amount

* *fix formatting

* *rollback zk enabled circuit artfiact builds at runtime.

* fmt

---------

Co-authored-by: illuzen <illuzen@users.noreply.github.com>

* Enforce miner wormhole address (#344)

* feat: qp-header for Planck release (#338)

* no circuit padding hasher for block header

* *use custom hasher for header that encodes the pre-image in a felt aligned manner.

* *bespoke header hasher

* *patch bug with hash header fall back

* *replace custom poseidon header hasher on
generic header with a fork of header that has a custom
hasher that overrides default on the header trait.

* *rmv commented out impl of prior hash method

* Update primitives/header/src/lib.rs

Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>

* fixed tests

* Use inherent struct method

* Update Cargo.toml

---------

Co-authored-by: Ethan <tylercemer@gmail.com>
Co-authored-by: illuzen <illuzen@users.noreply.github.com>
Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>

* Exponentially decaying token rewards (#340)

* exponentially decaying token rewards

* script to simulate emissions

* clean up constants and switch python script to rust test

* log if we hit max supply somehow

* convert rewards_address to rewards_preimage to enforce wormhole address usage

* better documentation

* change arg name

* Exponentially decaying token rewards (#340)

* exponentially decaying token rewards

* script to simulate emissions

* clean up constants and switch python script to rust test

* log if we hit max supply somehow

* convert rewards_address to rewards_preimage to enforce wormhole address usage

* better documentation

* change arg name

* address style comments

---------

Co-authored-by: Cezary Olborski <cezary.olborski@gmail.com>
Co-authored-by: Ethan <tylercemer@gmail.com>
Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>

* update qp-poseidon version

* made transfer count per-recipient

* feat: enable wormhole verifier tests (#356)

* bring back wormhole transfer proof generation tests

* fmt

---------

Co-authored-by: illuzen <illuzen@users.noreply.github.com>

* remove painful test, we sent it to quantus-cli

* burn half the volume fee

* fmt

* use new plonky2-verifier crate

* fix: Remove no_random feature and patch plonky2 crates to use local versions

- Remove no_random feature from qp-wormhole-verifier and qp-zk-circuits-common dependencies
- Add patches to use local qp-plonky2 and qp-plonky2-field with fixed rand feature handling
- These changes ensure consistent feature resolution across all workspace members

* lock

* new agg logic

* better logging of agg proof failure modes

* refresh bins

* only one proof verified event necessary

* update to latest rusty crystals

* no minimum, no single proof verification

* lock

* handle new derivation rules

* fmt

* put wormhole transfer minimum back in, remove unused single-proof files

* better lock

* remove local plonky2 references

* fix build.rs bin validation

* remove unused functions

* format

* no more local deps

* missing dev accounts

* clippy

---------

Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>
Co-authored-by: Ethan <tylercemer@gmail.com>
Co-authored-by: Cezary Olborski <cezary.olborski@gmail.com>

* generate TransferProofs on batch transfer

* clippy + zk versions

* feat: Treasury config pallet (#372)

* feat: Treasury config pallet

* fix: Taplo

---------

Co-authored-by: Ethan <tylercemer@gmail.com>
Co-authored-by: illuzen <illuzen@users.noreply.github.com>
Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>
Co-authored-by: Nikolaus Heger <nheger@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants