feat(based-op-reth): based-op changes for reth without fork #266
Conversation
pamungkaski
changed the title
mempirate
left a comment
mempirate
left a comment
There was a problem hiding this comment.
Nice, this is looking like a good base. Implementing the executor as in the flashblocks example would be a good starting point, after that we can start looking at how to serve it at the RPC level and how to integrate canonical blocks (should be fairly easy). Let's do the frag executor first since it's probably the most difficult.
based/crates/reth/src/exec.rs
Outdated
|
|
||
| /// A very small “dummy” executor so you can compile & test state machine early. | ||
| /// Replace with Reth executor. | ||
| pub struct NoopExecutor; |
There was a problem hiding this comment.
| type Resp<T> = oneshot::Sender<Reply<T>>; | ||
|
|
||
| #[derive(Debug)] | ||
| enum Reply<T> { |
There was a problem hiding this comment.
Why not just use result directly? Or type Reply<T> = Result<T, DriverError>
based/crates/reth/src/driver.rs
Outdated
| async fn handle_forkchoice_updated(&mut self, new_block_number: u64) -> Result<(), DriverError> { | ||
| self.fcu_count_since_unseal_reset += 1; | ||
|
|
||
| let Some(ub) = self.current_unsealed_block.as_ref() else { |
There was a problem hiding this comment.
Could you add references to the original based op geth code in a comment? So it's easier to cross-reference the implementations
|
|
||
| self.exec.set_canonical(&presealed_block).await?; | ||
|
|
||
| self.reset_current_unsealed_block(); |
There was a problem hiding this comment.
I'm not sure how this is actionable, but I see a lot of these resets sprinkled in the handlers.
I'd expect it to be present on the caller of these handlers, so the handlers mostly need to worry about the "happy path" and errors are handled upstream...
But again, unsure how actionable this is, but it felt weird so I had to leave a comment
|
|
||
| /// Execute all txs in `frag` on top of current overlay state. | ||
| /// | ||
| /// MUST be cumulative: txs execute after all previous frags's txs. |
There was a problem hiding this comment.
Hmm I'd probably either pass in the full set of txs or have it already included in the unsealed block (which I think is what we do here)
There was a problem hiding this comment.
yea we are moving the unsealedbloc inside executor so we are only going to pass frag
There was a problem hiding this comment.
just like on geth
There was a problem hiding this comment.
Then in that case this doc is outdated
Co-authored-by: Ki Ageng Satria Pamungkas <ageng.satria@bukalapak.com> Co-authored-by: Jonas Bostoen <jonasbostoen@fastmail.com>
Karrq
left a comment
Karrq
left a comment
There was a problem hiding this comment.
LGTM. There are a few differences with Geth, but overall I think it's more complete so it's good differences
| #[method(name = "getBlockByNumber")] | ||
| async fn block_by_number(&self, number: BlockNumberOrTag, full: bool) -> RpcResult<Option<RpcBlock<Optimism>>>; | ||
|
|
||
| /// Returns the transaction receipt for a given transaction hash, with support for frags. |
There was a problem hiding this comment.
nit: technically the trait doesn't need to be "aware of frags" (as is the case, looking at the signatures) so I'd rather have this documented in the implementor of the trait
| timeout_ms: Option<u64>, | ||
| ) -> RpcResult<RpcReceipt<Optimism>>; | ||
|
|
||
| /// Executes a call with flashblock state support. |
There was a problem hiding this comment.
| /// Executes a call with flashblock state support. | |
| /// Executes a call with unsealed block state support. |
Plus others below
There was a problem hiding this comment.
(even this, I'd say can be omitted as it's not part of the API)
based/crates/reth/src/api/eth.rs
Outdated
| if self.use_unsealed_state(&number) && | ||
| let Some(unsealed_block) = self.unsealed_block.load_full() | ||
| { | ||
| Ok(Some(unsealed_block.to_rpc_block(full))) |
There was a problem hiding this comment.
I don't think Geth supports this. I can see that they retrieve the miner block for pending, but also they just retrieve the current block if it's the "latest", which doesn't resolve to the unsealed block even with the flag
There was a problem hiding this comment.
Yep, I think this endpoint shouldn't use unsealed block at all. Latest and pending can just return canonical block
There was a problem hiding this comment.
changed, thanks!
| address = %address | ||
| ); | ||
| let block_id = block_number.unwrap_or_default(); | ||
| if self.use_unsealed_state(&block_id) { |
There was a problem hiding this comment.
Same here, Geth doesn't resolve block header/number from unsealed state
| let mut count = | ||
| EthState::transaction_count(&self.canonical, address, block_number).await.map_err(Into::into)?; | ||
|
|
||
| if self.use_unsealed_state(&block_id) { |
|
|
||
| let ub_arc = ub_arc_opt.ok_or(ExecError::NotInitialized)?; | ||
|
|
||
| // Make an owned, mutable working copy from the start |
There was a problem hiding this comment.
| // Make an owned, mutable working copy from the start | |
| // Make a local working copy for the frag simulation |
| } | ||
| } | ||
|
|
||
| db = evm.into_db(); |
There was a problem hiding this comment.
| db = evm.into_db(); | |
| // Update state views of the local unsealed block and | |
| // commit frag execution to it | |
| db = evm.into_db(); |
|
|
||
| ub.accept_frag_execution(frag, logs, receipts, gas_used); | ||
|
|
||
| self.current_unsealed_block.store(Some(Arc::new(ub))); |
There was a problem hiding this comment.
| self.current_unsealed_block.store(Some(Arc::new(ub))); | |
| // Finally, replace shared unsealed block with the updated one | |
| self.current_unsealed_block.store(Some(Arc::new(ub))); |
| fn seal(&mut self) -> Result<(), ExecError> { | ||
| let ub = self.current_unsealed_block.load_full().ok_or(ExecError::NotInitialized)?; | ||
| let withdrawals_hash = if ub.is_prague { | ||
| let canonical_block = ub.env.number.saturating_sub(1); |
There was a problem hiding this comment.
This was in another place with a TODO to check if it was correct. Worth adding a method to retrieve the canonical block from the unsealed block? Just to avoid fixing only one (if it's indeed incorrect)
Co-authored-by: Karrq <franci.dainese@gmail.com>
No description provided.