Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: min slot bid svm #291

Merged
merged 5 commits into from
Dec 19, 2024
Merged

Feat: min slot bid svm #291

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f2369a7
feat: min slot support for submitting svm bids
m30m Dec 12, 2024
c5639f9
Sleep between retries
m30m Dec 12, 2024
869e1f0
Address comments
m30m Dec 14, 2024
1b19191
Merge branch 'main' into feat/min-slot-bid-svm
m30m Dec 18, 2024
5b4bc2e
fix import
m30m Dec 18, 2024
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
2 changes: 2 additions & 0 deletions auction-server/src/auction/service/simulator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ impl Simulator {
Ok(result.value)
}

/// Fetches multiple accounts from the RPC in chunks
/// There is no guarantee that all the accounts will be fetched with the same slot
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is still weird, but i don't have a great solution for it atm. maybe highlight this as a TODO to think about downstream simulation issues

Copy link
Contributor

Choose a reason for hiding this comment

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

if the max rpc account limit is 100, should be fine though? max of 64 accounts per transaction, where did you get the max limit of 100 from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on the docs:
https://solana.com/docs/rpc/http/getmultipleaccounts

We try to fetch all the accounts needed for multiple transactions, so we might need chunking.

async fn get_multiple_accounts_chunked(
&self,
keys: &[Pubkey],
Expand Down
38 changes: 27 additions & 11 deletions auction-server/src/auction/service/verification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,10 @@ use {
U256,
},
},
litesvm::types::FailedTransactionMetadata,
solana_sdk::{
address_lookup_table::state::AddressLookupTable,
clock::Slot,
commitment_config::CommitmentConfig,
compute_budget,
instruction::CompiledInstruction,
Expand Down Expand Up @@ -579,7 +581,28 @@ impl Service<Svm> {

pub async fn simulate_bid(&self, bid: &entities::BidCreate<Svm>) -> Result<(), RestError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think calling this function recursively will make the code more readable (instead of having loop)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also considered that but that was also a bit weird. Refactored a bit to make it more readable

const RETRY_LIMIT: usize = 5;
const RETRY_DELAY: Duration = Duration::from_millis(100);
let mut retry_count = 0;
let bid_slot = bid.chain_data.slot.unwrap_or_default();

let should_retry = |result_slot: Slot,
retry_count: usize,
err: &FailedTransactionMetadata|
-> bool {
if result_slot < bid_slot && retry_count < RETRY_LIMIT {
tracing::warn!(
"Simulation failed with stale slot. Simulation slot: {}, Bid Slot: {}, Retry count: {}, Error: {:?}",
result_slot,
bid_slot,
retry_count,
err
);
true
} else {
false
}
};

loop {
let response = self
.config
Expand All @@ -593,18 +616,9 @@ impl Service<Svm> {
})?;
return match result.value {
Err(err) => {
if result.context.slot < bid.chain_data.slot.unwrap_or_default()
&& retry_count < RETRY_LIMIT
{
if should_retry(result.context.slot, retry_count, &err) {
tokio::time::sleep(RETRY_DELAY).await;
retry_count += 1;
tracing::warn!(
"Simulation failed with stale slot. Simulation slot: {}, Bid Slot: {} Retry count: {}, Error: {:?}",
result.context.slot,
bid.chain_data.slot.unwrap_or_default(),
retry_count,
err
);
tokio::time::sleep(Duration::from_millis(100)).await;
continue;
}
let msgs = err.meta.logs;
Expand All @@ -613,6 +627,8 @@ impl Service<Svm> {
reason: msgs.join("\n"),
})
}
// Not important to check if bid slot is less than simulation slot if simulation is successful
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is a bit confusing, i'm not sure what you're trying to clarify

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

trying to clarify why we are not checking the bid slot against result.context.slot in all cases

// since we want to fix incorrect verifications due to stale slot
Ok(_) => Ok(()),
};
}
Expand Down
Loading