Calls inspect_account() on the fee payer #2718

brooksprumo · 2024-08-23T13:33:53Z

Problem

#2678 added a callback to inspect accounts prior to transaction processing. Unfortunately this did not include the fee payer, which is loaded separately.

Summary of Changes

Also inspect the fee payer.

brooksprumo · 2024-08-23T14:09:52Z

@HaoranYi - we missed this one earlier
@buffalojoec - you had reviewed the first PR, so wanted to include you on this one too
@jstarry - I asked you about where to put the call to inspect_account(), so wanted your review on the actual PR

HaoranYi

Ah. I missed this one. Looks like it is a recent refactoring that moved the fee accounts out of transaction account loading.

Good catch.

jstarry

Are there any tests for this inspect account feature? We should have a test to ensure all writable accounts are now inspected after this bug fix.

svm/src/transaction_processor.rs

brooksprumo · 2024-08-27T19:17:00Z

Are there any tests for this inspect account feature? We should have a test to ensure all writable accounts are now inspected after this bug fix.

Good call. I've added tests in f3efdd4.

jstarry

The new tests look great but I was actually hoping for a test that would have caught this bug at a higher level. Namely an integration test that tests that all accounts in a transaction were inspected. Actually it would be nice to have two integration tests.. one for a fully loaded transaction and one for a transaction which only validated the fee payer but is still able to be committed (named FeesOnlyTransaction)

jstarry · 2024-08-28T00:38:48Z

svm/src/account_loader.rs

