feat(lee)!: incremental updates - wire programs to diff-native execution - #791
feat(lee)!: incremental updates - wire programs to diff-native execution#791jonesmarvin8 wants to merge 14 commits into
Conversation
717d8f5 to
f4f9b81
Compare
There was a problem hiding this comment.
not introduced in this pr, but I think AccountDiff is a misnomer. the data in it is not a diff, only the balance. Maybe PostStateOutcome or PostStateEffects with balance_diff and new_data fields
There was a problem hiding this comment.
I agree. The names are weird given thee limited scope we're going for now.
AccountDiff -> PostStateEffects
AccountDiffOutput -> PostStateEffectsOutput
diff_data -> new_data
…view) AccountDiff's diff_data field was never actually a diff, just a full replacement value, making the type name misleading (schouhy's review on PR #791). Renames AccountDiff -> PostStateEffects, AccountDiffOutput -> PostStateEffectsOutput, and diff_data -> new_data across every guest program, lee_core, and the sequencer/circuit host code that consumes them. No behavior change.
| /// The account post states the program execution produced. | ||
| pub post_states: Vec<AccountPostState>, | ||
| /// The account diffs the program execution produced. | ||
| pub post_diffs: Vec<PostStateEffectsOutput>, |
There was a problem hiding this comment.
This renames the guest-facing output type and field (and read_lee_inputs to read_lee_call), which breaks every guest in lez-programs and anything on lez-framework's #[lez_program] macro, but no commit carries ! or a BREAKING CHANGE: footer; can those be added?
There was a problem hiding this comment.
Title and body flag it now, but no branch commit carries ! or the footer, and this repo merges with merge commits so only branch commits land on dev (compare 4bda70f): could the rename commit carry the footer?
Addresses moudyellaz's PR #791 review comment: PostStateEffects.id was never compared against the positionally-paired pre_state's account_id, in validate_execution or either materializer. With ModifiedNonce/ModifiedProgramOwner gone (PostStateEffects carries neither), nothing detected a pre_states/post_diffs misalignment - a program with a reordering bug would have its diffs silently applied to the wrong accounts, passing every existing aggregate check. Adds ExecutionValidationError::MismatchedPostStateId and a check for it as the first rule in validate_execution's per-account loop, ahead of the authorization/balance rules that already implicitly assume correct pairing. Since both materializers call validate_execution before ever applying a diff, one check covers both call sites. Adds a new guest, reordering_transfer - identical to simple_balance_transfer except it reports its two diffs in the opposite order from pre_states, with both diffs' id fields honestly naming the correct account - modeling an honest reordering bug rather than a forged id. New end-to-end test reordered_post_diffs_are_rejected proves the fix over the whole transition_from_public_transaction pipeline and that state is left untouched. Also adds insufficient_balance_transfer_leaves_state_untouched (moudyellaz's other open comment): the two new validate_execution unit tests added for erhant's earlier balance-sufficiency fix only exercised validate_execution as a pure function, dropping the old malicious_program_cannot_break_balance_validation_if_not_in_genesis test's end-to-end assertion that a rejected transaction leaves real state completely untouched. The old test's specific exploit (fabricating extra post-states to defeat total-balance conservation) is already closed by validate_execution's length check, so no malicious program is needed to reconstruct the property - an ordinary transfer requesting more than the sender holds is enough.
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, BorshSerialize, BorshDeserialize)] | ||
| pub struct AccountDiff { | ||
| pub struct PostStateEffects { |
There was a problem hiding this comment.
why does the post state effects need an id? isnt the id inferred by the position in the post state vec and the same position in the pre state?
There was a problem hiding this comment.
This is a hold over for incremental updates. PostStateEffects.id guarantees that the wrong account PostStateEffects cannot be passed for a different Account.
This is worth keeping since we're still investigating incremental updates, and we want to avoid breaking changes on the program shape.
There was a problem hiding this comment.
I think it's better to have the circuit produce this pairing between effects and ids instead of the program itself
| #[cfg_attr(any(feature = "host", test), derive(PartialEq, Eq))] | ||
| pub struct AccountDiffOutput { | ||
| diff: AccountDiff, | ||
| pub struct PostStateEffectsOutput { |
There was a problem hiding this comment.
The naming does not make much sense to me. PostStateEffectOutput is a wrapper around PostStateEffect with a claiming flag. Why Output? Maybe better PostStateEffectWithClaiming. But that's a mouthful. Why not just move the claim field inside the PostStateEffect struct and avoid the mamushka?
There was a problem hiding this comment.
I agree that right now PostStateEffects with claiming makes sense due to the limited usage of PostStateEffects.
Ultimately, we want PostStateEffects to be used with incremental updates. This means that the updates will "live" longer than claiming. E.g., sequencer receives the PostStateEffects for each public account used in a privacy transaction. To this end, the claim field would be awkward to keep passing.
We can rename PostStateEffectsWithClaiming if you really want...
There was a problem hiding this comment.
no, I'm not convinced about that naming either. It's just that by looking at the name I get no information of what that struct is with PostStateEffectsOutput. I'd put Output to a struct that's the result of a process, but in this case that's already in the Effects part
Addresses two review comments from schouhy: - PostStateEffects::unchanged -> new_unchanged - PostStateEffectsOutput.diff field/accessors -> effects, for consistency with the renamed constructor
moudyellaz
left a comment
There was a problem hiding this comment.
lgtm, thanks! Left one nit
f861282 to
d3519b3
Compare
…ild artifacts h2 0.4.14 had an unbounded empty DATA frames flaw (low severity); bumped to 0.4.18 via cargo update -p h2. cargo deny check now passes clean (advisories/bans/licenses/sources all ok). Rebuild every guest artifact and the prebuilt test fixture to match the dependency bump.
…view) AccountDiff's diff_data field was never actually a diff, just a full replacement value, making the type name misleading (schouhy's review on PR #791). Renames AccountDiff -> PostStateEffects, AccountDiffOutput -> PostStateEffectsOutput, and diff_data -> new_data across every guest program, lee_core, and the sequencer/circuit host code that consumes them. No behavior change.
Addresses moudyellaz's PR #791 review comment: PostStateEffects.id was never compared against the positionally-paired pre_state's account_id, in validate_execution or either materializer. With ModifiedNonce/ModifiedProgramOwner gone (PostStateEffects carries neither), nothing detected a pre_states/post_diffs misalignment - a program with a reordering bug would have its diffs silently applied to the wrong accounts, passing every existing aggregate check. Adds ExecutionValidationError::MismatchedPostStateId and a check for it as the first rule in validate_execution's per-account loop, ahead of the authorization/balance rules that already implicitly assume correct pairing. Since both materializers call validate_execution before ever applying a diff, one check covers both call sites. Adds a new guest, reordering_transfer - identical to simple_balance_transfer except it reports its two diffs in the opposite order from pre_states, with both diffs' id fields honestly naming the correct account - modeling an honest reordering bug rather than a forged id. New end-to-end test reordered_post_diffs_are_rejected proves the fix over the whole transition_from_public_transaction pipeline and that state is left untouched. Also adds insufficient_balance_transfer_leaves_state_untouched (moudyellaz's other open comment): the two new validate_execution unit tests added for erhant's earlier balance-sufficiency fix only exercised validate_execution as a pure function, dropping the old malicious_program_cannot_break_balance_validation_if_not_in_genesis test's end-to-end assertion that a rejected transaction leaves real state completely untouched. The old test's specific exploit (fabricating extra post-states to defeat total-balance conservation) is already closed by validate_execution's length check, so no malicious program is needed to reconstruct the property - an ordinary transfer requesting more than the sender holds is enough.
4ecfd03 to
190a437
Compare
Addresses two review comments from schouhy: - PostStateEffects::unchanged -> new_unchanged - PostStateEffectsOutput.diff field/accessors -> effects, for consistency with the renamed constructor
…ccountPostState Second PR for incremental updates, scoped down from PR #757. Programs now report an AccountDiff (diff_balance + optional diff_data) per account instead of a fully materialized post-state; the sequencer (public path, validated_state_diff) and the privacy-preserving circuit (private path, execution_state) apply apply_balance_diff for the balance and copy diff_data straight into the new account.data when present, rather than PR #757's approach of re-invoking the owning program's own update_from_diff entrypoint to materialize it. validate_execution drops the ModifiedNonce/ModifiedProgramOwner/ NonDefaultAccountWithDefaultOwner checks: AccountDiff has no nonce or program_owner field, so a program can no longer express those changes at all, and ownership is exclusively governed by the claim mechanism. Total balance conservation is now checked as sum(Add) == sum(Sub) across a call's diffs rather than comparing pre/post totals. Every guest program (lez/programs, test_methods, test_programs, examples) is rewired to the new AccountDiffOutput/AccountDiff API and to read_lee_call's CallKind-prefixed frame. nonce_changer, program_owner_changer, and modified_transfer are removed along with their negative tests, since the class of bug they exercised is now structurally impossible. Guest-side balance-sufficiency checks (checked_add/checked_sub) are dropped in favor of relying on apply_balance_diff's single, centralized checked arithmetic at materialization time. BREAKING CHANGE: every guest program now reports account changes via an AccountDiffOutput carrying an AccountDiff (diff_balance + optional diff_data) read through read_lee_call's CallKind-prefixed frame, instead of a materialized AccountPostState read through read_lee_inputs; any guest in lez-programs or built on lez-framework's #[lez_program] macro must be updated to the new API.
…balance-error test Converts AccountWithMetadata params to references where the function only reads fields (never consumes them) — clippy's needless_pass_by_value was firing across amm, token, and one examples guest program after the diff-native rewiring dropped the full-Account moves these functions used to need. Also trims several multi-line comments introduced by the diff-native migration down to one line, and removes one that only restated the code below it. sequencer_core's transaction_pre_check_native_transfer_sent_too_much expected an over-large transfer to fail via a guest-side panic (ProgramExecutionFailed): balance sufficiency is no longer checked in-guest, so this now correctly fails via apply_balance_diff's centralized check (BalanceDiffFailed) instead. Also rebuilds every guest artifact and the prebuilt sequencer fixture against this branch's current source.
Addresses erhant's review comment on PR 749 asking whether read_lee_inputs could be removed once nothing called it directly. That wasn't yet possible there (guests hadn't been switched over), but on this branch every guest now goes through read_lee_call, so read_lee_inputs had exactly one caller left: read_lee_call itself. Inlines it rather than leaving a single-caller function around. Rebuilds every guest artifact and the prebuilt sequencer fixture against this branch's current source.
Rebased onto marvin/incremental-updates-1, which renamed AccountDiffOutput::required_claim() to claim() per review feedback. Updates this branch's own call sites (added during the diff-native program rewiring, after that rename point) to match. Rebuilds every guest artifact and the prebuilt sequencer fixture.
…post-rebase fallout Rebased onto marvin/incremental-updates-1's diff_balance: Option<BalanceDiff> change. Replaces every literal diff_balance: BalanceDiff::Add(0) placeholder (the old way to express "no balance change") with None across guest programs, wraps genuine non-zero diffs in Some(...), and drops now-unused BalanceDiff imports. Also fixes a clippy arbitrary_source_item_ordering violation this branch introduced in token/src/tests.rs, and rebuilds artifacts (required: the borsh wire format for AccountDiff changed).
The field holds AccountDiffOutput, not materialized post-account states; post_diffs matches what it actually carries.
…view) AccountDiff's diff_data field was never actually a diff, just a full replacement value, making the type name misleading (schouhy's review on PR #791). Renames AccountDiff -> PostStateEffects, AccountDiffOutput -> PostStateEffectsOutput, and diff_data -> new_data across every guest program, lee_core, and the sequencer/circuit host code that consumes them. No behavior change.
…date_execution validate_execution only checked global sum conservation (total Add == total Sub) and ownership of decreases, not that any individual account's diff was valid against its own pre-state balance. A program could Sub more than one account held while crediting the difference elsewhere, passing the sum check; apply_balance_diff would then return InsufficientBalance downstream, which the privacy circuit's execution_state.rs turned into a panic via .expect(). Sum conservation and per-account feasibility are orthogonal checks (double-entry bookkeeping: individual line validity vs. debits equal credits) - neither implies the other, so this adds the missing one as a new step, backed by a new ExecutionValidationError::InvalidBalanceDiff variant. Updates two existing tests whose expected error variant moved earlier in the pipeline as a result, adds direct coverage in validate_execution's own tests, and rebuilds artifacts.
Addresses moudyellaz's PR #791 review comment: PostStateEffects.id was never compared against the positionally-paired pre_state's account_id, in validate_execution or either materializer. With ModifiedNonce/ModifiedProgramOwner gone (PostStateEffects carries neither), nothing detected a pre_states/post_diffs misalignment - a program with a reordering bug would have its diffs silently applied to the wrong accounts, passing every existing aggregate check. Adds ExecutionValidationError::MismatchedPostStateId and a check for it as the first rule in validate_execution's per-account loop, ahead of the authorization/balance rules that already implicitly assume correct pairing. Since both materializers call validate_execution before ever applying a diff, one check covers both call sites. Adds a new guest, reordering_transfer - identical to simple_balance_transfer except it reports its two diffs in the opposite order from pre_states, with both diffs' id fields honestly naming the correct account - modeling an honest reordering bug rather than a forged id. New end-to-end test reordered_post_diffs_are_rejected proves the fix over the whole transition_from_public_transaction pipeline and that state is left untouched. Also adds insufficient_balance_transfer_leaves_state_untouched (moudyellaz's other open comment): the two new validate_execution unit tests added for erhant's earlier balance-sufficiency fix only exercised validate_execution as a pure function, dropping the old malicious_program_cannot_break_balance_validation_if_not_in_genesis test's end-to-end assertion that a rejected transaction leaves real state completely untouched. The old test's specific exploit (fabricating extra post-states to defeat total-balance conservation) is already closed by validate_execution's length check, so no malicious program is needed to reconstruct the property - an ordinary transfer requesting more than the sender holds is enough.
Addresses two review comments from schouhy: - PostStateEffects::unchanged -> new_unchanged - PostStateEffectsOutput.diff field/accessors -> effects, for consistency with the renamed constructor
Drops the now-dead modified_transfer_program accessor (its guest source was removed by the AccountDiff rewiring), removes an unused WrappedBalanceSum import, applies cargo +nightly fmt, and rebuilds every guest artifact plus the prebuilt sequencer fixture against the final rebased source.
…laiming Per PR #791 review discussion, PostStateEffectsWithClaiming is a clearer name for the wrapper carrying an optional Claim alongside a program's PostStateEffects. Renamed across lee_core, lee, and every guest program that constructs or destructures it. Also runs cargo +nightly fmt (re-wraps several use blocks) and rebuilds guest artifacts and the prebuilt sequencer fixture dump, since the rename touches guest source.
190a437 to
6a34013
Compare
BREAKING CHANGE: Changes output of LEZ programs to a list of `AccountStateDiff`. This struct pairs `pre_states` with `post_balance_diff`, `post_data` and `post_claim`.
🎯 Purpose
Second PR for incremental updates. This PR rewire programs to output
AccountDiffOutput(wrapper ofAccountDiffandclaim) rather thanAccountPostState. This change restricts what programs can concretely affect. These resulting diffs from a program's execution is immediately applied afterwards to produce theAccountPostState. This restricts the PR's updates to program wiring.Breaking change: The renames of
AccountDiff, etc as requested by @schouhy is a breaking change from the previous PR 749.⚙️ Approach
lez/programs,test_methods,test_programs,examples) to emitAccountDiffOutput/AccountDiffinstead ofAccountPostState, viaread_lee_call'sCallKind-prefixed frame.validate_executiondrops theModifiedNonce/ModifiedProgramOwner/NonDefaultAccountWithDefaultOwnerchecks —AccountDiffhas nononce/program_ownerfield, so a program can no longer express those changes at all, and ownership is governed exclusively by the claim mechanism. Total balance conservation is now checked assum(Add) == sum(Sub)across a call's diffs, rather than comparing pre/post totals.validated_state_diff) and privacy-preserving circuit (execution_state) both applyapply_balance_diffand copydiff_datato materialize each account's post-state.nonce_changer,program_owner_changer, andmodified_transfer(and their negative tests) — the class of bug they exercised is now structurally impossible.checked_add/checked_subwith panics);apply_balance_diff's single, centralized checked arithmetic now enforces this at materialization time.AccountDiffs to the circuit's own journal —PrivacyPreservingCircuitOutput'spublic_actions/private_actionsare untouched; diffs are strictly internal to how the circuit computes each account's post-state.🧪 How to Test
All previous tests should pass.
🔗 Dependencies
PR 749: introduces the core
AccountDiff/BalanceDiff/AccountDiffOutputtypes this PR wires in.🔜 Future Work
AccountPostState.📋 PR Completion Checklist