Resubmit the proposal's inner call on multisig execute - #145
Conversation
Regenerates the metadata and subxt bindings against the runtime from chain PR #675 and passes the proposal's stored call to `multisig.execute`, so the signer signs the call that dispatches instead of an opaque proposal id. Cold wallets get this for free: `submit_transaction` already routes them through `sign_and_submit_cold`, and the payload it builds now carries the inner call. Unlike `approve`, execute takes a `Box<RuntimeCall>` — inline, no length prefix — so the stored bytes are decoded back into a call rather than passed through. `decode_proposal_call` rejects bytes this build cannot decode instead of submitting a call the chain would reject, and `handle_approve` now shares the proposal fetch with it. Codegen derives codec's Decode/Encode for the call surface, which that decode needs. The regenerated metadata also trims the runtime surface: - pallet-recovery is gone: the `recovery` command, its exercise phase, its example scripts and its README section go with it. - Treasury `set_treasury_portion` and the `TreasuryPortion` storage item are gone, so `tech-referenda submit-treasury-portion` goes and `treasury info` drops the portion line. The governance exercise re-sets the treasury account to its current value instead, so it still enacts a real root call. - Balances `force_*`/`upgrade_accounts` and every Utility call but `batch_all` are gone. The utility phase now covers both halves of `batch_all`: every item applies on success, none apply when one fails. - `ReversibleTransfers::GuardianIndex` is gone. `fetch_entrusted_accounts` replaces it by iterating `HighSecurityAccounts` and keeping the entries naming the guardian; `high-security` and `wallet list` now share it. Tests: the stored bytes round-trip through decode/encode; undecodable and trailing bytes are rejected; execute is asserted to encode its inner call inline; and the cold-signing payload is asserted to carry the executed call, so the hardware flow shows it.
A call a signer cannot review is one they cannot meaningfully approve, so bound what propose and execute will put in front of one. Sized by the largest call we ever expect to put through a multisig: a `batch_all` of 32 transfers (double the 16 a batch is expected to carry). At the worst-case encoding of every field that is 1667 bytes, or 1707 inside a multisig wrapper, so 2 KiB leaves headroom — asserted against the constant at compile time. The chain's own `MaxCallSize` is 10 KiB; this is deliberately tighter. The five propose call sites now share one `build_propose_tx` that enforces it, and `decode_proposal_call` enforces it on the approve and execute paths.
…er limit A client limit below the chain's refuses proposals the chain accepts, leaving a multisig no signer can act on. Use the chain's number: 10 KiB, from `pallet_multisig::Config::MaxCallSize`.
Builds a signing request per supported call, encodes it through the bundled chain metadata, wraps it in the wallet envelope, UR-encodes it and writes the QR frames as SVG plus a viewer page. The output is committed in quantus-apps under cold-wallet-app/test/fixtures/qr and is parsed by tests in both wallets. SVG rather than PNG so the corpus needs no image toolchain, and horizontal runs are merged into one path so a frame is about 5 KB instead of 100 KB.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
Verdict (advisory): Request changes
Blocking finding:
- [P2] Require the rollback exercise to observe the intended on-chain failure (
src/cli/exercise/scenarios/utility.rs:79).submit_ok(...).awaitcan fail becausebatch_allreached the deliberately failing second transfer, but it can also fail because of an RPC error, pool rejection, metadata mismatch, or failure to include the extrinsic. Discarding every error leaves the fresh recipient at balance zero, so the next check reports a successful rollback even when the batch never ran. Validate a specific expected dispatch failure (and therefore inclusion) before using the unchanged balance as rollback evidence.
Validation on exact head 65d40ac6c9bb3c3ab422ed3f26ce828e6106cbe4:
git diff --check 6245cd0...65d40ac: passed.cargo test --locked --lib: 308 passed.SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings: passed.cargo run --locked --example generate_qr_fixtures -- <temp-dir>: generated 9 cases / 60 files; the output matched the referenced quantus-apps fixture corpus byte-for-byte.- GitHub CI at review time: format, dependency cooldown, Clippy/docs, and security audit passed; Ubuntu/macOS build-test jobs and examples were still running.
…chain The rollback step discarded the submit error. An RPC error or a pool rejection also leaves the recipient at zero, so the step reported a successful rollback even when the batch never ran. It now uses submit_expect_failure and requires the insufficient-funds dispatch error, which only comes from the ExtrinsicFailed event and so proves the extrinsic was included. The expected error fragments move to a shared constant, reused by the negative over-balance step.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
Verdict (advisory): Request changes
Blocking finding:
- [P1] Regenerate the bindings for the released spec-148 runtime (
src/quantus_metadata.scale;examples/generate_qr_fixtures.rs:20). This head bundles spec 147 / transaction 6 metadata, while the current chain release, v0.11.1, reports spec 148 / transaction 6. The CLI accepts that node with only its newer-runtime warning, but a utility-only exercise against a fresh v0.11.1 dev node fails during fee estimation withMetadata(IncompatibleCodegen). Spec 148 addsOriginsto the recursiveOriginCaller/RuntimeCallshape; consequently the changedmultisig.executepayload, which now contains a generatedRuntimeCall, fails the same static Subxt validation on the current release. Please regenerate the metadata and bindings against spec 148 with the existing recursive codec derives, add(148, 6)to the compatible runtime list, update the fixture spec version/corpus as needed, and rerun the live exercise.
The previous rollback-exercise blocker is fixed at this head: the scenario now requires the intended insufficient-funds dispatch failure before checking the rolled-back balance.
Validation on exact head 1ee52ef9bd48890ec4389e88c6cd2dcca8b164e1:
git diff --check, pinned nightly formatting, Taplo formatting, andSKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings: passed.cargo test --locked --lib: 308 passed after generating the circuit binaries.- Regenerating
quantus_subxt.rsfrom the bundled metadata reproduced the committed file exactly. - The QR generator produced 9 cases / 60 files matching the shared quantus-apps fixture corpus byte-for-byte.
- Live exercise against chain v0.11.1 (spec 148 / transaction 6): failed at static payload validation with
Metadata(IncompatibleCodegen). - GitHub CI: all eight current checks passed.
The chain binds on-chain signatures to a FIPS 204 ML-DSA context (chain commit 85296439). ML-DSA sign and verify always took a ctx argument, and the published qp-dilithium-crypto still passes None, so sp_core::Pair::sign produces a signature the node rejects with "Transaction has a bad signature". Reproduced against a live v0.11.1 dev node before this change. New src/chain/signing.rs signs and verifies under that context, going straight to qp-rusty-crystals-dilithium, which does expose ctx. Every extrinsic signature the CLI makes or checks goes through it: the three subxt Signer impls, the cold-sign-sim device simulator, and the QR response check. litep2p node identity is not involved and is unchanged. Also regenerates the bindings for the released spec-148 runtime. Spec 148 adds Origins to the recursive OriginCaller and RuntimeCall shape, so the spec-147 bindings failed static subxt validation on v0.11.1, including the multisig.execute payload that now carries a RuntimeCall. (148, 6) joins COMPATIBLE_RUNTIMES and the QR fixture generator moves to spec 148. The batch_all rollback exercise asserted an insufficient-funds error but used an amount near u128::MAX, which underflows the balance arithmetic first. It now sends twice the sender's balance, so the failure is the one the step asserts. 49 of 50 exercise steps passed against a live v0.11.1 node before the rollback amount fix; the utility phase is 5 of 5 after it.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
Verdict (advisory): Request changes
Blocking finding:
- [P1] Preserve the signature-context boundary for pre-148 runtimes (
src/chain/signing.rs:21-30;src/config/mod.rs:19-30). This head signs every hot-wallet and simulated-cold-wallet extrinsic withSome(b"QUANTUS_EXTRINSIC"), butQuantusClient::newstill accepts specs 134–147 as compatible. Those runtimes use the previous empty-context verifier (for example, chain v0.10.0/spec 145 calls ML-DSA verification withNone), and FIPS 204 contexts are domain-separated, so they reject these signatures asTransaction has a bad signature. Make signing and cold-response verification select the context from the connected runtime, or remove/reject all pre-context runtime pairs before any transaction can be signed. Please add a regression test covering both an older accepted pair and spec 148.
The previous spec-148 metadata and utility rollback blockers are fixed at this head. I found no additional blocking issue in the multisig call reconstruction, runtime-surface removals, or regenerated metadata.
Validation on exact head 4e6fbb84cf7dc1588410d7d13687cae76134e433:
git diff --check ecc1f363...4e6fbb84, pinned nightly Rust formatting, and Taplo formatting: passed.cargo test --locked --lib: 311 passed.SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings: passed.- Regenerating
quantus_subxt.rsfrom the bundled metadata with the recursive codec derives reproduced the committed file exactly. - Chain history verification: the pre-context spec-147 source and released v0.10.0/spec 145 verifier use no ML-DSA context; v0.11.1/spec 148 uses
QUANTUS_EXTRINSIC. - GitHub CI at review time: format, Ubuntu build/test, Clippy/docs, security audit, and dependency checks passed; macOS build/test and examples were still running.
Signing every extrinsic under QUANTUS_EXTRINSIC broke the pre-148 runtimes the CLI still accepts. FIPS 204 contexts are domain separated, so specs 134 to 147, which verify with no context at all, reject a contextful signature exactly as spec 148 rejects a contextless one. COMPATIBLE_RUNTIMES gains binds_signing_context, false up to spec 147 and true from 148. The context is now a property of the connected runtime rather than a constant: - context_for_runtime maps a runtime version to its context, and assumes newer unlisted specs keep it. - QuantusClient::signing_context reads the runtime version subxt already cached at connect, so it costs no RPC. - QuantusSigner carries the context, because it is not a property of the key: the same wallet signs both ways depending on the chain. - Cold response verification uses the connected runtime's context. The cold-sign simulator stays on QUANTUS_EXTRINSIC, which is what Keystone firmware does. Drops the two bare-pair subxt Signer impls. Nothing used them and they had nowhere to carry a context. Tests sign for spec 147 and spec 148 and assert each verifies under its own context and fails across the boundary, and pin the pre-148 path byte-for-byte against sp_core::Pair::sign. 315 lib tests pass. 25 of 25 exercise steps pass against a live v0.11.1 node, including both multisig steps and both ML-DSA schemes.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT Sol
Verdict (advisory): Approve
No blocking findings. The previous signing-context blocker is fixed at this head: the connected runtime now selects the empty context for the listed pre-148 runtimes and QUANTUS_EXTRINSIC for spec 148 and newer, and that selection reaches both hot-wallet signing and cold-response verification. The regression coverage pins both sides of the boundary and rejects cross-context signatures.
Non-blocking documentation follow-up:
README.md:545andsrc/cli/treasury.rs:3,29,39still describe the treasury as receiving a portion of mining rewards, while this PR removesTreasuryPortionand the runtime now funds the treasury independently of an ongoing reward split. Updating that wording would keep the CLI documentation aligned with the new runtime surface.
Validation on exact head cd9a0642cf58febf4d8e8978e30d84f74b0a5255:
git diff --check ecc1f363...cd9a064, pinned nightly formatting, and Taplo formatting: passed.cargo test --locked: 315 library tests and 313 binary tests passed; one generated-binding doctest was ignored.SKIP_CIRCUIT_BUILD=1 cargo clippy --all-targets --locked -- -D warnings: passed.- Regenerating
quantus_subxt.rsfrom the bundled metadata with the recursive codec derives reproduced the committed file exactly. generate_qr_fixturesproduced the expected spec-148 / transaction-6 corpus: 9 cases and 60 files.- Live read-only compatibility check against Planck (spec 144 / transaction 3): passed.
- GitHub CI: all eight current checks passed, including Ubuntu/macOS build-test, Clippy/docs, examples, and security audit.
Updates the CLI for the runtime in chain#675.
Goes with keystone3-firmware #19 and quantus-apps #622 and #623.
What changed on chain
multisig.executenow takes the call it dispatches, and the chain only dispatches it if it re-encodes to the stored proposal bytes:handle_executereads the proposal and resubmits those exact bytes.Both flows come from one change.
submit_transactionalready routes a cold wallet throughsign_and_submit_cold, so the payload the Keystone shows now carries the executed call. A test asserts that directly on the raw signer payload.One catch.
approvetakesBoundedVec<u8>, which is length prefixed.executetakesBox<RuntimeCall>, which is inline with no prefix. So the stored bytes get decoded back into a call rather than passed through.decode_proposal_callfails on bytes this build cannot decode, which beats submitting a call the chain rejects.handle_approvenow shares its proposal fetch. Codegen gainedcodec::DecodeandEncodederives for the call surface, recorded inregenerate_metadata.shso future regens keep them.Call size limit
Propose and execute refuse a call over 10 KiB, which is the chain's own
MaxCallSize. Using the chain's number matters. Anything smaller would refuse proposals the chain accepts, and then no signer could act on them. The five propose call sites now share onebuild_propose_txthat enforces it.Runtime surface changes
Regenerating is not only the execute field. The new runtime removes a lot:
pallet-recoveryrecoverycommand, its exercise phase,scripts/recovery_examples.shand the README sectionset_treasury_portionandTreasuryPortionstoragetech-referenda submit-treasury-portion, dropped the portion line fromtreasury infoforce_*andupgrade_accountsupgrade_accountsexercise stepbatch_allReversibleTransfers::GuardianIndexUtility phase. It only covered removed calls. It now covers both halves of what
batch_allpromises: every item applies on success, and none apply when a later item fails.fuzzmoves frombatchtobatch_all.Governance exercise. It used
set_treasury_portionas its referendum payload. It now re-sets the treasury account to its current value. Still a real root call enacted through a referendum, without changing state the exercise does not own.GuardianIndex. The guardian to accounts reverse index is gone.
fetch_entrusted_accountsreplaces it by iteratingHighSecurityAccountsand keeping the entries naming the guardian.high-security entrustedandwallet listhad separate lookups before and now share it.spec_version147 andtransaction_version6 were already inCOMPATIBLE_RUNTIMES.QR fixture generator
New
examples/generate_qr_fixtures.rs. It builds a signing request per supported call, encodes it through the bundled chain metadata, wraps it in the wallet envelope, UR-encodes it, and writes the QR frames as SVG plus a viewer page.The output is committed in quantus-apps under
cold-wallet-app/test/fixtures/qrand is parsed by tests in both wallets, so all three parsers are checked against the same bytes.SVG rather than PNG so the corpus needs no image toolchain and works offline. Horizontal runs are merged into one path, which makes a frame about 5 KB instead of 100 KB.
Tests
decode_proposal_callround-trips the stored bytes through decode and encode, which is the byte equality the chain checks. It rejects undecodable bytes, trailing bytes and oversized calls.executeis asserted to encode its inner call inline.test_cold_payload_carries_the_executed_callwalks the raw signer payload the Keystone receives and asserts the inner call sits inline after the proposal id.cargo test --libpasses, 308 tests../clippy.shis clean.Runtime 148 and the signing context
The chain released v0.11.1 while this PR was open. Two things broke against it, and both are fixed here.
Bindings were a runtime behind. This branch bundled spec 147 metadata. v0.11.1 reports spec 148 / transaction 6, and spec 148 adds
Originsto the recursiveOriginCaller/RuntimeCallshape. Static subxt validation then failed withMetadata(IncompatibleCodegen), which hit fee estimation on any call and the newmultisig.executepayload in particular, since it now carries a generatedRuntimeCall.src/quantus_metadata.scaleandsrc/chain/quantus_subxt.rsare regenerated from a live v0.11.1 dev node throughregenerate_metadata.sh, so the recursivecodec::Decode/Encodederives are preserved.(148, 6)joinsCOMPATIBLE_RUNTIMES.Extrinsic signatures now need a FIPS 204 context. Chain commit
85296439binds on-chain signatures to the context stringQUANTUS_EXTRINSIC. ML-DSAsign/verifyalways took actxargument; the publishedqp-dilithium-cryptostill passesNone, sosp_core::Pair::signproduces a signature the node rejects with "Transaction has a bad signature". I reproduced that against a live v0.11.1 node before fixing it.New
src/chain/signing.rssigns and verifies under that context, going straight toqp-rusty-crystals-dilithium, which does exposectx. Every extrinsic signature the CLI makes or checks goes through it: the three subxtSignerimpls, thecold-sign-simdevice simulator, and the QR response check invalidate_signature_response. Nothing else changes signing paths, and litep2p node identity is not involved.mainis merged in for the dilithium 4.1.1 and zk-circuits 4.3.0 bumps, which the v0.11.1 chain is built against.The QR fixture generator moves to
SPEC_VERSION = 148. The committed corpus in quantus-apps is not regenerated here: that repo pinsbundledSpecVersion = 147and its test asserts the manifest matches, so the corpus has to move with the app's own metadata bump. That is tracked separately, along with the same signing-context change for the Dart SDK, the cold wallet and the mobile wallet.Tests
src/chain/signing.rscovers the round trip for ML-DSA-87 and ML-DSA-65 and asserts that an empty-context signature does not verify as an extrinsic signature, which is exactly what the node rejects.Live exercise against chain v0.11.1
A full run against a v0.11.1 dev node (spec 148 / tx 6) passed 49 of 50 steps, including
[multisig] lifecycle, which creates a 2-of-3 multisig, approves and executes a proposal, and verifies the payout. That is the flow this PR changes. The one failure was this PR's own new rollback assertion: it expected an insufficient-funds error but sent an amount nearu128::MAX, which underflows the balance arithmetic first and returnsArithmetic(Underflow). The step now sends twice the sender's balance and getsToken(FundsUnavailable). The utility phase re-ran 5 of 5 after the fix.Before the signing-context fix, every extrinsic was rejected at the pool with "Transaction has a bad signature" and the run could not get past funding the ephemeral accounts.
Follow-ups in other repos
The same context change is needed wherever else a Quantus extrinsic is signed. Both are open:
rust/apps/quantus/src/lib.rssigns withctx = None, so the device's signatures are rejected the same way.