Skip to content

feat: Track minimal data in partial account and always enable lazy loading in tests#1963

Merged
PhilippGackstatter merged 1 commit intonextfrom
pgackst-always-enable-lazy-loading
Oct 10, 2025
Merged

feat: Track minimal data in partial account and always enable lazy loading in tests#1963
PhilippGackstatter merged 1 commit intonextfrom
pgackst-always-enable-lazy-loading

Conversation

@PhilippGackstatter
Copy link
Copy Markdown
Contributor

@PhilippGackstatter PhilippGackstatter commented Oct 2, 2025

Enables lazy loading by default in all tests by changing the conversion from Account to PartialAccount to only represent the minimal account and by setting TransactionContextBuilder::is_lazy_loading_enabled = true. Builds on top of #1955.

The changes:

  • Remove TransactionArgs::foreign_account_inputs. So, foreign accounts are now no longer loaded upfront but only when they are needed.
  • Change how Account is converted to PartialAccount . Essentially: New accounts are converted to a "full" partial representation and existing accounts are converted to a minimal representation.
    • This cleaned up a few lazy loading things in test code like minimal_partial_account.
  • MockChain::get_foreign_account_inputs returns Account instead of PartialAccount. See Remove foreign_account_inputs field from the TransactionArgs #1473 (comment) for details.
  • transaction_with_stale_foreign_account_inputs_fails was removed because the checks it was testing no longer exist. In principle, we could move this check to TransactionExecutorHost::on_foreign_account_requested but in practice it makes little difference: We're already in the middle of transaction execution and so whether the host aborts or the kernel isn't very important. The pro for having it is that the host could return a better error than the tx kernel while the pro for not having it is that it makes testing the kernel logic possible at all. For now it's the latter.
  • Overhaul test_epilogue_asset_preservation_violation_too_few_input and test_epilogue_asset_preservation_violation_too_many_fungible_input to be more concise, execute a full transaction instead of execute_code. This turned out to be unnecessary in hindsight, but also partially addresses Consider removing create_mock_notes #1845, which I think is a plus.

closes #1473

Copy link
Copy Markdown
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! I reviewed primarily non-testing code and left just a few comments inline.

@sergerad - some changes here may affect #1934.

Copy link
Copy Markdown
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! Once @igamigo had a change to do another view, I think we can merge.

Copy link
Copy Markdown
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

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

LGTM! Left one comment and also have one other question: once the foreign account gets loaded by DataStore::get_foreign_account_inputs, if the contained PartialAccount has any storage and vault data that the transaction will use, there would be no calls to lazily load witnesses for foreign account state, right?

/// - For existing accounts, the storage is tracked minimally, i.e. the minimal necessary data
/// is included.
///
/// Because new accounts always have empty vaults, in both cases, the asset vault is a minimal
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the case for genesis accounts as well, right? I don't recall precisely but I believe they were created with a non-zero nonce? Asking mostly to sanity check that this is fine

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

once the foreign account gets loaded by DataStore::get_foreign_account_inputs, if the contained PartialAccount has any storage and vault data that the transaction will use, there would be no calls to lazily load witnesses for foreign account state, right?

Yes. On foreign account loading, advice inputs are constructed via TransactionAdviceInputs::add_account which adds all storage or vault info in the partial account into the advice provider. So if that partial account contained all to-be-accessed data, then no additional calls should be made after that.

This is the case for genesis accounts as well, right?

You mean "genesis account" as created in the node, right? If those are created with a non-zero nonce, then they will be treated as existing accounts in this conversion.

@partylikeits1983
Copy link
Copy Markdown
Member

Remove TransactionArgs::foreign_account_inputs. So, foreign accounts are now no longer loaded upfront but only when they are needed.

Super cool PR, this will drastically improve the devex of working with price oracles!!!

Base automatically changed from pgackst-use-tx-exec-host-in-tests to next October 10, 2025 10:32
feat: Refactor asset preservation rule tests

chore: Add missing lazy loading event

feat: Refactor stale foreign account test

feat: Remove `TransactionArgs::foreign_account_inputs`

chore: Refactor `MockChain::get_foreign_account_inputs`

chore: Use printdiagnostic for nice errors

chore: Remove `transaction_with_stale_foreign_account_inputs_fails`

chore: Remove unused foreign account injection

feat: Introduce `new_minimal` and `new_full` constructors for storage

chore: Remove `TransactionContextBuilder::enable_lazy_loading`

chore: clarify asset vault tracking

chore: format and cleanup

chore: add changelog

feat: Replace `From` impl with `PartialVault::{new_minimal, new_full}`
@PhilippGackstatter PhilippGackstatter force-pushed the pgackst-always-enable-lazy-loading branch from f27e131 to 6453d5c Compare October 10, 2025 10:58
@PhilippGackstatter PhilippGackstatter merged commit e595a82 into next Oct 10, 2025
19 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-always-enable-lazy-loading branch October 10, 2025 11: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.

Remove foreign_account_inputs field from the TransactionArgs

4 participants