Shared sequencer integration #1922
Conversation
# Conflicts: # CHANGELOG.md # Cargo.lock
Converted SS client into service. Fixed the issues with getting Verifing key from default consensus parameters.
Fetch most of parameters from the network itself. Estimate transaction before sending. Fixed the issue with wrong signature.
…ss-enable, endpoints).
## Description This PR duplicates #1922, but on top of the `fuel-core 0.40.0`. Since the current `master` is 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 `reqwest` is bumped to `0.12`, because it has a fix important for SS. ### Before requesting review - [x] I have reviewed the code myself --------- Co-authored-by: Mårten Blankfors <[email protected]>
# Conflicts: # Cargo.lock # Cargo.toml
| @@ -31,7 +31,7 @@ cargo make check --all-features --locked && | |||
| cargo make check --locked && | |||
| OVERRIDE_CHAIN_CONFIGS=true cargo test --test integration_tests local_node && | |||
| cargo nextest run --workspace && | |||
| FUEL_ALWAYS_USE_WASM=true cargo nextest run --all-features --workspace && | |||
| cargo nextest run --all-features --workspace && | |||
There was a problem hiding this comment.
What's the rationale behind this change on this PR ?
There was a problem hiding this comment.
A mistake. Removed in b3ea605.
| #[cfg(feature = "shared-sequencer")] | ||
| { | ||
| expected_services += 1; | ||
| } |
There was a problem hiding this comment.
not for this PR, but maybe it's better here to have an enum that lists all the expected services, with feature-specific variants, and then use strum_enum::count to retrieve the number of expected services.
|
I have reviewed the whole code. From the architectural point of view I would like to understand the following (note that my questions probably spill into the shared sequencer domain):
|
|
My understanding is rather imperfect here, so these answers might not be 100% correct.
It's shared sequencer is tendermint, with a sidecar that implements Fuel-specific logic. I don't think we verify that the blocks are correct, we just post them there.
Nothing is validated. The data we send is included as-is.
That's the plan, yes. I'm not sure if they do that already. |
Dentosal
dismissed stale reviews from acerone85, AurelienFT, and xgreenx
via
b3ea605
| _: &StateWatcher, | ||
| _: Self::TaskParams, | ||
| ) -> anyhow::Result<Self::Task> { | ||
| let shared_sequencer_client = if let Some(endpoints) = &self.config.endpoints { |
There was a problem hiding this comment.
Is there a reason we (try to) create the client even though the self.config.enabled could be false?
| }; | ||
|
|
||
| let mut account = self.account_metadata.take().expect("Account metadata is not set"); | ||
| let next_order = self.prev_order.map(|prev| prev.wrapping_add(1)).unwrap_or(0); |
There was a problem hiding this comment.
It seems that ensure_prev_order doesn't really ensure that we have the "prev_order", because the http_api::get_topic can still return None.
It's probably fine to start at 0 in this case, but I'm just leaving the comment for others to double-check :)
| return TaskNextAction::ErrorContinue(err); | ||
| } | ||
|
|
||
| tracing::info!("Posted block to shared sequencer {blobs:?}"); |
There was a problem hiding this comment.
How many blobs we can potentially have in the collection? Asking because we may risk clogging the log with super long messages on info level. Maybe logging the .len() would suffice? Or something like range: "blobs.first().height, blobs.last().height"?
| // We don't want our transactions to be stay in the mempool for a long time, | ||
| // so we set the timeout height to be 64 blocks ahead of the latest block height. | ||
| // The 64 is an arbitrary number. | ||
| latest_height.saturating_add(64), |
There was a problem hiding this comment.
Do we know at what pace the sequencer chain produces blocks?
| // Convert the signature to non-normalized form | ||
| let mut signature_bytes = *signature; | ||
| signature_bytes[32] &= 0x7f; |
There was a problem hiding this comment.
A nit: It could be a little bit more clear if we have the fn non_normalized() added to impl Signature.
| if let Some(index) = minimum_gas_price.find('.') { | ||
| minimum_gas_price.truncate(index); | ||
| } | ||
| let gas_price: u128 = minimum_gas_price.parse()?; | ||
| // Ceil the gas price to the next integer. | ||
| let gas_price = gas_price.saturating_add(1); |
There was a problem hiding this comment.
Do we have a rationale for this piece of code. I was wondering:
- why we truncate? (and how can we be sure that dot will be used as decimal separator)?
- why we ceil to the next integer?
Related #1621
As part of the feature, we created a new crate https://github.com/FuelLabs/sequencer-rs-sdk with protobuf types from the SS.
The PR doesn't have any testing currently. The
fuel-corewas tested with enabled SS functionality on the local node created from https://github.com/FuelLabs/fuel-rollup/blob/main/docker/docker-compose.yml, and on the SS testnet using data from https://github.com/fuel-infrastructure/networks/tree/main/seq-testnet-1.We need to add integration tests and unit tests in the follow-up PR.
Checklist
Before requesting review