-
Notifications
You must be signed in to change notification settings - Fork 95
perf/store: optimize updating existing accounts #1567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
40db564 to
e305139
Compare
e305139 to
9a98291
Compare
14dc3a6 to
5f11a29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR optimizes applying partial account deltas by avoiding loading full Account state (code bytes, all storage-map entries, all vault assets) during upsert_accounts, instead fetching only minimal state and computing updated headers/roots.
Changes:
- Added an optimized delta-update module to query minimal account state and compute updated
storage_header/vault_root. - Updated
upsert_accountsto use anAccountStateForInsertenum to handle private, full-state, and partial-state inserts. - Added new tests covering the optimized delta path, private upserts, and full-state delta upserts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/store/src/db/models/queries/accounts/delta.rs | New optimized queries + helpers for partial delta application (minimal state + root/header computation). |
| crates/store/src/db/models/queries/accounts/delta/tests.rs | New tests for optimized partial delta update, private account upsert, and full-state delta upsert. |
| crates/store/src/db/models/queries/accounts.rs | Integrates optimized partial-delta path into upsert_accounts; adds AccountRowInsert constructors. |
| alt_approach.txt | Documents an alternative design (precompute roots in InnerForest). |
| CHANGELOG.md | Adds a changelog entry for the performance improvement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| storage_header: new_storage_header, | ||
| vault_root: new_vault_root, | ||
| }; | ||
|
|
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the partial-delta path, the code no longer verifies that the derived minimal state (nonce/storage/vault/code commitment) is consistent with update.final_state_commitment(). Previously this was enforced by applying the delta to a full Account and checking AccountCommitmentsMismatch. Without an equivalent check here, it’s possible to persist an accounts row whose commitment does not match its stored nonce/storage_header/vault_root fields (especially given the new ad-hoc SMT computations). Please add a commitment verification using the minimal fields (or otherwise ensure consistency) and fail the update if it doesn’t match.
| // Verify that the derived minimal state matches the expected final commitment. | |
| // This mirrors the full-state delta path where we build a full `Account` and | |
| // compare `account.commitment()` with `update.final_state_commitment()`. | |
| let calculated_commitment = Account::commitment_from_minimal_state( | |
| account_state.nonce, | |
| account_state.code_commitment, | |
| account_state.storage_header, | |
| account_state.vault_root, | |
| ); | |
| if calculated_commitment != update.final_state_commitment() { | |
| return Err(DatabaseError::AccountCommitmentsMismatch { | |
| calculated: calculated_commitment, | |
| expected: update.final_state_commitment(), | |
| }); | |
| } |
| /// Tests that the optimized delta update path produces the same results as the old | ||
| /// method that loads the full account. | ||
| /// | ||
| /// Covers partial deltas that update: | ||
| /// - Nonce (via `nonce_delta`) | ||
| /// - Value storage slots | ||
| /// - Vault assets (fungible) starting from empty vault | ||
| /// | ||
| /// The test ensures the optimized code path in `upsert_accounts` produces correct results | ||
| /// by comparing the final account state against a manually constructed expected state. | ||
| #[test] | ||
| #[allow(clippy::too_many_lines)] | ||
| fn optimized_delta_matches_full_account_method() { | ||
| let mut conn = setup_test_db(); | ||
|
|
||
| // Create an account with value slots only (no map slots to avoid SmtForest complexity) | ||
| let slot_value_initial = | ||
| Word::from([Felt::new(100), Felt::new(200), Felt::new(300), Felt::new(400)]); | ||
|
|
||
| let component_storage = vec![ | ||
| StorageSlot::with_value(StorageSlotName::mock(0), slot_value_initial), | ||
| StorageSlot::with_value(StorageSlotName::mock(1), EMPTY_WORD), | ||
| ]; | ||
|
|
||
| let account_component_code = CodeBuilder::default() | ||
| .compile_component_code("test::interface", "pub proc foo push.1 end") | ||
| .unwrap(); | ||
|
|
||
| let component = AccountComponent::new(account_component_code, component_storage) | ||
| .unwrap() | ||
| .with_supported_type(AccountType::RegularAccountImmutableCode); | ||
|
|
||
| let account = AccountBuilder::new([10u8; 32]) | ||
| .account_type(AccountType::RegularAccountImmutableCode) | ||
| .storage_mode(AccountStorageMode::Public) | ||
| .with_component(component) | ||
| .with_auth_component(AuthFalcon512Rpo::new(PublicKeyCommitment::from(EMPTY_WORD))) | ||
| .build_existing() | ||
| .unwrap(); | ||
|
|
||
| let block_1 = BlockNumber::from(1); | ||
| let block_2 = BlockNumber::from(2); | ||
| insert_block_header(&mut conn, block_1); | ||
| insert_block_header(&mut conn, block_2); | ||
|
|
||
| // Insert the initial account at block 1 (full state) - no vault assets | ||
| let delta_initial = AccountDelta::try_from(account.clone()).unwrap(); | ||
| let account_update_initial = BlockAccountUpdate::new( | ||
| account.id(), | ||
| account.commitment(), | ||
| AccountUpdateDetails::Delta(delta_initial), | ||
| ); | ||
| upsert_accounts(&mut conn, &[account_update_initial], block_1).expect("Initial upsert failed"); | ||
|
|
||
| // Verify initial state | ||
| let full_account_before = | ||
| select_full_account(&mut conn, account.id()).expect("Failed to load full account"); | ||
| assert_eq!(full_account_before.nonce(), account.nonce()); | ||
| assert!( | ||
| full_account_before.vault().assets().next().is_none(), | ||
| "Vault should be empty initially" | ||
| ); | ||
|
|
||
| // Create a partial delta to apply: | ||
| // - Increment nonce by 5 | ||
| // - Update the first value slot | ||
| // - Add 500 tokens to the vault (starting from empty) | ||
|
|
||
| let new_slot_value = | ||
| Word::from([Felt::new(111), Felt::new(222), Felt::new(333), Felt::new(444)]); | ||
| let faucet_id = AccountId::try_from(ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET).unwrap(); | ||
|
|
||
| // Find the slot name from the account's storage | ||
| let value_slot_name = | ||
| full_account_before.storage().slots().iter().next().unwrap().name().clone(); | ||
|
|
||
| // Build the storage delta (value slot update only) | ||
| let storage_delta = { | ||
| let deltas = BTreeMap::from_iter([( | ||
| value_slot_name.clone(), | ||
| StorageSlotDelta::Value(new_slot_value), | ||
| )]); | ||
| AccountStorageDelta::from_raw(deltas) | ||
| }; | ||
|
|
||
| // Build the vault delta (add 500 tokens to empty vault) | ||
| let vault_delta = { | ||
| let mut delta = AccountVaultDelta::default(); | ||
| let asset = Asset::Fungible(FungibleAsset::new(faucet_id, 500).unwrap()); | ||
| delta.add_asset(asset).unwrap(); | ||
| delta | ||
| }; | ||
|
|
||
| // Create a partial delta | ||
| let nonce_delta = Felt::new(5); | ||
| let partial_delta = AccountDelta::new( | ||
| full_account_before.id(), | ||
| storage_delta.clone(), | ||
| vault_delta.clone(), | ||
| nonce_delta, | ||
| ) | ||
| .unwrap(); | ||
| assert!(!partial_delta.is_full_state(), "Delta should be partial, not full state"); | ||
|
|
||
| // Construct the expected final account by applying the delta | ||
| let expected_nonce = Felt::new(full_account_before.nonce().as_int() + nonce_delta.as_int()); | ||
| let expected_code_commitment = full_account_before.code().commitment(); | ||
|
|
||
| let mut expected_account = full_account_before.clone(); | ||
| expected_account.apply_delta(&partial_delta).unwrap(); | ||
| let final_account_for_commitment = expected_account; | ||
|
|
||
| let final_commitment = final_account_for_commitment.commitment(); | ||
| let expected_storage_commitment = final_account_for_commitment.storage().to_commitment(); | ||
| let expected_vault_root = final_account_for_commitment.vault().root(); | ||
|
|
||
| // ----- Apply the partial delta via upsert_accounts (optimized path) ----- | ||
| let account_update = BlockAccountUpdate::new( | ||
| account.id(), | ||
| final_commitment, | ||
| AccountUpdateDetails::Delta(partial_delta), | ||
| ); | ||
| upsert_accounts(&mut conn, &[account_update], block_2).expect("Partial delta upsert failed"); | ||
|
|
||
| // ----- VERIFY: Query the DB and check that optimized path produced correct results ----- | ||
|
|
||
| let (header_after, storage_header_after) = | ||
| select_account_header_with_storage_header_at_block(&mut conn, account.id(), block_2) | ||
| .expect("Query should succeed") | ||
| .expect("Account should exist"); | ||
|
|
||
| // Verify nonce | ||
| assert_eq!( | ||
| header_after.nonce(), | ||
| expected_nonce, | ||
| "Nonce mismatch: optimized={:?}, expected={:?}", | ||
| header_after.nonce(), | ||
| expected_nonce | ||
| ); | ||
|
|
||
| // Verify code commitment (should be unchanged) | ||
| assert_eq!( | ||
| header_after.code_commitment(), | ||
| expected_code_commitment, | ||
| "Code commitment mismatch" | ||
| ); | ||
|
|
||
| // Verify storage header commitment | ||
| assert_eq!( | ||
| storage_header_after.to_commitment(), | ||
| expected_storage_commitment, | ||
| "Storage header commitment mismatch" | ||
| ); | ||
|
|
||
| // Verify vault assets | ||
| let vault_assets_after = select_account_vault_at_block(&mut conn, account.id(), block_2) | ||
| .expect("Query vault should succeed"); | ||
|
|
||
| assert_eq!(vault_assets_after.len(), 1, "Should have 1 vault asset"); | ||
| assert_matches!(&vault_assets_after[0], Asset::Fungible(f) => { | ||
| assert_eq!(f.faucet_id(), faucet_id, "Faucet ID should match"); | ||
| assert_eq!(f.amount(), 500, "Amount should be 500"); | ||
| }); | ||
|
|
||
| // Verify the account commitment matches | ||
| assert_eq!( | ||
| header_after.commitment(), | ||
| final_commitment, | ||
| "Account commitment should match the expected final state" | ||
| ); | ||
|
|
||
| // Also verify we can load the full account and it has correct state | ||
| let full_account_after = select_full_account(&mut conn, account.id()) | ||
| .expect("Failed to load full account after update"); | ||
|
|
||
| assert_eq!(full_account_after.nonce(), expected_nonce, "Full account nonce mismatch"); | ||
| assert_eq!( | ||
| full_account_after.storage().to_commitment(), | ||
| expected_storage_commitment, | ||
| "Full account storage commitment mismatch" | ||
| ); | ||
| assert_eq!( | ||
| full_account_after.vault().root(), | ||
| expected_vault_root, | ||
| "Full account vault root mismatch" | ||
| ); | ||
| } |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added tests for the optimized delta path only cover (a) empty vault -> add fungible, and explicitly avoid storage map slots. Since the optimization introduces new SMT-root computation logic for both vault and storage maps, please add coverage for at least: (1) updating a non-empty vault (add + remove to reach 0), and (2) updating a non-empty storage map slot. This will catch cases where computing roots requires prior tree state.
| - Pin tool versions in CI ([#1523](https://github.com/0xMiden/miden-node/pull/1523)). | ||
| - Add `GetVaultAssetWitnesses` and `GetStorageMapWitness` RPC endpoints to store ([#1529](https://github.com/0xMiden/miden-node/pull/1529)). | ||
| - Add check to ensure tree store state is in sync with database storage ([#1532](https://github.com/0xMiden/miden-node/issues/1534)). | ||
| - Improve speed account updates ([#1567](https://github.com/0xMiden/miden-node/pull/1567)). |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog entry grammar: consider changing "Improve speed account updates" to "Improve speed of account updates" (or similar) for clarity.
| - Improve speed account updates ([#1567](https://github.com/0xMiden/miden-node/pull/1567)). | |
| - Improve speed of account updates ([#1567](https://github.com/0xMiden/miden-node/pull/1567)). |
| // Compute vault keys for each faucet ID | ||
| let vault_keys: Vec<Vec<u8>> = Vec::from_iter(faucet_ids.iter().filter_map(|faucet_id| { | ||
| FungibleAsset::new(*faucet_id, 0).ok().map(|asset| { | ||
| let key: Word = asset.vault_key().into(); | ||
| key.to_bytes() | ||
| }) | ||
| })); |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select_vault_balances_by_faucet_ids() silently skips faucet IDs where FungibleAsset::new(*faucet_id, 0) returns an error (via filter_map(... .ok() ...)). That can turn an invalid faucet ID into an implicit 0 balance and lead to incorrect state updates. Prefer validating all faucet_ids up front and returning an error if any are invalid instead of dropping them.
| // Compute vault keys for each faucet ID | |
| let vault_keys: Vec<Vec<u8>> = Vec::from_iter(faucet_ids.iter().filter_map(|faucet_id| { | |
| FungibleAsset::new(*faucet_id, 0).ok().map(|asset| { | |
| let key: Word = asset.vault_key().into(); | |
| key.to_bytes() | |
| }) | |
| })); | |
| // Compute vault keys for each faucet ID, failing if any faucet ID is invalid | |
| let vault_keys: Vec<Vec<u8>> = faucet_ids | |
| .iter() | |
| .map(|faucet_id| { | |
| let asset = FungibleAsset::new(*faucet_id, 0)?; | |
| let key: Word = asset.vault_key().into(); | |
| Ok(key.to_bytes()) | |
| }) | |
| .collect::<Result<Vec<_>, _>>()?; |
| let mut forest = SmtForest::new(); | ||
| for (slot_name, map_delta) in delta.maps() { | ||
| // Find the previous root from the header | ||
| let prev_root = header | ||
| .slots() | ||
| .find(|s| s.name() == slot_name) | ||
| .map(StorageSlotHeader::value) | ||
| .unwrap_or_default(); | ||
|
|
||
| let entries = | ||
| Vec::from_iter(map_delta.entries().iter().map(|(key, value)| ((*key).into(), *value))); | ||
|
|
||
| if !entries.is_empty() { | ||
| let new_root = | ||
| forest.batch_insert(prev_root, entries.iter().copied()).map_err(|e| { | ||
| DatabaseError::DataCorrupted(format!( | ||
| "Failed to compute storage map root: {e:?}" | ||
| )) | ||
| })?; | ||
| map_updates.insert(slot_name, new_root); |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply_storage_delta_to_header() uses a fresh SmtForest::new() and then calls batch_insert(prev_root, ...) for map slots. If prev_root corresponds to an existing non-empty map SMT, this ad-hoc forest will not have the prior tree nodes/entries needed to compute an updated root (InnerForest keeps a long-lived SmtForest specifically for this). This risks producing an incorrect root or failing for any account with non-empty storage maps. Consider reusing the existing forest/state that already tracks these maps, or rebuilding the SMT from persisted map entries for the affected slot(s) before applying the delta (or otherwise providing the required authentication data).
| let mut entries: Vec<(Word, Word)> = Vec::new(); | ||
| let mut forest = SmtForest::new(); | ||
|
|
||
| // Process fungible asset changes | ||
| for (faucet_id, amount_delta) in vault_delta.fungible().iter() { | ||
| let prev_balance = prev_balances.get(faucet_id).copied().unwrap_or(0); | ||
| let new_balance: u64 = | ||
| (i128::from(prev_balance) + i128::from(*amount_delta)).try_into().map_err(|_| { | ||
| DatabaseError::DataCorrupted(format!("Balance underflow for faucet {faucet_id}")) | ||
| })?; | ||
|
|
||
| let key: Word = | ||
| FungibleAsset::new(*faucet_id, 0).expect("valid faucet id").vault_key().into(); | ||
|
|
||
| let value = if new_balance == 0 { | ||
| EMPTY_WORD | ||
| } else { | ||
| let asset: Asset = FungibleAsset::new(*faucet_id, new_balance) | ||
| .expect("valid fungible asset") | ||
| .into(); | ||
| Word::from(asset) | ||
| }; | ||
|
|
||
| entries.push((key, value)); | ||
| } | ||
|
|
||
| // Process non-fungible asset changes | ||
| for (asset, action) in vault_delta.non_fungible().iter() { | ||
| let key: Word = asset.vault_key().into(); | ||
| let value = match action { | ||
| NonFungibleDeltaAction::Add => Word::from(Asset::NonFungible(*asset)), | ||
| NonFungibleDeltaAction::Remove => EMPTY_WORD, | ||
| }; | ||
| entries.push((key, value)); | ||
| } | ||
|
|
||
| let new_root = forest.batch_insert(prev_root, entries.iter().copied()).map_err(|e| { | ||
| DatabaseError::DataCorrupted(format!("Failed to compute vault root: {e:?}")) | ||
| })?; |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compute_vault_root_after_delta() creates a new SmtForest and calls batch_insert(prev_root, ...). For accounts with a non-empty vault, computing the updated root generally requires knowledge of the previous tree (nodes/entries). A fresh forest won’t have that context, so this can yield an incorrect vault root or error in real updates (the added tests only cover empty-vault start). Consider computing the new root via the existing long-lived forest (e.g., InnerForest), or rebuilding the vault SMT from the persisted account_vault_assets set when prev_root is non-empty, and add a correctness check against final_state_commitment.
| let key: Word = | ||
| FungibleAsset::new(*faucet_id, 0).expect("valid faucet id").vault_key().into(); | ||
|
|
||
| let value = if new_balance == 0 { | ||
| EMPTY_WORD | ||
| } else { | ||
| let asset: Asset = FungibleAsset::new(*faucet_id, new_balance) | ||
| .expect("valid fungible asset") |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compute_vault_root_after_delta() uses expect("valid faucet id") / expect("valid fungible asset"). A malformed/invalid delta (or unexpected data) would panic the node instead of returning a DatabaseError. Please convert these to fallible error handling and return a structured error so apply_block can fail gracefully.
| let key: Word = | |
| FungibleAsset::new(*faucet_id, 0).expect("valid faucet id").vault_key().into(); | |
| let value = if new_balance == 0 { | |
| EMPTY_WORD | |
| } else { | |
| let asset: Asset = FungibleAsset::new(*faucet_id, new_balance) | |
| .expect("valid fungible asset") | |
| let key: Word = FungibleAsset::new(*faucet_id, 0) | |
| .map_err(|e| { | |
| DatabaseError::DataCorrupted(format!( | |
| "Invalid faucet id {faucet_id} when computing vault root: {e}" | |
| )) | |
| })? | |
| .vault_key() | |
| .into(); | |
| let value = if new_balance == 0 { | |
| EMPTY_WORD | |
| } else { | |
| let asset: Asset = FungibleAsset::new(*faucet_id, new_balance) | |
| .map_err(|e| { | |
| DatabaseError::DataCorrupted(format!( | |
| "Invalid fungible asset for faucet {faucet_id} and balance {new_balance} when computing vault root: {e}" | |
| )) | |
| })? |
Context
Currently applying a partial delta to an existing account requires loading the full account state
consisting of code, all storage map entries, all vault assets, which is very costly - we spend ~250ms on this per block (that's a lot!).
See #1538
What
Optimizes account delta updates by avoiding loading full
Accountobjects from the database when only applying partial deltas.Core changes
module
crates/store/src/db/models/queries/accounts/delta.rsfn select_account_state_for_delta()- fetches only nonce, code_commitment, storage_header, vault_rootfn select_vault_balances_by_faucet_ids()- fetches only specific vault balances being updatedfn apply_storage_delta_to_header()- updates storage header using an ad-hocSmtForestfn compute_vault_root_after_delta()- compute new vault root using ad-hocSmtForestfn upsert_accounts- use optimized path for partial deltas viaenum AccountStateForInsertwith three variants:::Private- no public state::FullAccount- new account creation == "full-state delta"::PartialState- incremental update == "partial delta"Caveats
We currently create temporary/ad-hoc
SmtForests to calculate the roots as part offn Db::apply_block. Now we then feed it intoForestInnerwhich recomputes these same trees again as part offn InnerForest::apply_block_updates. This is obviously duplicated effort.Downsides
The optimizations breaks abstraction, it sidesteps them and fetches valuesets from DB directly. It entangles DB schema and node more then before.
Future Work
We could technically reverse the flow and update the
InnerForestfirst AND make it track storage map values instead of just maps ( CC @iamrecursion , I think this is already covered byLargeSmtForestwork). This would allow us to provide pre-computed smt values directly to theDb::apply_block.There is a downside though: Locks, lock hold time, and making the defacto state the
SmtForestandAccountTreerather than the DB. I'd like to experiment in a follow up and land the core change as part of this PR.ToDo
Eval the impact of building the extra ad-hoc
SmtForests vs the full accounts.