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.

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: --debug.etherscan for fake consensus client #8082

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sevazhidkov
Copy link
Contributor

This is an early draft for fake consensus client that fetches blocks from Etherscan and propagates it to execution layer as FCUs and new payloads. Designed to replace --debug.tip when developing. #6711

@shekhirin shekhirin added C-enhancement New feature or request A-consensus Related to the consensus engine labels May 3, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice, left some suggestions

Comment on lines 56 to 70
let block: EtherscanBlockResponse = self
.http_client
.get(&self.etherscan_base_api_url)
.query(&[
("module", "proxy"),
("action", "eth_getBlockByNumber"),
("tag", "latest"),
("boolean", "true"),
("apikey", &self.etherscan_api_key),
])
.send()
.await
.unwrap()
.json()
.await
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to make this configurable, for example I think it could be useful to provide another node's rpc endpoint and use eth_subscribe + eth_getBlockByHash to fetch the block

so perhaps a stream type helper that yields full blocks

this is perhaps a better solution that just etherscan,
see for example:

https://github.com/alloy-rs/examples/blob/51934948a20c6edadd1ca5ab04726962ee153d38/examples/providers/examples/builtin.rs#L16

Comment on lines 97 to 101
reth_rpc_types::engine::ForkchoiceState {
head_block_hash: block_hash,
safe_block_hash: block_hash,
finalized_block_hash: block_hash,
},
Copy link
Member

Choose a reason for hiding this comment

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

this might break certain assumptions about blocks that can (or cannot in this case) be reorged. i'd prefer that API call above was a batch call that also fetches safe & finalized hashes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good callout. I haven't found a way to get safe/finalized blocks state from Etherscan, because their eth_getBlockByNumber doesn't support tags other than latest. maybe you know some tricks? afaik beaconscan isn't exposed in API.

otherwise, should I just change the --debug.etherscan parameter to --debug.consensus-rpc and use its eth_getBlockByNumber?

Copy link
Member

Choose a reason for hiding this comment

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

The safe block hash will be at most 32 blocks behind head, and finalized is at most 32 behind that, so you could use head, head-32, head-64 after fetching the three hashes

/// by number to fetch past finalized and safe blocks.
pub trait BlockProvider: Send + Sync + 'static {
/// Spawn a block provider to send new blocks to the given sender.
fn spawn(&self, tx: mpsc::Sender<RichBlock>) -> impl Future<Output = ()> + Send;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you prefer, this or impl Stream<Output = RichBlock>? IMO impl Stream<Output = RichBlock> is much less nicer to implement (bc of manual state machine building vs just async fn), but can change it

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

nice! I'd prefer to see this generalised to any node infrastructure service that uses API keys, not only Etherscan. wdyt @shekhirin @mattsse @rkrasiuk ?

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this should work,

one question and nit,

need to think about the trait a bit more, but I see why the spawn is preferable.

crates/consensus/debug-client/src/client.rs Show resolved Hide resolved
// Load previous block hashes. We're using (head - 32) and (head - 64) as the safe and
// finalized block hashes.
// todo: chdeck for off by ones in offset
let safe_block_hash = get_or_fetch_previous_block(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be a function of self

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, if understood this comment correctly. if not — ping again pls.

@sevazhidkov sevazhidkov marked this pull request as ready for review May 17, 2024 04:26
@sevazhidkov sevazhidkov requested a review from mattsse May 17, 2024 04:34
@sevazhidkov
Copy link
Contributor Author

@mattsse highlighting this PR just in case it slipped through the cracks!

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this is great!

I made some smol edits re naming, derives, ptal.

and only have a few more pedantic nits re the cli args

conflicts_with = "tip",
conflicts_with = "rpc_consensus_http"
)]
pub etherscan: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should take an Option<Option<String>> if Some(Some(api_url)) use that, if Some(None) use the chain's default

conflicts_with = "tip",
conflicts_with = "rpc_consensus_http"
)]
pub etherscan: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should take an Option<Option<String>> if Some(Some(api_url)) use that, if Some(None) use the chain's default

Comment on lines +41 to +61
/// Runs a fake consensus client using blocks from an RPC endpoint. HTTP endpoint for getting
/// past blocks.
#[arg(
long = "debug.rpc-consensus-http",
help_heading = "Debug",
conflicts_with = "tip",
conflicts_with = "etherscan",
requires = "rpc_consensus_ws"
)]
pub rpc_consensus_http: Option<String>,

/// Runs a fake consensus client using blocks from an RPC endpoint. WS endpoint for getting new
/// blocks.
#[arg(
long = "debug.rpc-consensus-ws",
help_heading = "Debug",
conflicts_with = "tip",
conflicts_with = "etherscan",
requires = "rpc_consensus_http"
)]
pub rpc_consensus_ws: Option<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

technically we don't need both endpoints, but I'm fine with this limitation for now, but a single arg is sufficient

Comment on lines +451 to +456
.expect("etherscan urls not found for rpc consensus client")
.0
.to_owned(),
chain
.etherscan_api_key()
.expect("etherscan api key not found for rpc consensus client"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

these should be errors not panics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Related to the consensus engine C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants