build: add option to compile the node without rpc

[ShahakShama]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 82.85714% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 71.85%. Comparing base (88f2623) to head (a84e89f).
Report is 1 commits behind head on main.

Files	Patch %	Lines
crates/papyrus_node/src/bin/dump_config.rs	0.00%	5 Missing ⚠️
crates/papyrus_node/src/main.rs	95.23%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1689   +/-   ##
=======================================
  Coverage   71.84%   71.85%           
=======================================
  Files         141      141           
  Lines       20201    20224   +23     
  Branches    20201    20224   +23     
=======================================
+ Hits        14514    14532   +18     
- Misses       3515     3521    +6     
+ Partials     2172     2171    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yair-starkware]

Reviewed 6 of 9 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @ShahakShama)

---

crates/papyrus_node/src/bin/dump_config.rs line 43 at r2 (raw file):

}

#[cfg(not(feature = "rpc"))]

Does something break if we keep the RPC config in the default file in the scenario of no rpc?
If not, I think we should avoid creating another default config file.

---

crates/papyrus_node/src/bin/dump_config.rs line 76 at r2 (raw file):

/// Updates the default config file by:
/// cargo run --bin dump_config -q

Don't we need to run without the rpc feature somehow in case we want to keep both default files?

Code quote:

cargo run --bin dump_config -q

[ShahakShama]

Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @yair-starkware)

---

crates/papyrus_node/src/bin/dump_config.rs line 43 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Does something break if we keep the RPC config in the default file in the scenario of no rpc?
If not, I think we should avoid creating another default config file.

Done.

---

crates/papyrus_node/src/bin/dump_config.rs line 76 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

Don't we need to run without the rpc feature somehow in case we want to keep both default files?

Done.

[yair-starkware]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @ShahakShama)

---

crates/papyrus_node/src/main.rs line 5 at r3 (raw file):

use std::env::args;
use std::future::{self, pending, Future};

Is this on purpose?

Code quote:

self

---

crates/papyrus_node/src/bin/dump_config.rs line 72 at r3 (raw file):

        ),
        vec!["monitoring_gateway.collect_metrics".to_owned()],
    )];

Do we still need this extra code? We don't dump default config for no rpc anymore, right?

Code quote:

#[cfg(not(feature = "rpc"))]
lazy_static! {
    /// Returns vector of (pointer target name, pointer target serialized param, vec<pointer param path>)
    /// to be applied on the dumped node config.
    /// The config updates will be performed on the shared pointer targets, and finally, the values
    /// will be propagated to the pointer params.
    static ref CONFIG_POINTERS: Vec<((ParamPath, SerializedParam), Vec<ParamPath>)> = vec![(
        ser_pointer_target_param(
            "chain_id",
            &ChainId("SN_MAIN".to_string()),
            "The chain to follow. For more details see https://docs.starknet.io/documentation/architecture_and_concepts/Blocks/transactions/#chain-id.",
        ),
        vec!["storage.db_config.chain_id".to_owned()],
    ),
    (
        ser_pointer_target_param(
            "starknet_url",
            &"https://alpha-mainnet.starknet.io/".to_string(),
            "The URL of a centralized Starknet gateway.",
        ),
        vec!["central.url".to_owned(), "monitoring_gateway.starknet_url".to_owned()],
    ),
    (
        ser_pointer_target_param(
            "collect_metrics",
            &false,
            "If true, collect metrics for the node.",
        ),
        vec!["monitoring_gateway.collect_metrics".to_owned()],
    )];

[ShahakShama]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @yair-starkware)

---

crates/papyrus_node/src/main.rs line 5 at r3 (raw file):

Previously, yair-starkware (Yair) wrote…

Is this on purpose?

This is a rebase result. See the full diff and it will be clearer

---

crates/papyrus_node/src/bin/dump_config.rs line 72 at r3 (raw file):

Previously, yair-starkware (Yair) wrote…

Do we still need this extra code? We don't dump default config for no rpc anymore, right?

Yes, as the destination of the pointers changes between the features. This has nothing to do with the default config

[yair-starkware]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @ShahakShama)

---

crates/papyrus_node/src/bin/dump_config.rs line 6 at r5 (raw file):

#[cfg(feature = "rpc")]
use lazy_static::lazy_static;

Put everything in a module and use #[cfg(feature = "rpc")] on the whole thing

[ShahakShama]

Reviewable status: 9 of 11 files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @yair-starkware)

---

crates/papyrus_node/src/bin/dump_config.rs line 6 at r5 (raw file):

Previously, yair-starkware (Yair) wrote…

Put everything in a module and use #[cfg(feature = "rpc")] on the whole thing

Done.

