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

Define tick related helper test methods #33537

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

ryoqun
Copy link
Member

@ryoqun ryoqun commented Oct 5, 2023

Problem

my scheduler thing needs to introduce rather invasive code changes including the BankWithScheduler wrapping, which touches tick registration code.

Summary of Changes

Prepare some helper functions.

extracted from: #33070

@ryoqun ryoqun requested a review from apfitzge October 5, 2023 14:07
@@ -29,6 +29,7 @@ source ci/rust-version.sh nightly
# reason to bend dev-context-only-utils's original intention and that listed
# package isn't part of released binaries.
declare tainted_packages=(
solana-ledger-tool
Copy link
Member Author

@ryoqun ryoqun Oct 5, 2023

Choose a reason for hiding this comment

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

after very loong thought since hacking on dcou, i've finally decided to regard solana-ledger-tool as rather development tool, although it's sometimes used in the production environment indeed (like create-snapshot).

it contains very dangerous code, which i'd like to guard under dcou. for one, the touched code in this pr is literally arbitrarily mangling snapshot with new bank...

so, i think it's okay to blacklist it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Marking ledger-tool as a tainted package might warrant some wider discussion from ledger-tool focussed people: @steviez @brooksprumo

Copy link
Contributor

@steviez steviez Oct 6, 2023

Choose a reason for hiding this comment

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

Thanks for bringing this up apfitzge. I'm not sure I understand the full consequences of adding solana-ledger-tool to this list; can you help me understand the implications of adding solana-ledger-tool here

to regard solana-ledger-tool as rather development tool

I'd mostly agree with this statement, with the caveat that you already mentioned of operators needing this in cluster-restart scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

as a quick recap, dcou-ed code (#[cfg(feature = "dev-context-only-utils")]) are much like #[cfg(test)]. those code is used for testing and benching or other development purposes. dcou works across the monorepo workspace, unlike #[cfg(test)]. so, The main benefit of dcou is that dangerous code can safely be shared across monorepo, instead of pub-ing with some for_test suffix and gentleman agreement among engs.

to accomplish the aforementioned goals, dcou-ed code is opt-in only after explicitly enabling the dev-context-only-utils feature. AND, dcou is enforced as a dev-dependency via ci. in this way , any production code (i.e. not test/bench code) can't access dcou code.

dcou was introduced recently as an experiment, but working as intended. so, it's expected to dcou code will increase.

hope the story is straightforward so far. :)

then, there's some fine prints.... sometimes we want exceptions. specifically, we want to protect some code under dcou, while still allowing to be used outside dev-dep.

I'm not sure I understand the full consequences of adding solana-ledger-tool to this list; can you help me understand the implications of adding solana-ledger-tool here

solana-ledger-tool is the exceptional case as being a dev tool. and, adding solana-ledger-tool here means the crate is now one of the exceptions. thus, solana-ledger-tool' s production code will remain to be able to call dcou-ed api without restriction, even after dcou usage is increased.

using this pr as an example, i think tainting it is okay instead of not dcou-ing the .register_unique_tick() just for solana-ledger-tool.

that's because there will be more similar situations, considering the nature of solana-ledger-tool, although it is indeed used for cluster-restart as you mentioned:

to regard solana-ledger-tool as rather development tool

I'd mostly agree with this statement, with the caveat that you already mentioned of operators needing this in cluster-restart scenarios.

on contrast, definitely we want to avoid to add solana-validator and solana (the cli) here.

all in all, I'm saying aggressively protecting solana-validator and the like from some code with dcou outweigh the potential accidental use of dcou code in solana-ledger-tool, which might be a trouble only if it's part of the create-snapshot codepath.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, didn't give my ship-it before you pushed. But in any case ...

hope the story is straightforward so far. :)

Yep, everything up to that point I was aware of

solana-ledger-tool is the exceptional case as being a dev tool. and, adding solana-ledger-tool here means the crate is now one of the exceptions. thus, solana-ledger-tool' s production code will remain to be able to call dcou-ed api without restriction, even after dcou usage is increased

This is what I was unaware of, so thank you for calling it out explicitly!

all in all, I'm saying aggressively protecting solana-validator and the like from some code with dcou outweigh the potential accidental use of dcou code in solana-ledger-tool, which might be a trouble only if it's part of the create-snapshot codepath.

Agree with this sentiment, and while I can't give a retro-active ship it, I'm onboard with the decision / change. Thanks again for the heads up apfitzge & for the explanation ryoqun

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as ☝️ for me too

Copy link
Member Author

Choose a reason for hiding this comment

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

err, thanks for polite post-merge replies... as i have tons of code to rebase and ship (#33070), i was aggressive in hindsight...

@@ -1,3 +1,4 @@
#![cfg(feature = "dev-context-only-utils")]
Copy link
Member Author

Choose a reason for hiding this comment

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

dcou with inner attr is kinda cool.

}

#[cfg(feature = "dev-context-only-utils")]
pub fn register_unique_tick(&self) {
Copy link
Member Author

Choose a reason for hiding this comment

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

_for_test is omitted intentionally due to the use by VoteSimulator and solana-ledger-tool.

@ryoqun ryoqun changed the title Define tick related helper methods Define tick related helper test methods Oct 5, 2023
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #33537 (7eef3ea) into master (c66af12) will increase coverage by 0.0%.
Report is 4 commits behind head on master.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #33537   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         805      805           
  Lines      218111   218120    +9     
=======================================
+ Hits       178323   178355   +32     
+ Misses      39788    39765   -23     

@ryoqun ryoqun requested a review from steviez October 7, 2023 05:33
@ryoqun ryoqun merged commit 1704789 into solana-labs:master Oct 10, 2023
27 checks passed
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