Update state-fold use on eth-input-reader #59

GMKrieger · 2023-08-23T13:34:02Z

CHANGELOG.md

offchain/eth-input-reader/src/setup.rs

gligneul

I will do a full review tomorrow.

gligneul · 2023-09-19T22:13:47Z

CHANGELOG.md

@@ -10,13 +10,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added

 - Added authority claimer service to support reader mode
+- Added `SFConfig`, `BHConfig` and `subscription_depth` configuration to `eth-input-reader`


The internal names are not helpful for those who use the node. We should add the full env var names here.

Here is my proposal to the changelog:

### Changed - Replaced the state-server with the state-fold library in the eth-input-reader - Moved `SF_*` and `BH_*` environment variables from state-server config to the eth-input-reader ### Removed - Removed the state-server from the node - Removed SS_MAX_DECODING_MESSAGE_SIZE environment variable from eth-input-reader because it doesn't use gRPC anymore

gligneul · 2023-09-20T22:08:58Z

offchain/eth-input-reader/src/config.rs

@@ -43,25 +45,28 @@ pub struct EthInputReaderEnvCLIConfig {
    /// Duration of rollups epoch in seconds, for which eth-input-reader will make claims.
    #[arg(long, env, default_value = "604800")]
    pub rd_epoch_duration: u64,
+
+    /// Depth on the blockchain the reader will be listening to
+    #[arg(long, env)]


We should put the default value here, like the other variables.

Furthermore, the chain_id has to be verified. Either one should just gets it from the blockchain, no configuration needed, or one gets it from the config but validates it nonetheless. Although I lean more towards the latter, here I think either is acceptable because we're not sending transactions.

Also, a default value would be nice.

Finally, why do we need the chain_id? For metrics?

Signing needs it, but I'm not sure this component does.

gligneul · 2023-09-20T22:10:26Z

offchain/eth-input-reader/src/config.rs


    pub dapp_deployment: DappDeployment,
    pub rollups_deployment: RollupsDeployment,
    pub epoch_duration: u64,
    pub priority: Priority,
+    pub subscription_depth: usize,


Is this the equivalent of the former variable SC_DEFAULT_CONFIRMATIONS? I think it is, but I'm not sure. If it is, we should use the same name.

It is, I changed because SC_DEFAULT_CONFIRMATIONS is a terrible (and deprecated) name. SC stands for State Client, which we no longer use, and default confirmations really poorly instructs the user that we are asking which depth we should be reading, imho. It can be another name other than this, but I really think we should change this.

Ok, it makes sense. Could you add this change to the backlog?

offchain/eth-input-reader/src/config.rs

gligneul · 2023-09-20T22:13:11Z

offchain/eth-input-reader/src/machine/driver.rs

+use eth_state_fold_types::Block;
+use ethers::types::Address;


I didn't get this change. Why do we need to change this type?

The type isn't changed, but before the state-fold-types re-exported the ethers crate, and we called the ethers types from inside state-fold-types. With the refactoring, we use state-fold-types only for the types defined in it, and import the ethers types directly from itself.

The state-fold is an abstraction on top of ethers, so it makes sense to use the re-exported type instead of importing it from ethers directly.

Me and Renan have been refactoring to stop this using ethers from inside state-fold, he was very adamant about it. I can change it back, no problem, but it's going diverge from the rest of the refactoring. I think it's worth to have a talk between the three of us to understand why this should change/remain the same. @renan061

Yeah, I'd be really cautious about not using reexported ethers-rs.

gligneul · 2023-09-20T22:15:13Z

offchain/eth-input-reader/src/setup.rs

        .await
-        .context(StateServerSnafu)?
+        .expect("should get genesis block")


We should handle errors properly with Snafu instead of using expect.

gligneul · 2023-09-20T23:02:08Z

CHANGELOG.md

@@ -10,13 +10,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added

 - Added authority claimer service to support reader mode
+- Added `SFConfig`, `BHConfig` and `subscription_depth` configuration to `eth-input-reader`

 ### Changed

 - Renamed `dispatcher` service to `eth-input-reader`

 ### Removed


Let's remove the state-server from the repository in this PR because it won't be used anymore.

gligneul · 2023-09-20T23:03:32Z

offchain/eth-input-reader/src/setup.rs

+        config.sf_config.genesis_block,
+        config.sf_config.query_limit_error_codes.clone(),
+        config.sf_config.concurrent_events_fetch,
+        10000,


What is this magic number? We should at least create a local const magic_number = 10000.

I got this magic number directly from state-server, where is just as magical, you can check on the state-server/src/lib.rs at line 92. I checked the StateFoldEnvironment and copied the comment from there above the const inside the setup.

gligneul · 2023-09-20T23:04:35Z

@GCdePaula, if you have some spare time, could you look at this PR?

GCdePaula

Nice!

I left a few comments. Also, I'm not sure what's the timeline here, and there's the Go node stuff. So maybe some of the comments are not really relevant. Feel free to ignore the minor and more abstract comments.

GCdePaula · 2023-10-27T18:44:55Z

offchain/eth-input-reader/src/eth_input_reader.rs

-        .query_state(initial_state, block.hash)
-        .await
-        .context(StateServerSnafu)?;
+    let state = RollupsState::get_state_for_block(


I believe with this change you don't need the whole RollupsState, instead just the InputBox.

GCdePaula · 2023-10-27T18:49:24Z

offchain/eth-input-reader/src/config.rs

@@ -43,25 +45,28 @@ pub struct EthInputReaderEnvCLIConfig {
    /// Duration of rollups epoch in seconds, for which eth-input-reader will make claims.
    #[arg(long, env, default_value = "604800")]
    pub rd_epoch_duration: u64,
+
+    /// Depth on the blockchain the reader will be listening to
+    #[arg(long, env)]


Furthermore, the chain_id has to be verified. Either one should just gets it from the blockchain, no configuration needed, or one gets it from the config but validates it nonetheless. Although I lean more towards the latter, here I think either is acceptable because we're not sending transactions.

Also, a default value would be nice.

GCdePaula · 2023-10-27T18:49:57Z

offchain/eth-input-reader/src/config.rs

@@ -43,25 +45,28 @@ pub struct EthInputReaderEnvCLIConfig {
    /// Duration of rollups epoch in seconds, for which eth-input-reader will make claims.
    #[arg(long, env, default_value = "604800")]
    pub rd_epoch_duration: u64,
+
+    /// Depth on the blockchain the reader will be listening to
+    #[arg(long, env)]


Finally, why do we need the chain_id? For metrics?

Signing needs it, but I'm not sure this component does.

GCdePaula · 2023-10-27T18:52:29Z

offchain/eth-input-reader/src/config.rs

+    pub chain_id: Option<u64>,
+
+    /// Depth on the blockchain the reader will be listening to
+    #[arg(long, env, default_value = "7")]


I think this should be larger. Current value makes nobody happy; 7 is neither safe nor fast. I think maybe 12? Or 0.

GCdePaula · 2023-10-27T18:53:14Z

offchain/eth-input-reader/src/error.rs

@@ -2,10 +2,11 @@
 // SPDX-License-Identifier: Apache-2.0 (see LICENSE)

 use axum::http::uri::InvalidUri;
-use eth_state_client_lib::error::StateServerError;
+use ethers::providers::{Http, Provider, RetryClient};


I would be extremely careful about not using the reexported ethers-rs. Is there a good reason for it?

No good reason I can explain well. Renan was pretty adamant about not doing it this way, and at the time Rust would still be the main project going forward. I can change it back, no problem.

GCdePaula · 2023-10-27T19:21:26Z

offchain/eth-input-reader/src/eth_input_reader.rs

-        &state_server,
-        config.sc_config.default_confirmations,
+    let mut block_subscription = create_subscription(
+        Arc::clone(&block_subscriber),


Reconsider the need of this clone.

GCdePaula · 2023-10-27T19:23:13Z

offchain/eth-input-reader/src/setup.rs

@@ -1,88 +1,116 @@
 // (c) Cartesi and individual authors (see AUTHORS)
 // SPDX-License-Identifier: Apache-2.0 (see LICENSE)



I think many functions in this file could be joined into a single one, returning just what the start function needs.

GCdePaula · 2023-10-27T19:26:20Z

offchain/eth-input-reader/src/setup.rs

    broker: &impl BrokerStatus,
    dapp_metadata: DAppMetadata,
    metrics: EthInputReaderMetrics,
 ) -> Result<Context, EthInputReaderError> {
-    let genesis_timestamp: u64 = block_server
-        .query_block(config.dapp_deployment.deploy_block_hash)
+    let genesis_timestamp: u64 = block_subscriber


I don't think this makes sense. I think you should use the provider instead.

How do I use the provider to get the genesis timestamp of a block?

GCdePaula · 2023-10-27T19:27:02Z

offchain/log/src/lib.rs

@@ -10,7 +10,7 @@ pub mod built_info {
    include!(concat!(env!("OUT_DIR"), "/built.rs"));
 }

-#[derive(Debug, Parser)]
+#[derive(Debug, Parser, Clone)]


I'd consider whether the cases you're cloning the config could instead just borrow.

GCdePaula · 2023-10-27T19:27:14Z

offchain/rollups-events/src/broker/mod.rs

@@ -346,7 +346,7 @@ pub enum BrokerError {
    InvalidPayload { source: serde_json::Error },
 }

-#[derive(Debug, Parser)]
+#[derive(Debug, Parser, Clone)]


I'd consider whether the cases you're cloning the config could instead just borrow.

The borrow requires a lifetime specifier, the flatten command also complains, and the other configs from block-history and state-fold are cloned, so I just kept the pattern.

GCdePaula

LGTM. Just some minor comments on style.

GCdePaula · 2023-11-01T20:25:06Z

offchain/eth-input-reader/src/setup.rs

+        impl Stream<
+                Item = Result<
+                    BlockStreamItem,
+                    SubscriptionError<InputProvider>,
+                >,
+            > + std::marker::Unpin,


This could be a type alias.

GCdePaula · 2023-11-01T20:26:37Z

offchain/eth-input-reader/src/setup.rs

+                    SubscriptionError<InputProvider>,
+                >,
+            > + std::marker::Unpin,
+        StateFoldEnvironment<InputProvider, Mutex<UserData>>,


Maybe this too?

GMKrieger added this to the 1.1.0 milestone Aug 23, 2023

GMKrieger requested review from renan061 and a team August 23, 2023 13:34

GMKrieger self-assigned this Aug 23, 2023

GMKrieger force-pushed the feature/remove-claims-dispatcher branch from ab8f1df to bdd2837 Compare August 24, 2023 17:08

GMKrieger force-pushed the feature/refactor-eth-input-reader branch 2 times, most recently from d5cf8f7 to eb1b900 Compare August 24, 2023 17:25

GMKrieger force-pushed the feature/remove-claims-dispatcher branch 2 times, most recently from e33f069 to 4c92ad6 Compare August 28, 2023 17:26

GMKrieger force-pushed the feature/refactor-eth-input-reader branch from eb1b900 to 568311c Compare August 28, 2023 17:30

GMKrieger force-pushed the feature/remove-claims-dispatcher branch from 4c92ad6 to 9ed8c8e Compare August 28, 2023 18:01

GMKrieger force-pushed the feature/refactor-eth-input-reader branch from 568311c to 4e3dc50 Compare August 28, 2023 18:04

gligneul linked an issue Sep 4, 2023 that may be closed by this pull request

Change how the state-fold is used in the dispatcher #41

Closed

2 tasks

torives reviewed Sep 5, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

offchain/eth-input-reader/src/setup.rs Outdated Show resolved Hide resolved

GMKrieger force-pushed the feature/refactor-eth-input-reader branch 2 times, most recently from a81e2ba to 3ce9ad1 Compare September 12, 2023 17:38

torives approved these changes Sep 19, 2023

View reviewed changes

gligneul suggested changes Sep 19, 2023

View reviewed changes

gligneul suggested changes Sep 20, 2023

View reviewed changes

gligneul requested a review from GCdePaula September 20, 2023 23:04

GMKrieger force-pushed the feature/refactor-eth-input-reader branch from 3ce9ad1 to ce5a4be Compare September 27, 2023 17:08

gligneul modified the milestones: 1.1.0, 1.2.0 Sep 28, 2023

GMKrieger force-pushed the feature/remove-claims-dispatcher branch from 9ed8c8e to 52030b1 Compare October 25, 2023 17:46

GMKrieger force-pushed the feature/refactor-eth-input-reader branch 2 times, most recently from c081e5d to bf27f7c Compare October 25, 2023 18:22

GCdePaula requested changes Oct 27, 2023

View reviewed changes

refactor(input-reader): update state-fold use

8ec82d5

GMKrieger force-pushed the feature/refactor-eth-input-reader branch from bf27f7c to 8ec82d5 Compare October 31, 2023 19:17

GCdePaula approved these changes Nov 1, 2023

View reviewed changes

GMKrieger merged commit 4f36d4a into feature/remove-claims-dispatcher Nov 3, 2023
3 of 5 checks passed

GMKrieger deleted the feature/refactor-eth-input-reader branch November 3, 2023 13:00

		@@ -1,88 +1,116 @@
		// (c) Cartesi and individual authors (see AUTHORS)
		// SPDX-License-Identifier: Apache-2.0 (see LICENSE)

Update state-fold use on eth-input-reader #59

Update state-fold use on eth-input-reader #59

Conversation

GMKrieger commented Aug 23, 2023

gligneul left a comment

Choose a reason for hiding this comment

gligneul Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

gligneul Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GCdePaula Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

GCdePaula Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

gligneul Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gligneul commented Sep 20, 2023

GCdePaula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GCdePaula Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

GCdePaula Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

GCdePaula Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GCdePaula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gligneul Sep 19, 2023 •

edited

Loading

gligneul Sep 20, 2023 •

edited

Loading

GCdePaula Oct 27, 2023 •

edited

Loading

GCdePaula Oct 27, 2023 •

edited

Loading

gligneul Sep 20, 2023 •

edited

Loading

GCdePaula Oct 27, 2023 •

edited

Loading

GCdePaula Oct 27, 2023 •

edited

Loading

GCdePaula Oct 27, 2023 •

edited

Loading