Skip to content

fix: skip value slot normalization for new account's deltas#2075

Merged
bobbinth merged 5 commits intomainfrom
pgackst-fix-new-account-delta
Nov 8, 2025
Merged

fix: skip value slot normalization for new account's deltas#2075
bobbinth merged 5 commits intomainfrom
pgackst-fix-new-account-delta

Conversation

@PhilippGackstatter
Copy link
Contributor

Skip value slot normalization for new account's deltas to avoid removing storage slots whose value is validly Word::empty.

This affects:

  • the StorageDeltaTracker to skip normalization when the account is new,
  • computing the in-kernel account delta commitment to always include value slots for new accounts, and essentially skip the initial_value != current_value comparison.

closes #2072

Copy link
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

LGTM ✅

exec.memory::get_init_nonce eq.0 or
# => [was_changed, INIT_VALUE', CURRENT_VALUE, slot_idx, RATE, RATE, PERM]

# only include in delta if the slot's value has changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should this comment be updated too?

Comment on lines +872 to +875
/// Tests that creating a new account with a slot whose value is empty is correctly included in the
/// delta and not normalized away.
#[tokio::test]
async fn delta_for_new_account_retains_empty_value_storage_slots() -> anyhow::Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested this on next by:

$ git restore -s origin/pgackst-fix-new-account-delta crates/miden-testing/src/kernel_tests/tx/test_account_delta.rs
$ cargo test delta_for_new_account_retains_empty_value_storage_slots

and it fails with the error reported in #2072.

👍🏼

Co-authored-by: Marti <marti@miden.team>
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

# more generally, we want the delta for a new account to include all its newly added values,
# regardless of the exact value, because the initial delta for an account must represent its
# full state
exec.memory::get_init_nonce eq.0 or
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR: but we use the check init_nonce == 0 in a few places now and I wonder if we should add something like memory::is_account_new procedure.

@bobbinth bobbinth merged commit 39eaf85 into main Nov 8, 2025
17 checks passed
@bobbinth bobbinth deleted the pgackst-fix-new-account-delta branch November 8, 2025 07:07
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.

3 participants