Str 786 main refactor part1 #561

bewakes · 2024-12-25T10:28:38Z

Description

This is the part 1 of main refactor which moves around a couple of things:

Move db init to rocksdb-store crate
Move Config to crates/config as it is where it should be. Corresponding minor changes to ReaderConfig and Args methods.
Adds NodeStorage, which at the moment consolidates two managers. It will get extended and included in the context in future.
Expose Handle from TaskManager and use it instead of passing around runtime everywhere.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests
Dependency Update

Notes to Reviewers

This is a part of bigger refactor that lies ahead and I kindly request to review this as soon as possible.

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

codecov · 2024-12-26T05:55:42Z

Codecov Report

Attention: Patch coverage is 26.43312% with 231 lines in your changes missing coverage. Please review.

Project coverage is 56.50%. Comparing base (7b9d76c) to head (78c20dd).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
bin/strata-client/src/args.rs	0.00%	82 Missing ⚠️
crates/rocksdb-store/src/lib.rs	0.00%	58 Missing ⚠️
bin/strata-client/src/main.rs	0.00%	25 Missing ⚠️
bin/strata-client/src/helpers.rs	0.00%	21 Missing ⚠️
crates/storage/src/lib.rs	0.00%	17 Missing ⚠️
crates/btcio/src/reader/config.rs	0.00%	7 Missing ⚠️
crates/tasks/src/manager.rs	0.00%	6 Missing ⚠️
crates/consensus-logic/src/sync_manager.rs	0.00%	5 Missing ⚠️
bin/strata-client/src/l1_reader.rs	0.00%	4 Missing ⚠️
crates/config/src/config.rs	95.34%	4 Missing ⚠️
... and 1 more

@@           Coverage Diff           @@
##             main     #561   +/-   ##
=======================================
  Coverage   56.50%   56.50%           
=======================================
  Files         314      316    +2     
  Lines       31957    31959    +2     
=======================================
+ Hits        18057    18059    +2     
  Misses      13900    13900

Files with missing lines	Coverage Δ
crates/bridge-relay/src/relayer.rs	`34.46% <ø> (ø)`
crates/config/src/bridge.rs	`100.00% <100.00%> (ø)`
crates/consensus-logic/src/checkpoint.rs	`0.00% <ø> (ø)`
crates/consensus-logic/src/csm/worker.rs	`0.00% <ø> (ø)`
crates/primitives/src/relay/types.rs	`61.20% <ø> (-0.22%)`	⬇️
crates/evmexec/src/fork_choice_state.rs	`0.00% <0.00%> (ø)`
bin/strata-client/src/l1_reader.rs	`0.00% <0.00%> (ø)`
crates/config/src/config.rs	`95.34% <95.34%> (ø)`
crates/consensus-logic/src/sync_manager.rs	`0.00% <0.00%> (ø)`
crates/tasks/src/manager.rs	`85.29% <0.00%> (-1.03%)`	⬇️
... and 6 more

... and 2 files with indirect coverage changes

delbonis · 2024-12-26T18:41:40Z

bin/strata-client/src/args.rs

+    pub fn derive_config(&self) -> Result<Config, String> {
+        let args = self.clone();
+        Ok(Config {
+            bitcoind_rpc: BitcoindConfig {
+                rpc_url: args
+                    .bitcoind_host
+                    .ok_or_else(|| "args: no bitcoin --rpc-url provided".to_string())?,


I would make this a toplevel function or even like from_args on Config, it doesn't really make sense to be a member function on Args. This also makes some sense to use anyhow instead of manipulating string error messages.

The idea behind this was that Args seems to be more inside the bin whereas Config seems somewhere common which other crates can/do use. To me, it doesn't make sense to pull in Arg dependency where config is defined.

I think config should not be in bin. It is just being used everywhere so it makes sense to keep it somewhere common and it should not have dependency on Args.

If we're separating the actual service config from the inner client/node config internally then yeah moving it apart makes sense.

But how these fns are organized here does feel weird to me.

delbonis · 2024-12-26T18:42:16Z

bin/strata-client/src/args.rs

+    pub fn update_config(&self, config: &mut Config) {
+        let args = self.clone();


Same here, doesn't make sense to be a member function on Args.

Same reasoning as above.

delbonis · 2024-12-26T18:42:48Z

bin/strata-client/src/args.rs

+                    rpc_url: args.reth_authrpc.unwrap_or("".to_string()), // TODO: sensible default
+                    secret: args.reth_jwtsecret.unwrap_or_default(),      /* TODO: probably
+                                                                           * secret should be
+                                                                           * Option */


There is no sensible default here and we always need a secret. We're not at the point where we can run without reth, so make these required.

Oh these, I haven't had a change to go through in detail. These were just being moved around. Will make it required.

crates/storage/src/managers/mod.rs

delbonis · 2024-12-26T18:49:41Z

crates/rocksdb-store/src/lib.rs

+pub type CommonDb =
+    CommonDatabase<L1Db, L2Db, SyncEventDb, ClientStateDb, ChainstateDb, RBCheckpointDB>;


I would like to get rid of this CommonDatabase abstraction since it's not useful anymore. But since there's a lot of stuff still using the Database trait I'm not sure how feasible that is yet.

The feasibility is yet to be assessed. Once things are in place where they should be, we can then get rid of the stuffs.

delbonis · 2024-12-26T18:50:37Z

crates/common/src/config.rs

This crate is for things that are common to every possible service, like logging. This configuration is specific to the strata client. Do you think it makes sense move it to a new node crate like Reth has and shift more init logic there?

Yes, I think that would be better. I've mentioned the rationale behind this in one of the comments above.

I am also working on a new crate, in a different branch, which I've named as "client". But I do like the name "node" more.

So yeah, let's keep Config under common as it is fairly serves its purpose(being "common" to almost all other crates in some ways). Can move to node crate in the subsequent PRs.

No it doesn't serve its purpose. That's just not what the crate is intended for.

The term "common" refers to what's shared underlying infra between all of the binaries.

It's not a consensus-related type like what the primitives crate is for.

Yes as I said before it would make sense to have a new node crate like Reth has.

because reader, writer, bridge, etc have their configs derived from this Config.

But we don't want to make btcio dependent on this new node crate, the node crate makes sense to be dependent on btcio instead. The btcio services have their own configuration that we build from our config.

Should we have a new crate config? Well, do you agree that Config should be somewhere outside of bin?

It just makes sense in that node crate.

Thought some more and decided to have a new config crate. This way, crates depending on config don't have to pull in the node crate.

What would be depending on this config crate but not the node crate?

delbonis

Ok I think I'm fine with this as-is. It's still a work in progress but this is an improvement.

bewakes requested review from a team as code owners December 25, 2024 10:28

bewakes force-pushed the STR-786-main-refactor-part1 branch 2 times, most recently from 61486b8 to 95d94a5 Compare December 26, 2024 05:50

bewakes requested a review from a team as a code owner December 26, 2024 05:50

bewakes force-pushed the STR-786-main-refactor-part1 branch 2 times, most recently from 0b95080 to c555431 Compare December 26, 2024 08:47

delbonis requested changes Dec 26, 2024

View reviewed changes

bewakes force-pushed the STR-786-main-refactor-part1 branch from c555431 to c870a11 Compare December 27, 2024 10:04

bewakes enabled auto-merge December 27, 2024 10:57

bewakes requested review from a team as code owners January 1, 2025 07:06

bewakes requested a review from delbonis January 1, 2025 08:00

Bibek Pandey added 6 commits January 2, 2025 14:21

Update task manager interface

4129903

Add DbManagers to consolidate managers

37e943e

Move db helpers to rocksdb crate

1675bdd

Move config to common

30d72e0

(config): Add a new crate config

cdb860d

Some cleanup in args module

76a8c03

bewakes force-pushed the STR-786-main-refactor-part1 branch from dcab80f to 76a8c03 Compare January 2, 2025 08:36

storage: Rename DbManagers to NodeStorage

78c20dd

delbonis approved these changes Jan 2, 2025

View reviewed changes

sapinb approved these changes Jan 3, 2025

View reviewed changes

sapinb disabled auto-merge January 3, 2025 11:39

sapinb merged commit ef1097b into main Jan 3, 2025
16 of 18 checks passed

sapinb deleted the STR-786-main-refactor-part1 branch January 3, 2025 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Str 786 main refactor part1 #561

Str 786 main refactor part1 #561

bewakes commented Dec 25, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading

delbonis Dec 26, 2024

bewakes Dec 27, 2024

delbonis Dec 30, 2024

delbonis Dec 26, 2024

bewakes Dec 27, 2024

delbonis Dec 26, 2024

bewakes Dec 27, 2024

delbonis Dec 26, 2024

bewakes Dec 27, 2024

delbonis Dec 26, 2024

bewakes Dec 27, 2024 •

edited

Loading

bewakes Dec 27, 2024

delbonis Dec 30, 2024

delbonis Dec 30, 2024

delbonis Dec 31, 2024

bewakes Dec 31, 2024

delbonis Dec 31, 2024

bewakes Jan 1, 2025

delbonis Jan 2, 2025

delbonis left a comment

		pub fn update_config(&self, config: &mut Config) {
		let args = self.clone();

		pub type CommonDb =
		CommonDatabase<L1Db, L2Db, SyncEventDb, ClientStateDb, ChainstateDb, RBCheckpointDB>;

Str 786 main refactor part1 #561

Str 786 main refactor part1 #561

Conversation

bewakes commented Dec 25, 2024 • edited Loading

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

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

bewakes Dec 27, 2024 • 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

delbonis left a comment

Choose a reason for hiding this comment

bewakes commented Dec 25, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading

bewakes Dec 27, 2024 •

edited

Loading