Skip to content

A failed Shield proof is refused free although its payer is already proven #5181

Description

@shumkov

Shield is the only transition in the pipeline that proves who would pay, then refuses free when an expensive check fails. Its two siblings charge; the wider codebase charges; and the reason it does not appears to be an omission rather than a decision, since no comment gives one.

The house rule, in the codebase's own words

The rule is not cheap-versus-expensive and it is not whether collateral is available. It is authorization: a transition is refused free until the pipeline has proved that the party who would be charged actually authorized it. Once that is proved, failures are paid, because the validation work has already been done. processor/v0/mod.rs:76-82, on a bad identity signature:

If the signature is not valid… we do not have the user pay for the state transition. Since it is most likely not from them.

Paid tier, once authorization is established: IdentityCreateFromAddresses key-structure and proof-of-possession failures (identity_create_from_addresses/advanced_structure/v0/mod.rs:43-73, BumpAddressInputNoncesAction with named penalties); document batches from protocol version 12, whose comment says the bump exists "so the user pays for the validation work that already ran" (batch/transformer/v0/mod.rs:849-852); IdentityUpdate, which sits in both tiers and splits exactly on this line — advanced-structure failures assert PaidConsensusError, basic-structure failures assert UnpaidConsensusError.

Where Shield falls out of it

Its address witnesses are validated at step 3 of process_state_transition_v0 (processor/v0/mod.rs:88-100), and its address balances and nonces are loaded at step 4 (:104-119). The Orchard proof is verified at step 12 (:239-243). So by the time the proof fails, the pipeline has already proved the address owners signed this transition and holds their balances.

Every other free refusal after the authorization step has a reason, and Shield's does not:

step line why it is free
address witnesses 88-100 the witness is the authorization
address balances / nonces 104-119 replay, or cannot pay
identity nonces 122-140 replay
basic structure 143-160 "extremely cheap to process, because of this attacks are not likely"
identity minimum balance 165-180 cannot pay
addresses minimum balance 183-200 cannot pay
prefunded specialized balance 202-220 "nobody can be charged for the vote, so it is refused unpaid"
shielded minimum fee 228-235 pool-paid, payer unproven
shielded proof — Shield 239-243 no stated reason

The pool-paid spends — Unshield, ShieldedTransfer, ShieldedWithdrawal, IdentityCreateFromShieldedPool, IdentityTopUpFromShieldedPool and the token variants — look like a second instance and are not. They have no identity in state and no address inputs; their only payer is the pool's value_balance, and the only authorization to spend from a pool is the proof and its nullifiers. A failed proof there means there is no proven payer and nothing chargeable, so free is correct. Shield differs precisely because its payer is transparent addresses, proven independently of the proof, nine steps earlier.

What it costs today

A bad proof reaches a block only through a faulty or malicious proposer — honest nodes refuse it in CheckTx and prepare_proposal strips it. That is true of the siblings too, so this is not about letting bad proofs through. The difference is what happens when a proposer includes one anyway:

  • ShieldFromIdentity: BumpIdentityNonceAction + shielded_proof_verification_failure → PaidConsensusError → the block commits and the submitter pays.
  • ShieldFromAssetLock: PartiallyUseAssetLockAction + the same penalty → the block commits and the asset lock is burned.
  • Shield: errors only, no action → UnpaidConsensusError → process_proposal rejects the entire block (abci/handler/process_proposal.rs:404-410). Every validator performed the Halo 2 verification and then discarded the block containing it. Nobody pays.

The intent for the siblings is stated outright at processor/traits/shielded_proof.rs:140-161: "ShieldFromAssetLock is intentionally excluded… because a failed proof must penalize the asset lock… Moving it here would let attackers spam bad proofs without burning their asset lock." Shield sits in the included, unpaid set at line 151, with no comparable note.

Current behaviour is pinned by shield/tests.rs:2606-2627 (assert_refused_for_its_proof, asserting UnpaidConsensusError), against the siblings' tests asserting PaidConsensusError with the penalty covered.

Smallest correct fix

Mirror ShieldFromIdentity:

  • A new ShieldTransition transform_into_action v1, selected only by the unreleased protocol version's validation table. This changes consensus — a block rejected today would commit — so it is a new generation, never an in-place edit.
  • Remove StateTransition::Shield(_) from has_shielded_proof_validation (processor/traits/shielded_proof.rs:151) in the new generation only, threading a check_tx_proof_verifier parameter through so CheckTx still refuses unpaid under the permit.
  • On failure, return BumpAddressInputNoncesAction with the transition's fee strategy and penalty_credits = shielded_proof_verification_failure.

One asymmetry to handle: Shield has no addresses_minimum_balance pre-check (addresses_minimum_balance.rs:119,128 → false), so unlike ShieldFromIdentity — whose transform relies on the balance pre-check guaranteeing the identity can cover it — nothing guarantees the inputs cover the penalty. ShieldFromAssetLock already solves that shape with saturating_sub and a min against what remains (shield_from_asset_lock/transform_into_action/v1/mod.rs:365-368); the same clamp applies.

Sequencing — this cannot be written independently

#5014 (fix(drive-abci)!: record and check the nullifiers of shielding transitions (PV14)) already adds a Shield transform_into_action v1 in the DRIVE_ABCI_VALIDATION_VERSIONS_V10 slot. It is about recording the dummy-spend nullifiers that outputs-only bundles reveal, not about the penalty, but it occupies the generation this fix needs. The two have to be sequenced or combined. Nothing else open touches shield penalties, proof-verification failure or address nonce bumps.

Found while auditing #4760, which does not touch Shield.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions