Add --block-{verification,production}-method flags (noop atm) #30746

ryoqun · 2023-03-16T13:41:42Z

Problem

There's no way to switch block verification/production implementations from cli in the replay/banking stages.

Summary of Changes

Add minimal cli-side plumbing so that we can add actual impls.

Hope, my unified scheduler for replaying can come soon....

Anyway, let's focus to battle for naming of cli args and descriptions here. :)

Conclusion

bike shed battle ended:

$ SOLANA_NO_HIDDEN_CLI_ARGS_= ./target/release/solana-ledger-tool verify --help
...
        --block-production-method <METHOD>
            Switch transaction scheduling method for producing ledger entries [default: thread-local-multi-iterator]
            [possible values: thread-local-multi-iterator]
        --block-verification-method <METHOD>
            Switch transaction scheduling method for verifying ledger entries [default: blockstore-processor] [possible
            values: blockstore-processor, unified-scheduler]
...

validator/src/cli.rs

core/src/validator.rs

codecov · 2023-03-16T15:31:51Z

Codecov Report

Merging #30746 (428fcfd) into master (721719d) will decrease coverage by 0.1%.
The diff coverage is 48.2%.

@@            Coverage Diff            @@
##           master   #30746     +/-   ##
=========================================
- Coverage    81.5%    81.4%   -0.1%     
=========================================
  Files         723      723             
  Lines      203577   203616     +39     
=========================================
- Hits       165923   165913     -10     
- Misses      37654    37703     +49

core/src/validator.rs

ledger-tool/src/main.rs

t-nelson · 2023-03-16T16:22:33Z

i don't imagine the interfaces here are clean enough that we could dyload the impl(s)?

tao-stones

Think I am missing context, this ability of switching replaying/banking implementation is for testing purpose, or to allow mainnet validators to configure their s/w (practically to have two versions of labs validators in mainnet)?

core/src/validator.rs

ledger-tool/src/main.rs

ryoqun · 2023-03-17T11:25:08Z

i don't imagine the interfaces here are clean enough that we could dyload the impl(s)?

yep. so i avoided dyn naming.

core/src/validator.rs

clap-utils/src/lib.rs

ryoqun · 2023-03-17T14:37:06Z

@brooksprumo @apfitzge @taozhu-chicago @t-nelson thanks for reviewing! I think I've relied to all. :) the hottest topic would be engine or else: #30746 (comment)

brooksprumo · 2023-03-21T00:03:09Z

I defer the naming to y'all. As I haven't worked on much of this code, I trust your judgement as domain experts here!

apfitzge · 2023-03-21T19:45:54Z

i don't imagine the interfaces here are clean enough that we could dyload the impl(s)?

this is actually something I'd like to eventually support for bank scheduling (now block-production arg?). Final interface is still in-flight, so not thinking too much about it yet.

However, I don't think that option will play very nicely with the use of strum here if we want to pass the library path as a cli arg along lines of --block-production-method dynamic-lib PATH

core/src/validator.rs

ledger-tool/src/main.rs

apfitzge · 2023-03-21T19:48:03Z

ledger-tool/src/main.rs

+                .possible_values(ReplayingBackend::cli_names())
+                .default_value(ReplayingBackend::default().into())


Not super familiar with how clap validation works, or if this is a product of us using strum.

I tested this and passing --replaying-backend (note I didn't specify the variant) works fine and results in the default.

That seemed a bit odd to me.

ugh, so much thanks for catching this: d1d317e + 14c0f88

ryoqun · 2023-03-22T14:52:18Z

@apfitzge could you review this to merge. I've finished off all review comments and renamed. :)

ryoqun · 2023-03-22T14:53:52Z

However, I don't think that option will play very nicely with the use of strum here if we want to pass the library path as a cli arg along lines of --block-production-method dynamic-lib PATH

in short, these will be possible, if desired. clap is very customizable if wanted...

apfitzge · 2023-03-22T15:21:35Z

However, I don't think that option will play very nicely with the use of strum here if we want to pass the library path as a cli arg along lines of --block-production-method dynamic-lib PATH

in short, these will be possible, if desired. clap is very customizable if wanted...

Yeah, definitely possible with clap. Just not sure we can still use the simple EnumIter stuff from strum

apfitzge

A few more changes - sorry the review took a while this morning...I always try to give suggestions instead of just saying I don't like something. But it took me a while to figure out how to do it more cleanly (imo)

core/src/validator.rs

validator/src/cli.rs

apfitzge

lgtm - thanks for all the work to get this ready for us both!

ryoqun · 2024-02-14T14:09:49Z

core/src/validator.rs

+        lazy_static! {
+            static ref MESSAGE: String = format!(
+                "Switch transaction scheduling method for verifying ledger entries [default: {}]",
+                BlockVerificationMethod::default()
+            );
+        };
+
+        &MESSAGE


ref: #35195 (comment)

ryoqun requested review from apfitzge and tao-stones March 16, 2023 13:41