+
+        let message = Message {
+            account_keys: vec![address0, address1, address2, address3],
+            header: MessageHeader::default(),


So I guess this default header is causing all accounts to be loaded as writable because it means there are 0 signers and 0 readonly non-signers? And the program is loaded as readonly because it was demoted by demote_program_id? Do you mind adding comments describing this behavior?

Sure, I can add a comment. I'm not familiar with these specifics; why is that information germane to this test?

Because it's important that account inspection correctly captures whether an account is loaded with a write lock, right?

I think no, not this test. This test only cares that the accounts are being inspected. The writability is assumed to be correct. (I imagine there are tests that are test this, yes?)

Ok that seems reasonable to me

HaoranYi · 2024-08-28T12:52:32Z

Looking at the code, I think we might miss sysvar too.

brooksprumo · 2024-08-28T15:10:31Z

Looking at the code, I think we might miss sysvar too.

inspect_account() will only "see" accounts loaded by transactions**. Any other account that is modified outside of transaction processing will need to be handled separately. For our lt hash work, yeah, we'll need to handle some extra stuff. For this PR, I don't think we need anything special, right?

**: we should see all the accounts in the transactions, right?

HaoranYi · 2024-08-28T15:31:55Z

Looking at the code, I think we might miss sysvar too.

inspect_account() will only "see" accounts loaded by transactions**. Any other account that is modified outside of transaction processing will need to be handled separately. For our lt hash work, yeah, we'll need to handle some extra stuff. For this PR, I don't think we need anything special, right?

**: we should see all the accounts in the transactions, right?

Yeah. we will handle them specially in lt-hash work. Feel free to ignore the comments. It is out the scope of this PR.

brooksprumo · 2024-08-29T22:08:19Z

The new tests look great but I was actually hoping for a test that would have caught this bug at a higher level. Namely an integration test that tests that all accounts in a transaction were inspected.

Done! I've added an integration test in a401c5f.

Actually it would be nice to have two integration tests.. one for a fully loaded transaction and one for a transaction which only validated the fee payer but is still able to be committed (named FeesOnlyTransaction)

I'm not sure what this would be testing. Should the transaction not load the other accounts in the FeesOnlyTransaction case? If that's true, then I think it should be a test on load_transaction_accounts() then, and not inspect_accounts(). Wdyt?

jstarry · 2024-08-30T01:43:13Z

I'm not sure what this would be testing. Should the transaction not load the other accounts in the FeesOnlyTransaction case? If that's true, then I think it should be a test on load_transaction_accounts() then, and not inspect_accounts(). Wdyt?

Yeah I'm not really sure either. Maybe it can come later? I'm just thinking about how loading could bail halfway through and so some accounts would be recorded as inspected but others would not be. And in the end we will only be committing state changes for fee payer / nonce.

jstarry · 2024-08-30T01:40:13Z

svm/tests/mock_bank.rs

@@ -90,6 +93,19 @@ impl TransactionProcessingCallback for MockBankCallback {
            .unwrap()
            .insert(*program_id, account_data);
    }
+
+    fn inspect_account(&self, address: &Pubkey, account_state: AccountState, is_writable: bool) {


Do we want the mock to push read-only accounts here?

I was thinking 'yes', but I guess doesn't need to. What would you prefer here?

I don't have any preference, we can keep it as is

jstarry · 2024-08-30T01:40:44Z

svm/tests/integration_test.rs

+    // Ensure all the expected inspected accounts were inspected
+    let actual_inspected_accounts = mock_bank.inspected_accounts.read().unwrap().clone();
+    for (expected_pubkey, expected_account) in expected_inspected_accounts {
+        let actual_account = actual_inspected_accounts.get(&expected_pubkey).unwrap();
+        assert_eq!(
+            expected_account, *actual_account,
+            "pubkey: {expected_pubkey}",
+        );
+    }


Should also assert that len of actual and expected accounts is the same. In your test, the actual inspected accounts includes extra read only accounts.

Good call. I've added a len check, and also added read-only accounts in 877a38c.

brooksprumo · 2024-08-30T15:40:31Z

PR #2623 refactored the svm integration tests, so I need to rebase and rewrite the integration test in this PR.

brooksprumo self-assigned this Aug 23, 2024

brooksprumo marked this pull request as ready for review August 23, 2024 14:09

brooksprumo requested review from HaoranYi, jstarry and buffalojoec August 23, 2024 14:09

HaoranYi previously approved these changes Aug 23, 2024

View reviewed changes

brooksprumo requested a review from 2501babe August 23, 2024 17:34

2501babe previously approved these changes Aug 23, 2024

View reviewed changes

buffalojoec previously approved these changes Aug 23, 2024

View reviewed changes

jstarry reviewed Aug 24, 2024

View reviewed changes

svm/src/transaction_processor.rs Outdated Show resolved Hide resolved

brooksprumo dismissed stale reviews from buffalojoec, 2501babe, and HaoranYi via f3efdd4 August 27, 2024 19:16

brooksprumo requested a review from jstarry August 27, 2024 20:30

jstarry reviewed Aug 28, 2024

View reviewed changes

2501babe mentioned this pull request Aug 29, 2024

svm: allow conflicting transactions in entries #2702

Closed

jstarry reviewed Aug 30, 2024

View reviewed changes

brooksprumo added 5 commits August 30, 2024 11:33

Calls inspect_account() on the fee payer

31b1b5f

pr: adds tests

eb553b9

pr: add integration test

2238a6c

clippy

5ee617f

pr: check non-writable accounts in integration test

a2556cc

updates to handle the refactor

b19d98c

brooksprumo force-pushed the lthash/inspect-accounts branch from 877a38c to b19d98c Compare August 30, 2024 18:31

jstarry approved these changes Aug 30, 2024

View reviewed changes

brooksprumo merged commit 4659342 into anza-xyz:master Aug 31, 2024
40 checks passed

brooksprumo deleted the lthash/inspect-accounts branch September 2, 2024 14:04

ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024

Calls inspect_account() on the fee payer (anza-xyz#2718)

883a50a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calls inspect_account() on the fee payer #2718

Calls inspect_account() on the fee payer #2718

brooksprumo commented Aug 23, 2024

brooksprumo commented Aug 23, 2024

HaoranYi left a comment

jstarry left a comment

brooksprumo commented Aug 27, 2024

jstarry left a comment

jstarry Aug 28, 2024

brooksprumo Aug 28, 2024

jstarry Aug 29, 2024

brooksprumo Aug 29, 2024

jstarry Aug 29, 2024

HaoranYi commented Aug 28, 2024

brooksprumo commented Aug 28, 2024

HaoranYi commented Aug 28, 2024

brooksprumo commented Aug 29, 2024

jstarry commented Aug 30, 2024

jstarry Aug 30, 2024

brooksprumo Aug 30, 2024

jstarry Aug 30, 2024

jstarry Aug 30, 2024

brooksprumo Aug 30, 2024

brooksprumo commented Aug 30, 2024

Calls inspect_account() on the fee payer #2718

Calls inspect_account() on the fee payer #2718

Conversation

brooksprumo commented Aug 23, 2024

Problem

Summary of Changes

brooksprumo commented Aug 23, 2024

HaoranYi left a comment

Choose a reason for hiding this comment

jstarry left a comment

Choose a reason for hiding this comment

brooksprumo commented Aug 27, 2024

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoranYi commented Aug 28, 2024

brooksprumo commented Aug 28, 2024

HaoranYi commented Aug 28, 2024

brooksprumo commented Aug 29, 2024

jstarry commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo commented Aug 30, 2024