[yair-starkware]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @ShahakShama)

---

crates/papyrus_node/src/bin/dump_config.rs line 6 at r5 (raw file):

Previously, ShahakShama wrote…

Done.

I meant in the same file but I guess that's fine.
Also make a function that will be called in main so all the use statements can be in the module too

[ShahakShama]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @yair-starkware)

---

crates/papyrus_node/src/bin/dump_config.rs line 6 at r5 (raw file):

Previously, yair-starkware (Yair) wrote…

I meant in the same file but I guess that's fine.
Also make a function that will be called in main so all the use statements can be in the module too

Done.

[yair-starkware]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)

[dan-starkware]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ShahakShama)

---

.github/workflows/ci.yml line 118 at r8 (raw file):

      - run: |
          cargo test -p papyrus_node --no-default-features

Can we have something more explicit, like --no-rpc

Code quote:

no-default-features

---

crates/papyrus_node/src/main.rs line 71 at r8 (raw file):

    _storage_reader: StorageReader,
) -> anyhow::Result<impl Future<Output = Result<(), JoinError>>> {
    Ok(futures::future::pending())

Please explain the difference between this pending and the one imported, sort the multiple pending

Code quote:

(futures::future::pending(

---

crates/papyrus_node/src/main.rs line 100 at r8 (raw file):

        block: PendingBlockOrDeprecated::Current(PendingBlock {
            parent_block_hash: BlockHash(
                GENESIS_HASH.try_into().expect("Failed converting genesis hash to StarkHash"),

Is it related to this PR?

Code quote:

 GENESIS_HASH.try_into().expect("Failed converting genesis hash to StarkHash"),

---

crates/papyrus_node/src/main_test.rs line 23 at r8 (raw file):

#[cfg(not(feature = "rpc"))]
fn fix_execution_config_path(_config: &mut NodeConfig) {}

Please explain why RPC and execution are coupled here

Code quote:

#[cfg(feature = "rpc")]
fn fix_execution_config_path(config: &mut NodeConfig) {
    let default_execution_config_path = RpcConfig::default().execution_config;
    config.rpc.execution_config =
        get_absolute_path(default_execution_config_path.to_str().unwrap());
}

#[cfg(not(feature = "rpc"))]
fn fix_execution_config_path(_config: &mut NodeConfig) {}

---

crates/papyrus_node/src/bin/dump_config.rs line 59 at r8 (raw file):

    with_rpc::dump_config();
    // TODO(shahak): Try to find a way to remove this binary altogether when the feature rpc is
    // turned off.

Please check this and otherwise move the inner module into a separate file

Code quote:

    // TODO(shahak): Try to find a way to remove this binary altogether when the feature rpc is
    // turned off.

---

crates/papyrus_node/src/config/mod.rs line 45 at r8 (raw file):

pub struct NodeConfig {
    #[validate]
    pub rpc: RpcConfig,

I would expect this to be conditionally as well

Code quote:

pub rpc: RpcConfig,

---

crates/papyrus_node/src/config/mod.rs line 99 at r8 (raw file):

}

// TODO(shahak): Try to make this config empty.

What fails?

Code quote:

Try to make this config empty

[ShahakShama]

Reviewable status: 3 of 12 files reviewed, 3 unresolved discussions (waiting on @dan-starkware and @yair-starkware)

---

.github/workflows/ci.yml line 118 at r8 (raw file):

Previously, dan-starkware wrote…

Can we have something more explicit, like --no-rpc

Sadly no rust-lang/cargo#3126

---

crates/papyrus_node/src/main.rs line 100 at r8 (raw file):

Previously, dan-starkware wrote…

Is it related to this PR?

Yes. I fixed it by changing starknet api in the Cargo.toml to be with feature testing but I think this shows we shouldn't use this macro here. WDYT?

---

crates/papyrus_node/src/bin/dump_config.rs line 59 at r8 (raw file):

Previously, dan-starkware wrote…

Please check this and otherwise move the inner module into a separate file

Looks like someone else moved it to a separate module for me :)

---

crates/papyrus_node/src/config/mod.rs line 45 at r8 (raw file):

Previously, dan-starkware wrote…

I would expect this to be conditionally as well

Done.

---

crates/papyrus_node/src/config/mod.rs line 99 at r8 (raw file):

Previously, dan-starkware wrote…

What fails?

Irrelevant due to previous comment

[dan-starkware]

Reviewed 1 of 4 files at r2, 1 of 5 files at r3, 8 of 9 files at r9, all commit messages.
Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @yair-starkware)

[dan-starkware]

Reviewed 1 of 9 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ShahakShama)

[dan-starkware]

Reviewed 1 of 2 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ShahakShama)