feat(standards): add the pass-through transaction script - #3713
feat(standards): add the pass-through transaction script#3713mmagician wants to merge 17 commits into
Conversation
The script forwards the assets of every input note into a single P2ID note addressed to the target named in its payload, leaving the account it runs on untouched. That lets a batch builder append a pass-through transaction to every batch it builds concurrently, sweeping the batch's TX_FEE notes into one note it collects out of band, instead of consuming the fees into its own account and forcing batches to be built serially (0xMiden/node#2501). The assets come from each input note's initial assets, since the note scripts have already emptied the notes by the time a transaction script runs. Sweeping every input note rather than a payload-supplied list is what makes the pass-through property hold by construction: the vault delta is zero, so the account commitment is unchanged. A note that does not deposit its assets fails the transaction on an underfunded vault. Closes part of #3709; skipping fee note creation on a fee-charging chain is left to a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
Applies the reviewer's doc suggestions to the MASM script and the same trimming to the Rust type and tests: drop the rationale that belongs in the PR description, keep the statements of what the code does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
A transaction script has no account context, so it cannot read the account's vault - which is why the first version derived what to move from each input note's initial assets, at a cost of one syscall per note plus one call per asset. Adds a `PassThrough` account component exposing the two steps that do need the account context, and rewrites the script around them: the payload now names the assets rather than the notes, `sweep_asset_to_note` moves the whole balance of each, and `assert_vault_unchanged` fails the transaction if anything is left behind. Cost is now independent of how many notes the transaction consumes - for a batch of fee notes in one asset, a single call instead of one per note. The assert also closes a hole in the previous version: on an account holding assets of its own, an input note that deposited and immediately re-emitted its assets made the sweep withdraw the account's own funds, and `NoAuth` accepted the resulting state change silently. That now fails the transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
…shape "Pass-through" names the property (the account's state does not change), not what the assets are forwarded into, and there are several plausible shapes: one P2ID for the whole transaction, one P2ID per input note, P2IDE, and so on. Makes `pass_through` a directory and names the script after its output, so the siblings have somewhere to land: - `tx_scripts/pass_through/single_p2id.masm`, with the payload loading pulled out into a `load_payload` helper. - `PassThroughSingleP2idTransactionScript`, keeping the shared `PassThroughTransactionScriptError`. - `tests/scripts/pass_through/single_p2id.rs`, with the account fixture shared from the module root. Also addresses review comments: bounds `MAX_ASSET_IDS` by `NoteAssets::MAX_NUM_ASSETS` so it cannot drift, uses `padw` over `push.0.0.0.0`, and trims the doc comments further. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
…ript The rename left `PassThrough`'s rustdoc pointing at the old type name, which fails `make doc` under RUSTDOCFLAGS="-D warnings". Switching the Rust `MAX_ASSET_IDS` to `NoteAssets::MAX_NUM_ASSETS` also let it float away from the MASM constant that actually guards the payload, so the constructor could hand back a script the VM rejects. Pins the two with a compile-time assertion and covers the upper bound end to end with a transaction forwarding the maximum number of assets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
…ript The previous commit inserted `MASM_MAX_ASSET_IDS` under the doc comment of the script path constant, so both ended up mislabelled. Also states what the compile-time assertion actually pins (a Rust-side change; the MASM side is covered by the tests), notes that the MASM bound duplicates a kernel-internal constant, and records that `@locals` takes a literal and must track `NUM_ASSET_IDS_LOC`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
Addresses review comments: shortens the module headers to what the modules are, drops the references to the Rust types from the MASM docs, and removes the commentary around the asset bound and the locals attribute. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
5e80485 to
a6fda68
Compare
Registers `PassThrough` in the MASM root-stability check, which is meant to cover every installable account component; without it a reordering of the component's exports would not be caught. Adds the two missing tests: that both procedure roots resolve, which forces the lazy `procedure_root!` lookup that otherwise only panics at first use, and that naming an asset the vault does not hold is a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
Records on the previous-release component list that a component added to the current one has to be mirrored there once a release contains it, otherwise its code commitment is compared against nothing. Also pins the component's export count and that the two accessors resolve to different roots, which membership alone would not catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
The note landed above the current list, where it was both mislabelled and self-referential. Both lists open identically, which is how it got there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
`sweep_asset_to_note` reads the balance itself, unlike `move_asset_to_note`, which makes the caller name the amount. On a general-purpose account that hands every note script the account consumes an unconditional drain, so say where the component may be installed. Also gives the tests a serial number with distinct elements: a uniform one would not catch the payload's serial-number word being read in the wrong order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
…t asset `sweep_asset_to_note` reads the balance itself, so as an account procedure it was an unconditional full-balance drain: any note script the account consumes could call it and move out a pre-existing balance, which `move_asset_to_note` cannot do since it makes the caller name the amount. Asserting the initial balance is empty bounds it to what the transaction itself deposited, which is the precondition the docs stated. It also turns the "a pre-existing balance is swept out along with the deposits" failure mode from silent into a failed transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
The previous commit read as though the assert closed the note-script drain concern. It bounds the sweep to what the transaction deposited, not to who moves it: any note script the account consumes can still redirect what the transaction's other notes deposited. Restores the installation restriction and drops the stale all-assets wording the per-asset assert superseded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
The security note pointed at "a component providing authentication" as the mitigation, while the component a pass-through account uses authenticates nobody - so a third party needs no note script at all to redirect assets in transit. States that, and that assets are only safe if the input note's own script constrains its destination. Also propagates to the script: a successful transaction does not imply the named assets reached the target, since a note script consumed by the same transaction can sweep them first and the vault ends empty either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
…case The rule as written was broken by every shipped example: TX_FEE notes constrain nothing about their destination. They are safe here because their assets were already claimable by any account, so routing them through takes nothing away - which is exactly what a destination-restricted note would lose. Says so, rather than leaving a reader to conclude the rule is advisory. Also fixes a reference to a procedure name that does not exist and the auth sentence that read as advice to install real authentication, which would defeat the property the component exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
`@locals` only takes a literal, so the frame size that bounds what `pipe_preimage_to_memory` writes was held by a comment while `MAX_ASSET_IDS` was pinned to the Rust side by a compile-time assert. Raising `NoteAssets::MAX_NUM_ASSETS` would break the Rust build and point a maintainer at bumping the MASM constant, which would widen the accepted payload past the frame and let it overwrite the loop state. Deriving the payload bounds from the frame size instead means the payload can never outgrow it, whichever constant someone reaches for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyebszMeyUrYBjsJBDLVwB
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks good overall, but I think the call to assert_vault_unchanged needs to be swapped with a different procedure, say assert_account_state_unchanged.
Currently the pass-through script does not enforce its core invariant. It checks only that the account vault root is unchanged, so an input note could call another installed account procedure which modifies storage. The assert_vault_unchanged check would then pass, even if the tx modified account state.
Since PassThrough can be combined with other components, I don’t think this should rely solely on whether the account vault is unchanged.
I think the final assertion check needs to compare the complete initial and current account commitments instead.
| # The number of field elements in a word. Replace with | ||
| # `miden::protocol::constants::WORD_NUM_ELEMENTS`, once | ||
| # https://github.com/0xMiden/miden-vm/issues/3429 is fixed. | ||
| const WORD_NUM_ELEMENTS = 4 |
There was a problem hiding this comment.
To remove this comment to the vm issue afaik we need to bump to VM version 0.29.2
| # The size of `forward_assets`'s local frame, which the payload is piped into. `@locals` only | ||
| # takes a literal, so the bounds below are derived from it rather than the other way round: that | ||
| # way the payload the length check accepts can never outgrow the frame reserved for it. |
There was a problem hiding this comment.
nit: can we simplify / make this comment clearer? maybe its just me who doesn't understand this comment very well :)
| # only what this transaction deposited may be moved out, so the account must have started | ||
| # without the asset |
There was a problem hiding this comment.
Nit: I'd move this comment wording to the procedure comment above or delete it since this is already mentioned in the procedure comment
| if.true | ||
| # the account holds none of the asset, so there is nothing to move | ||
| dropw dropw drop | ||
| # => [pad(16)] |
There was a problem hiding this comment.
Does nothing if the account holds none of the asset.
Is this desired? If you call sweep_asset_to_note and don't have the asset you're intending to move to the output note, should this not fail?
If you call this procedure now with an asset you don't have, the call to this procedure would succeed, but it would have no side effects.
| #! This is a pass-through transaction script: the state of the account it executes against does | ||
| #! not change. The input notes' scripts deposit their assets into the account's vault, and this tx |
There was a problem hiding this comment.
the state of the account it executes against does not change
Here the comment says that the account state does not change, but the script only enforces that the vault root is unchanged via assert_vault_unchanged.
If the account exposes another state mutating procedure, an input note could modify storage while leaving the vault unchanged; NoAuth would then increment the nonce, changing the account commitment.
Is this intended?
Adds the canonical
pass_throughtx script and account component. First part of #3709The idea is that the batch builder:
TX_FEEnote as inputs (which all move their assets into the executor's vault),single_p2id.masmtx script to call into the account'ssweep_asset_to_noteprocedure...P2IDnote for the batch builderNote: The reason why we have
single_p2id.masmunder a pass-through directory, rather than as a standalone file, is that there may be multiple variants of pass-through scripts in the future, e.g.P2IDnoteP2IDnote (this PR)P2IDEnote, etc.cc @Mirko-von-Leipzig