Shared sequencer integration for fuel-core 0.40.0
#2451
Conversation
|
|
||
| let block = self.tendermint()?.block(height).await?; | ||
|
|
||
| for item in block.block.data.iter().skip(1) { |
There was a problem hiding this comment.
@Dentosal Added it, so I don't know=) I will remove this function for now, since it is not used
|
|
||
| impl Client { | ||
| /// Create a new shared sequencer client from config. | ||
| pub async fn new(endpoints: Endpoints, topic: [u8; 32]) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
i think it should also accept the expected client_id so that we don't misconfigure with the wrong network/chain.
on L68 we can perform the check
There was a problem hiding this comment.
What is the client_id? Do you mean chain_id? I want to keep the current implementation as simple, as possible.
Plus, if the RPC is malicious, it can lie about its chain_id
|
|
||
| let k256_public_key = | ||
| k256::PublicKey::from_public_key_der(cached_public_key_bytes)?; | ||
| Ok(Some(k256_public_key.into())) |
There was a problem hiding this comment.
q: why don't we just set the public key into the enum variant instead of its raw form? we won't have to recalculate it when verifying_key() is called every time.
as i understand the code, make_payload() is leading to this function being called repeatedly
There was a problem hiding this comment.
It is more question to @Dentosal =) Performance is not critical in make_payload because we call this function twice each 12 seconds
| use std::sync::Arc; | ||
|
|
||
| /// Non-initialized shared sequencer task. | ||
| pub struct NonInitializedTask<S> { |
There was a problem hiding this comment.
Nit: I suggest renaming this to UninitializedTask to maintain parity with #2442
| // Ceil the gas price to the next integer. | ||
| let gas_price = gas_price.saturating_add(1); |
There was a problem hiding this comment.
What's the purpose of incrementing the gas price here? The comment says we're ceiling it, but it's already parsed as a u128 so that's not correct. If we get a float number in minimum_gas_price.parse(), we'll get an error, not a floored value.
| fn try_from(val: Args) -> anyhow::Result<fuel_core_shared_sequencer::Config> { | ||
| let endpoints = match (val.tendermint_api, val.blockchain_api) { | ||
| (Some(tendermint_api), Some(blockchain_api)) => { | ||
| Some(fuel_core_shared_sequencer::Endpoints { | ||
| tendermint_rpc_api: tendermint_api, | ||
| blockchain_rest_api: blockchain_api, | ||
| }) | ||
| } | ||
| (None, None) => None, | ||
| _ => { | ||
| return Err(anyhow::anyhow!( | ||
| "Both tendermint and blockchain API must be set or unset" | ||
| )) | ||
| } | ||
| }; | ||
|
|
||
| Ok(fuel_core_shared_sequencer::Config { | ||
| enabled: val.enable, | ||
| block_posting_frequency: val.block_posting_frequency.into(), | ||
| endpoints, | ||
| topic: *val.topic, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I think we should allow the case where enable is false and the tendermint_api and blockchain_api are unset. That seems to be the purpose of having them be optional afaiu, otherwise we could just make tendermint_api and blockchain_api be required arguments and skip this match statement.
Co-authored-by: Mårten Blankfors <[email protected]>
## Version v0.40.1 - [2450](#2450): Added support for posting blocks to the shared sequencer. ## What's Changed - Shared sequencer integration for `fuel-core 0.40.0` by @xgreenx in #2451 **Full Changelog**: v0.40.0...v0.40.1
Description
This PR duplicates #1922, but on top of the
fuel-core 0.40.0.Since the current
masteris not ready to be released, but we want this feature in the testnet, we will create a minor release with added off-chain logic that posts data to the SS.All changes are the same as in the original PR, except
reqwestis bumped to0.12, because it has a fix important for SS.Before requesting review