feat: Track minimal data in partial account and always enable lazy loading in tests#1963
Conversation
igamigo
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Super cool PR, this will drastically improve the devex of working with price oracles!!! |
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}`
f27e131 to
6453d5c
Compare
Enables lazy loading by default in all tests by changing the conversion from
AccounttoPartialAccountto only represent the minimal account and by settingTransactionContextBuilder::is_lazy_loading_enabled = true. Builds on top of #1955.The changes:
TransactionArgs::foreign_account_inputs. So, foreign accounts are now no longer loaded upfront but only when they are needed.Accountis converted toPartialAccount. Essentially: New accounts are converted to a "full" partial representation and existing accounts are converted to a minimal representation.minimal_partial_account.MockChain::get_foreign_account_inputsreturnsAccountinstead ofPartialAccount. See Removeforeign_account_inputsfield from theTransactionArgs#1473 (comment) for details.transaction_with_stale_foreign_account_inputs_failswas removed because the checks it was testing no longer exist. In principle, we could move this check toTransactionExecutorHost::on_foreign_account_requestedbut 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.test_epilogue_asset_preservation_violation_too_few_inputandtest_epilogue_asset_preservation_violation_too_many_fungible_inputto be more concise, execute a full transaction instead ofexecute_code. This turned out to be unnecessary in hindsight, but also partially addresses Consider removingcreate_mock_notes#1845, which I think is a plus.closes #1473