-
Notifications
You must be signed in to change notification settings - Fork 0
[feat] adds simulation to block loop #71
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
base: main
Are you sure you want to change the base?
Conversation
- shifts the tx-poller to a more actor oriented approach by streaming transactions out of the cache. - transaction deduplication and validation is intended to be carried out by the simulator.
* fix: break loop on closure and improve tracing * refactor: break out the task future
- adds init4 metrics - updates provider types to account for alloy changes
- align to alloy @ 0.12.6 - transfer off of zenith-rs and use signet-sdk instead
ac4bbb1
to
47a2ed7
Compare
47a2ed7
to
1ee991f
Compare
} | ||
|
||
/// polls the tx-pool for unique transactions and evicts expired transactions. | ||
/// unique transactions that haven't been seen before are sent into the builder pipeline. | ||
/// Polls the transaction cache for transactions. | ||
pub async fn check_tx_cache(&mut self) -> Result<Vec<TxEnvelope>, Error> { |
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.
FYI, this PR pulls this functionality into the SDK - but no need to block on it
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.
Will address in a follow-up PR 👍
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.
we are going places 😎
|
||
let port = config.builder_port; | ||
let server = serve_builder_with_span(([0, 0, 0, 0], port), span); | ||
|
||
select! { | ||
_ = tx_poller_jh => { | ||
tracing::info!("tx_poller finished"); |
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.
i miss is cooked
:(
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.
is cooked
got cooked
pub config: BuilderConfig, | ||
/// Reqwest client for fetching transactions from the tx-pool | ||
/// Reqwest Client for fetching transactions from the cache. |
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.
we can replace this with TxCache
after init4tech/signet-sdk#39
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.
Will address in a follow up PR 👍
- adds newlines after arguments and returns sections in comments - pulls several functions out into methods
…ng with env config
- refactors the rollup provider type from a FillProvider to a RootProvider - defines the alloy database provider type around this - updates relevant connection functions to use the new RuProvider type - renames Provider to HostProvider - renames WalletlessProvider to RuProvider
src/consts.rs
Outdated
@@ -0,0 +1,4 @@ | |||
//! Constants used in the builder. |
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.
mega nit: let's just call this file constants.rs
since that's the usual name elsewhere
EDIT: well just saw on the prev review that this was the suggestion. feel free to ignore
|
||
let port = config.builder_port; | ||
let server = serve_builder_with_span(([0, 0, 0, 0], port), span); | ||
|
||
select! { | ||
_ = tx_poller_jh => { | ||
tracing::info!("tx_poller finished"); |
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.
is cooked
got cooked
/// | ||
/// A `JoinHandle` for the basefee updater and a `JoinHandle` for the | ||
/// cache handler. | ||
pub fn spawn_cache_task( |
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.
nit: this actually spawns 2 tasks
AlloyDB::new(self.ru_provider.clone(), BlockId::from(latest)); | ||
let wrapped_db: WrapDatabaseAsync<AlloyDB<Ethereum, RuProvider>> = | ||
WrapDatabaseAsync::new(alloy_db).unwrap_or_else(|| { | ||
panic!("failed to acquire async alloy_db; check which runtime you're using") |
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.
this errorr only occurs on current thread runtimes, so we can actually rule it out entirely by setting the runtime flavor in the main macro link
db, | ||
constants, | ||
PecorinoCfg {}, | ||
NoopBlock, |
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.
can't use a noop-block here. need to load a header and modify in a way similar to what happens here:
https://github.com/init4tech/signet-sdk/blob/main/crates/rpc/src/ctx.rs#L500-L523
|
||
// Update the basefee on a per-block cadence | ||
let basefee_jh = | ||
tokio::spawn(async move { self.basefee_updater(Arc::clone(&basefee_price)).await }); |
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.
this arc clone is unnecessary, as we already cloned above
async fn check_basefee(&self, price: &Arc<AtomicU64>) { | ||
tracing::debug!("checking latest basefee"); | ||
let resp = self.ru_provider.get_block_by_number(Latest).await; | ||
if let Err(e) = resp { |
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.
I think this fn could be cleaned up like this
resp.inspect_err(|e| tracing::debug!(...));
if let Ok(Some(resp)) = resp {
...
}
let _ = submit_sender.send(block); | ||
} | ||
Err(e) => { | ||
tracing::error!(err = %e, "failed to send block"); |
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.
failed to send, or failed to build?
/// | ||
/// An `Instant` representing the deadline. | ||
pub fn calculate_deadline(&self) -> Instant { | ||
let now = SystemTime::now(); |
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.
i don't like that we're constantly mixing timestamps and Duration/Instant. it makes it very hard to reason about the logic. cc @anna-carroll can we add more convenience functions to SlotCalculator
to make these easier? what are the common actions we want to take using a calculator?
// add a buffer to the beginning of the block slot. | ||
fn secs_to_next_target(&self) -> u64 { | ||
self.secs_to_next_slot() + self.config.target_slot_time | ||
Instant::now().checked_add(Duration::from_secs(unix_seconds)).unwrap() |
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.
checked_add().unwrap()
is a little redundant
in_progress_block.ingest_bundle(bundle); | ||
/// Configuration struct for Pecorino network values. | ||
#[derive(Debug, Clone)] | ||
pub struct PecorinoCfg {} |
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.
if no probs, use a simple structdef without the {}
pub struct PecorinoCfg;
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.
also should impl Copy
pub fn setup_test_config() -> Result<BuilderConfig> { | ||
let config = BuilderConfig { | ||
host_chain_id: 17000, | ||
ru_chain_id: 17001, |
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.
do these need to match the Pecorino chain id used below?
[feat] adds simulation to block loop
This PR adds simulation to the block builder loop.
SlotCalculator
for managing slot timings.Configuration
Deploying this PR will require additional environment variables to be set
CONCURRENCY_LIMIT
to limit how many concurrent simulations the block builder runs.START_TIMESTAMP
to set the starting timestamp of the chain for anchoring slot calculation.Testing
This code is still being tested before final merge.
The current test plan is to