Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions crates/core/src/surfnet/locker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1059,8 +1059,19 @@ impl SurfnetSvmLocker {
// the RPC handler from hanging on recv() when errors occur during
// account fetching, ALT resolution, or other pre-processing steps.
// This is critical for issue #454 where program close stops block production.
let _ =
status_tx.try_send(TransactionStatusEvent::VerificationFailure(e.to_string()));
//
// AccountLoadedTwice errors should go through SimulationFailure to produce
// Agave-compatible JSON-RPC error format with structured `err` and `data` fields.
let err_str = e.to_string();
if err_str.contains("Account loaded twice") {
let _ = status_tx.try_send(TransactionStatusEvent::SimulationFailure((
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure SimulationFailure is the correct error here? What if simulation is disabled? (skip_preflight = true)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

enum ProcessTransactionResult {
    Success(TransactionMetadata),
    SimulationFailure(FailedTransactionMetadata),
    ExecutionFailure(FailedTransactionMetadata),
}

Not success, and the transaction is not executing at this point. We could introduce a new enum but it seems overkill.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I'm saying I think to better match with what a node would return, if skip_preflight = true we return an ExecutionFailure - because we're not simulating the node would (I believe) return with the same error but while actually executing the transaction. Vs if skip_preflight = false, on a real node you'd be simulating and you'd return a SimulationFailure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check is pre-execution / pre-simulation, and since the transaction is not executed (0 CU), simulation seems to be the least worst to me.
Neither of these variant are semantically correct: with the approach you're suggesting (ExecutionFailure), if we use the simulation rpc endpoint and hit that code-path, we'd be returning an execution error.

I honestly don't think it's a big deal as this enum is not exposed anyway in the RPC response.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this enum is not exposed anyway in the RPC response

That's not true. The send_transaction RPC method recives this TransactionStatusEvent and reacts differently according to the variant. VerificationFailure has no data on the final error and just has a message. SimulationError returns a RpcSimulateTransactionResult on the data of the error. ExecutionFailure is completely ignored - it's processed by the VM and the user would need to fetch the transaction by signature to see the error.

So perhaps your point is shining light on that we're further deviating from how mainnet would actually handle - if you skip the simulation on mainnet do you get the same error contents ("Account loaded twice") but during execution? Why isn't LiteSVM returning the correct error in the first place.

This is feeling more like a bandaid rather than a fix -perhaps this should be at the LiteSVM level.

if we use the simulation rpc endpoint and hit that code-path
Simulation RPC doesn't use this code path, I don't think that's relevant

Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is pre-execution / pre-simulation

Agreed - what I'm saying is that on mainnet this check isn't pre-execution/pre-simulation - so how can we have surfpool either behave the same way or appear to behave the same way

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on mainnet this check isn't pre-execution/pre-simulation

Wdym? it has nothing to do with mainnet / devnet, it's a client implementation detail.

With this patch, behavior Agave and Surfpool are identical - as explained in the description, the PR was built with a test vector Surfpool / Agave.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed this in the PR description:

Root cause: get_pubkeys_from_message() concatenated static keys with ALT-loaded addresses without deduplication.

Would fixing that root cause allow LiteSVM to return the correct error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LiteSVM should not be involved at all.

TransactionError::AccountLoadedTwice,
surfpool_types::TransactionMetadata::default(),
)));
} else {
let _ =
status_tx.try_send(TransactionStatusEvent::VerificationFailure(err_str));
}
return Err(e);
}
};
Expand Down Expand Up @@ -1144,6 +1155,18 @@ impl SurfnetSvmLocker {
.get_loaded_addresses(remote_ctx, &transaction.message)
.await?;

// Check for duplicate accounts between static keys and ALT-loaded addresses.
// Agave rejects such transactions pre-execution with AccountLoadedTwice.
if let Some(ref loaded) = tx_loaded_addresses {
let static_keys: HashSet<&Pubkey> =
transaction.message.static_account_keys().iter().collect();
for loaded_key in loaded.all_loaded_addresses() {
if static_keys.contains(loaded_key) {
return Err(TransactionError::AccountLoadedTwice.into());
}
}
}

// we don't want the pubkeys of the address lookup tables to be included in the transaction accounts,
// but we do want the pubkeys of the accounts _loaded_ by the ALT to be in the transaction accounts.
let transaction_accounts = self
Expand Down
107 changes: 105 additions & 2 deletions crates/core/src/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ use solana_epoch_info::EpochInfo;
use solana_hash::Hash;
use solana_keypair::Keypair;
use solana_message::{
AddressLookupTableAccount, Message, VersionedMessage, legacy,
v0::{self},
AddressLookupTableAccount, Message, MessageHeader, VersionedMessage, legacy,
v0::{self, MessageAddressTableLookup},
};
use solana_pubkey::Pubkey;
use solana_rpc_client_api::response::Response as RpcResponse;
Expand Down Expand Up @@ -7883,3 +7883,106 @@ async fn test_duplicate_transaction_rejected(test_type: TestType) {

println!("Duplicate transaction rejection test passed!");
}

// ============================================================================
// AccountLoadedTwice: V0 transactions with duplicate accounts in static keys + ALT
// ============================================================================
// When a V0 transaction has an account in both its static keys and an Address
// Lookup Table, Agave rejects pre-execution with AccountLoadedTwice (0 CU).
// Surfpool must match this behavior.
#[test_case(TestType::sqlite(); "with on-disk sqlite db")]
#[test_case(TestType::in_memory(); "with in-memory sqlite db")]
#[test_case(TestType::no_db(); "with no db")]
#[cfg_attr(feature = "postgres", test_case(TestType::postgres(); "with postgres db"))]
#[tokio::test(flavor = "multi_thread")]
async fn test_account_loaded_twice_rejected(test_type: TestType) {
let (svm_locker, _simnet_cmd_tx, _simnet_events_rx) =
boot_simnet(BlockProductionMode::Clock, Some(400), test_type);

let payer = Keypair::new();
svm_locker
.airdrop(&payer.pubkey(), LAMPORTS_PER_SOL)
.unwrap()
.unwrap();

let recent_blockhash = svm_locker.with_svm_reader(|svm| svm.latest_blockhash());

// Create an ALT that contains system_program (11111...111)
let alt_key = Pubkey::new_unique();
let system_program_id = system_program::id();

let alt_account_data = AddressLookupTable {
meta: LookupTableMeta {
authority: Some(payer.pubkey()),
..Default::default()
},
addresses: vec![system_program_id].into(),
};

svm_locker.with_svm_writer(|svm| {
let alt_data = alt_account_data.serialize_for_tests().unwrap();
let alt_account = Account {
lamports: 1_000_000,
data: alt_data,
owner: solana_address_lookup_table_interface::program::id(),
executable: false,
rent_epoch: 0,
};
svm.set_account(&alt_key, alt_account).unwrap();
});

// Build a V0 message that has system_program in BOTH static keys AND the ALT.
// try_compile would deduplicate, so we build the message manually.
let recipient = Pubkey::new_unique();
let v0_message = v0::Message {
header: MessageHeader {
num_required_signatures: 1,
num_readonly_signed_accounts: 0,
// system_program is readonly unsigned
num_readonly_unsigned_accounts: 1,
},
// Static keys: [payer (signer+writable), recipient (writable), system_program (readonly)]
account_keys: vec![payer.pubkey(), recipient, system_program_id],
recent_blockhash,
// A simple transfer instruction: program_id_index=2 (system_program),
// accounts=[0 (payer), 1 (recipient)]
instructions: vec![solana_message::compiled_instruction::CompiledInstruction {
program_id_index: 2,
accounts: vec![0, 1],
data: {
// system_instruction::transfer encodes as: [2,0,0,0] + 8-byte LE amount
let mut data = vec![2, 0, 0, 0];
data.extend_from_slice(&100u64.to_le_bytes());
data
},
}],
// ALT lookup that also loads system_program (index 0 in the ALT) as readonly
address_table_lookups: vec![MessageAddressTableLookup {
account_key: alt_key,
writable_indexes: vec![],
readonly_indexes: vec![0], // system_program is at index 0 in the ALT
}],
};

let tx = VersionedTransaction::try_new(VersionedMessage::V0(v0_message), &[&payer])
.expect("Failed to create transaction");

let (status_tx, _status_rx) = crossbeam_channel::unbounded();
let result = svm_locker
.process_transaction(&None, tx, status_tx, false, true)
.await;

assert!(
result.is_err(),
"Transaction with duplicate account should be rejected, got: {:?}",
result
);
let err_string = result.unwrap_err().to_string();
assert!(
err_string.contains("Account loaded twice"),
"Expected AccountLoadedTwice error, got: {}",
err_string
);

println!("AccountLoadedTwice rejection test passed!");
}