-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -578,25 +578,43 @@ impl Service<Svm> { | |
} | ||
|
||
pub async fn simulate_bid(&self, bid: &entities::BidCreate<Svm>) -> Result<(), RestError> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
let response = self | ||
.config | ||
.chain_config | ||
.simulator | ||
.simulate_transaction(&bid.chain_data.transaction) | ||
.await; | ||
let result = response.map_err(|e| { | ||
tracing::error!("Error while simulating bid: {:?}", e); | ||
RestError::TemporarilyUnavailable | ||
})?; | ||
match result.value { | ||
Err(err) => { | ||
let msgs = err.meta.logs; | ||
Err(RestError::SimulationError { | ||
result: Default::default(), | ||
reason: msgs.join("\n"), | ||
}) | ||
} | ||
Ok(_) => Ok(()), | ||
const RETRY_LIMIT: usize = 5; | ||
let mut retry_count = 0; | ||
loop { | ||
let response = self | ||
.config | ||
.chain_config | ||
.simulator | ||
.simulate_transaction(&bid.chain_data.transaction) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the retry should happen within the fetching? before the simulator actually runs? otherwise it's a weird interface where we're asking for a minimum slot to simulate against and then ignoring that as long as it passes simulation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can also add some checks to make sure searchers aren't submitting slots that are more than some reasonable number off of where the rpc currently stands There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I consider this some internal behaviour which is subject to change. Interface only adds the option for a searcher to guarantee successful simulation from the specified slot. How we verify this internally is not part of the interface. I classify that as a DoS attack and don't think it's a priority There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in that case, maybe it's worth a comment baking that into the interface on the SDK side, or in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is a comment already there. do you think we should explain more? |
||
.await; | ||
let result = response.map_err(|e| { | ||
tracing::error!("Error while simulating bid: {:?}", e); | ||
RestError::TemporarilyUnavailable | ||
})?; | ||
return match result.value { | ||
Err(err) => { | ||
if result.context.slot < bid.chain_data.slot.unwrap_or_default() | ||
&& retry_count < RETRY_LIMIT | ||
{ | ||
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; | ||
Err(RestError::SimulationError { | ||
result: Default::default(), | ||
reason: msgs.join("\n"), | ||
}) | ||
} | ||
Ok(_) => Ok(()), | ||
}; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a note this could lead to accounts drawn from different slots, there may be some issues with that with regard to mistaken simulation results? it's good to use the min slot below, but there may still be issues resulting from the different results having different context slots, e.g. dex router tx simulates successfully when accounts are all synced, but may fail simulation when they are drawn at different context slots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, this can become problematic with AMM accounts. I added a comment