Skip to content

Refactor - TransactionProcessingCallback#7381

Merged
Lichtso merged 3 commits intoanza-xyz:masterfrom
Lichtso:refactor/transaction_processing_callback
Aug 10, 2025
Merged

Refactor - TransactionProcessingCallback#7381
Lichtso merged 3 commits intoanza-xyz:masterfrom
Lichtso:refactor/transaction_processing_callback

Conversation

@Lichtso
Copy link

@Lichtso Lichtso commented Aug 7, 2025

Problem

With #6036 merged we don't need TransactionProcessingCallback::account_matches_owners() anymore and we can finally start working on the simplification of the global program cache index structure by using the last modified slot of the accounts-db. To do so we would have to add that as part of the return value of TransactionProcessingCallback::get_account_shared_data().

Summary of Changes

This is a breaking change to the interface (TransactionProcessingCallback) of the SVM:

  • Adds Slot to return type of TransactionProcessingCallback::get_account_shared_data().
  • Removes TransactionProcessingCallback::account_matches_owners().

@Lichtso Lichtso requested a review from a team as a code owner August 7, 2025 17:11
@Lichtso Lichtso changed the title Refactor/transaction processing callback Refactor - TransactionProcessingCallback Aug 7, 2025
@Lichtso Lichtso requested a review from buffalojoec August 7, 2025 17:57
.0
.and_then(|account| owners.iter().position(|entry| entry == account.owner()))
fn get_account_shared_data(&self, pubkey: &Pubkey) -> Option<(AccountSharedData, Slot)> {
self.do_load(pubkey).0.map(|account| (account, 0))
Copy link

Choose a reason for hiding this comment

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

This is a path used in production. Would you mind explaining here why the slot does not matter?

Copy link

Choose a reason for hiding this comment

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

Explaining = adding a comment in the code.

Copy link
Author

@Lichtso Lichtso Aug 7, 2025

Choose a reason for hiding this comment

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

It will matter, but not in this PR. It is only to get the interface changes at the v3.0 version bump. So for now, the slot is a dummy. I did add a comment in the last commit.

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.2%. Comparing base (5bf937d) to head (88c89ae).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7381   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         796      796           
  Lines      361890   361875   -15     
=======================================
+ Hits       301326   301388   +62     
+ Misses      60564    60487   -77     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

I think this feels like maybe the wrong approach. What about introducing a new method that has the desired return type of Option<(AccountSharedData, Slot)> and just using it in program cache operations?

Adding a dummy value to a method that's currently used on the hot path (as @LucasSte said) by things like account loader seems like an anti-pattern. Account loader & friends can just use get_account_shared_data as it's written and implemented, and this new method can use a dummy because it can be explicitly labeled as unused in account loader. Wdyt?

@Lichtso
Copy link
Author

Lichtso commented Aug 8, 2025

There is no way to "just using it in program cache operations" because the program cache will need to get this info from the accounts-db, it can not come up with it on its own. Also, Hana worked really hard so that we do not directly access the accounts-db for transaction loading anymore and instead go via AccountLoader. In a similar way, the AccountLoader will have to memorize the value as well and then return it to e.g. the program loader, replenish & extract. I am currently working on these parts, but they are larger and I wanted to separate the external interface changes in this PR.

@buffalojoec
Copy link

There is no way to "just using it in program cache operations"

So you're telling me it's impossible to only use get_account_shared_data_with_last_updated_slot for account loads from accountsDB that are programs? We already have loads of special-casing for programs as it is, including filter_executable_accounts, which builds the map that cache operations depend on.

Point being, non-programs wouldn't need a dummy/discarded value.

@Lichtso
Copy link
Author

Lichtso commented Aug 8, 2025

Just yesterday we removed most special handling around filter_executable_accounts. The AccountLoader also does not differentiate between normal accounts and program accounts. And how would it? You only know if something is owned by a loader after you loaded it. So, how would splitting that in two interfaces work? Also, normal accounts and program accounts can turn into one another during a TX batch, seems like it would cause more trouble than it is worth.

@Lichtso Lichtso merged commit 796f5bc into anza-xyz:master Aug 10, 2025
51 checks passed
@Lichtso Lichtso deleted the refactor/transaction_processing_callback branch August 10, 2025 10:16
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.

4 participants