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(node_framework): Support StateKeeper in the framework #1043

Merged
merged 18 commits into from
Feb 14, 2024

Conversation

popzxc
Copy link
Member

@popzxc popzxc commented Feb 12, 2024

What ❔

  • ⚠️ Docs are missing or outdated in many places -- I found myself spending too much time keeping them up to date while the framework in active development, so I gave up. Will update all the docs at once when the framework is more or less finalized.
  • ❗ Removes the ResourceProvider abstraction. State keeper task finally showed that it brings more problems than solutions. Main implication: now ordering of layers matters ("base" layers should go first), but hopefully it shouldn't cause too much problems.
  • Makes it possible to add resources to the context without
  • Creates PoolsLayer that was previously handled by ResourceProvider.
  • Creates SequencerFeeInputLayer and ObjectStoreLayer.
  • Adds notion of unique resource (to have Box<smth> as a non-shareable resource, like IO).
  • Adds support for state keeper (in form of StateKeeperLayer, MempoolIOLayer, and MainNodeBatchExecutorBuilderLayer).
  • Rewamps the main node example to look better.

Current "external" point of view for the framework:

fn main() -> anyhow::Result<()> {
    #[allow(deprecated)] // TODO (QIT-21): Use centralized configuration approach.
    let log_format = vlog::log_format_from_env();
    let _guard = vlog::ObservabilityBuilder::new()
        .with_log_format(log_format)
        .build();

    MainNodeBuilder::new()
        .add_pools_layer()?
        .add_fee_input_layer()?
        .add_object_store_layer()?
        .add_metadata_calculator_layer()?
        .add_state_keeper_layer()?
        .add_healthcheck_layer()?
        .build()
        .run()?;

    Ok(())
}

@popzxc popzxc requested review from slowli and EmilLuta February 14, 2024 07:24
@popzxc popzxc marked this pull request as ready for review February 14, 2024 07:24
EmilLuta
EmilLuta previously approved these changes Feb 14, 2024
Copy link
Contributor

@EmilLuta EmilLuta left a comment

Choose a reason for hiding this comment

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

I think there's some refactoring that could be left outside this PR, like injecting the ConnectionPool in MempoolFetcher. I know it's super annoying to make smaller PRs when CI is a PITA, but would still appreciate it when possible.

On documentation, I think it's the right call. Make sure there's a task to come back to it so it doesn't get lost.

@popzxc popzxc added this pull request to the merge queue Feb 14, 2024
/// The resource provider can be dynamic, however. For example, we can define a resource provider which may use
/// different config load scheme (e.g. env variables / protobuf / yaml / toml), and which resources to provide
/// (e.g. decide whether we need MempoolIO or ExternalIO depending on some config).
#[derive(Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

not debug anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, will fix in the next PR!

Merged via the queue into main with commit a80fff2 Feb 14, 2024
37 checks passed
@popzxc popzxc deleted the popzxc-state-keeper-task branch February 14, 2024 12:06
@@ -7,7 +7,7 @@ use crate::service::StopReceiver;

/// A task implementation.
#[async_trait::async_trait]
pub trait Task: 'static + Send + Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not Sync anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't have to be Sync in fact (no task is accessed from two thread simultaneously), so it was an incorrect bound.

github-merge-queue bot pushed a commit that referenced this pull request Feb 16, 2024
🤖 I have created a release *beep* *boop*
---


##
[20.7.0](core-v20.6.0...core-v20.7.0)
(2024-02-16)


### Features

* Add input field to CallRequest
([#1069](#1069))
([5087121](5087121))
* **api:** Remove unused and obsolete token info
([#1071](#1071))
([e920897](e920897))
* Better errors for JsonRPC calls
([#1002](#1002))
([079f999](079f999))
* **commitment:** Commitment component
([#1024](#1024))
([60305ba](60305ba))
* **en:** Make snapshots applier resilient and process storage log
chunks in parallel
([#1036](#1036))
([805218c](805218c))
* **node_framework:** Resources and layers for ETH clients
([#1074](#1074))
([776337a](776337a))
* **node_framework:** Support StateKeeper in the framework
([#1043](#1043))
([a80fff2](a80fff2))


### Bug Fixes

* **api:** Return on duplicate earlier
([#1059](#1059))
([cfa5701](cfa5701))
* **contract-verifier:** Use optimizer mode in solidity-single-file
verification
([#1079](#1079))
([fdab638](fdab638))
* Token distribution
([#1051](#1051))
([bd63b3a](bd63b3a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants