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.

Sign up for GitHub

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

Jump to bottom

Define tick related helper test methods #33537

Merged
merged 5 commits into from
Oct 10, 2023
Merged

Define tick related helper test methods #33537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abac72f
Define tick related helper methods
ryoqun Oct 5, 2023
b66f4e0
dcou VoteSimulator
ryoqun Oct 5, 2023
4497291
blacklist ledger-tool for dcou
ryoqun Oct 5, 2023
89614d0
fix dcou ci...
ryoqun Oct 5, 2023
7eef3ea
github
ryoqun Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/replay_stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4844,7 +4844,7 @@ pub(crate) mod tests {
genesis_config.ticks_per_slot = 4;
let bank0 = Bank::new_for_tests(&genesis_config);
for _ in 0..genesis_config.ticks_per_slot {
bank0.register_tick(&Hash::default());
bank0.register_default_tick_for_test();
}
bank0.freeze();
let arc_bank0 = Arc::new(bank0);
Expand Down Expand Up @@ -4889,7 +4889,7 @@ pub(crate) mod tests {
&solana_sdk::pubkey::new_rand(),
);
for _ in 0..genesis_config.ticks_per_slot {
bank.register_tick(&Hash::default());
bank.register_default_tick_for_test();
}
bank_forks.write().unwrap().insert(bank);
let arc_bank = bank_forks.read().unwrap().get(i).unwrap();
Expand Down
5 changes: 3 additions & 2 deletions core/src/vote_simulator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

use {
crate::{
cluster_info_vote_listener::VoteTracker,
Expand Down Expand Up @@ -117,7 +118,7 @@ impl VoteSimulator {
}
}
while new_bank.tick_height() < new_bank.max_tick_height() {
new_bank.register_tick(&Hash::new_unique());
new_bank.register_unique_tick();
}
if !visit.node().has_no_child() || is_frozen {
new_bank.freeze();
Expand Down Expand Up @@ -358,7 +359,7 @@ pub fn initialize_state(
}

while bank0.tick_height() < bank0.max_tick_height() {
bank0.register_tick(&Hash::new_unique());
bank0.register_unique_tick();
}
bank0.freeze();
let mut progress = ProgressMap::default();
Expand Down
6 changes: 3 additions & 3 deletions core/tests/snapshots.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ fn goto_end_of_slot(bank: &Bank) {
let mut tick_hash = bank.last_blockhash();
loop {
tick_hash = hashv(&[tick_hash.as_ref(), &[42]]);
bank.register_tick(&tick_hash);
bank.register_tick_for_test(&tick_hash);
if tick_hash == bank.last_blockhash() {
bank.freeze();
return;
Expand Down Expand Up @@ -742,7 +742,7 @@ fn test_bank_forks_incremental_snapshot(
assert_eq!(bank.process_transaction(&tx), Ok(()));

while !bank.is_complete() {
bank.register_tick(&Hash::new_unique());
bank.register_unique_tick();
}

bank_forks.insert(bank)
Expand Down Expand Up @@ -1041,7 +1041,7 @@ fn test_snapshots_with_background_services(
assert_eq!(bank.process_transaction(&tx), Ok(()));

while !bank.is_complete() {
bank.register_tick(&Hash::new_unique());
bank.register_unique_tick();
}

bank_forks.write().unwrap().insert(bank);
Expand Down
5 changes: 4 additions & 1 deletion ledger-tool/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ solana-logger = { workspace = true }
solana-measure = { workspace = true }
solana-program-runtime = { workspace = true }
solana-rpc = { workspace = true }
solana-runtime = { workspace = true }
solana-runtime = { workspace = true, features = ["dev-context-only-utils"] }
solana-sdk = { workspace = true }
solana-stake-program = { workspace = true }
solana-storage-bigtable = { workspace = true }
Expand All @@ -57,6 +57,9 @@ jemallocator = { workspace = true }
assert_cmd = { workspace = true }
bytecount = { workspace = true }

[features]
dev-context-only-utils = []

[target."cfg(unix)".dependencies]
signal-hook = { workspace = true }

Expand Down
2 changes: 1 addition & 1 deletion ledger-tool/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3139,7 +3139,7 @@ fn main() {

if child_bank_required {
while !bank.is_complete() {
bank.register_tick(&Hash::new_unique());
bank.register_unique_tick();
}
}

Expand Down
1 change: 1 addition & 0 deletions ledger/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ features = ["lz4"]
bs58 = { workspace = true }
solana-account-decoder = { workspace = true }
solana-logger = { workspace = true }
solana-runtime = { workspace = true, features = ["dev-context-only-utils"] }
spl-pod = { workspace = true }
test-case = { workspace = true }

Expand Down
2 changes: 1 addition & 1 deletion ledger/src/blockstore_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3373,7 +3373,7 @@ pub mod tests {

let blockhash = bank.last_blockhash();
while blockhash == bank.last_blockhash() {
bank.register_tick(&Hash::default());
bank.register_default_tick_for_test();
}

// ensure bank can process 2 entries that do not have a common account and tick is registered
Expand Down
16 changes: 16 additions & 0 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4211,6 +4211,22 @@ impl Bank {
self.tick_height.fetch_add(1, Relaxed);
}

#[cfg(feature = "dev-context-only-utils")]
pub fn register_tick_for_test(&self, hash: &Hash) {
// currently meaningless wrapper; upcoming pr will make it an actual helper...
self.register_tick(hash)
}

#[cfg(feature = "dev-context-only-utils")]
pub fn register_default_tick_for_test(&self) {
self.register_tick(&Hash::default())
}

#[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.

self.register_tick(&Hash::new_unique())
}

pub fn is_complete(&self) -> bool {
self.tick_height() == self.max_tick_height()
}
Expand Down
6 changes: 3 additions & 3 deletions runtime/src/bank/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ fn test_race_register_tick_freeze() {
let register_tick_thread = Builder::new()
.name("register_tick".to_string())
.spawn(move || {
bank0_.register_tick(&hash);
bank0_.register_tick_for_test(&hash);
})
.unwrap();

Expand Down Expand Up @@ -4204,7 +4204,7 @@ fn test_is_delta_true() {
assert!(!bank1.is_delta.load(Relaxed));
assert_ne!(hash1, bank.hash());
// ticks don't make a bank into a delta or change its state unless a block boundary is crossed
bank1.register_tick(&Hash::default());
bank1.register_default_tick_for_test();
assert!(!bank1.is_delta.load(Relaxed));
assert_eq!(bank1.hash_internal_state(), hash1);
}
Expand Down Expand Up @@ -4928,7 +4928,7 @@ fn test_hash_internal_state_unchanged_with_ticks() {
// because blockhashes are only recorded at block boundaries
for _ in 0..genesis_config.ticks_per_slot {
assert_eq!(bank1.hash_internal_state(), hash1);
bank1.register_tick(&Hash::default());
bank1.register_default_tick_for_test();
}
assert_eq!(bank1.hash_internal_state(), hash1);
}
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/bank_forks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,7 @@ mod tests {
let bank = Bank::new_for_tests(&genesis_config);
let mut bank_forks = BankForks::new(bank);
let child_bank = Bank::new_from_parent(bank_forks[0].clone(), &Pubkey::default(), 1);
child_bank.register_tick(&Hash::default());
child_bank.register_default_tick_for_test();
bank_forks.insert(child_bank);
assert_eq!(bank_forks[1u64].tick_height(), 1);
assert_eq!(bank_forks.working_bank().tick_height(), 1);
Expand Down
48 changes: 24 additions & 24 deletions runtime/src/snapshot_bank_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ mod tests {
let original_bank = Bank::new_for_tests(&genesis_config);

while !original_bank.is_complete() {
original_bank.register_tick(&Hash::new_unique());
original_bank.register_unique_tick();
}

let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests();
Expand Down Expand Up @@ -1359,7 +1359,7 @@ mod tests {
.transfer(sol_to_lamports(3.), &mint_keypair, &key3.pubkey())
.unwrap();
while !bank0.is_complete() {
bank0.register_tick(&Hash::new_unique());
bank0.register_unique_tick();
}

let slot = 1;
Expand All @@ -1374,7 +1374,7 @@ mod tests {
.transfer(sol_to_lamports(5.), &mint_keypair, &key5.pubkey())
.unwrap();
while !bank1.is_complete() {
bank1.register_tick(&Hash::new_unique());
bank1.register_unique_tick();
}

let slot = slot + 1;
Expand All @@ -1383,7 +1383,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank2.is_complete() {
bank2.register_tick(&Hash::new_unique());
bank2.register_unique_tick();
}

let slot = slot + 1;
Expand All @@ -1392,7 +1392,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank3.is_complete() {
bank3.register_tick(&Hash::new_unique());
bank3.register_unique_tick();
}

let slot = slot + 1;
Expand All @@ -1401,7 +1401,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank4.is_complete() {
bank4.register_tick(&Hash::new_unique());
bank4.register_unique_tick();
}

let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests();
Expand Down Expand Up @@ -1476,7 +1476,7 @@ mod tests {
.transfer(sol_to_lamports(3.), &mint_keypair, &key3.pubkey())
.unwrap();
while !bank0.is_complete() {
bank0.register_tick(&Hash::new_unique());
bank0.register_unique_tick();
}

let slot = 1;
Expand All @@ -1491,7 +1491,7 @@ mod tests {
.transfer(sol_to_lamports(5.), &mint_keypair, &key5.pubkey())
.unwrap();
while !bank1.is_complete() {
bank1.register_tick(&Hash::new_unique());
bank1.register_unique_tick();
}

let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests();
Expand Down Expand Up @@ -1519,7 +1519,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank2.is_complete() {
bank2.register_tick(&Hash::new_unique());
bank2.register_unique_tick();
}

let slot = slot + 1;
Expand All @@ -1528,7 +1528,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank3.is_complete() {
bank3.register_tick(&Hash::new_unique());
bank3.register_unique_tick();
}

let slot = slot + 1;
Expand All @@ -1537,7 +1537,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank4.is_complete() {
bank4.register_tick(&Hash::new_unique());
bank4.register_unique_tick();
}

let incremental_snapshot_archive_info = bank_to_incremental_snapshot_archive(
Expand Down Expand Up @@ -1597,7 +1597,7 @@ mod tests {
.transfer(sol_to_lamports(3.), &mint_keypair, &key3.pubkey())
.unwrap();
while !bank0.is_complete() {
bank0.register_tick(&Hash::new_unique());
bank0.register_unique_tick();
}

let slot = 1;
Expand All @@ -1612,7 +1612,7 @@ mod tests {
.transfer(sol_to_lamports(3.), &mint_keypair, &key3.pubkey())
.unwrap();
while !bank1.is_complete() {
bank1.register_tick(&Hash::new_unique());
bank1.register_unique_tick();
}

let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests();
Expand Down Expand Up @@ -1640,7 +1640,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank2.is_complete() {
bank2.register_tick(&Hash::new_unique());
bank2.register_unique_tick();
}

let slot = slot + 1;
Expand All @@ -1649,7 +1649,7 @@ mod tests {
.transfer(sol_to_lamports(2.), &mint_keypair, &key2.pubkey())
.unwrap();
while !bank3.is_complete() {
bank3.register_tick(&Hash::new_unique());
bank3.register_unique_tick();
}

let slot = slot + 1;
Expand All @@ -1658,7 +1658,7 @@ mod tests {
.transfer(sol_to_lamports(3.), &mint_keypair, &key3.pubkey())
.unwrap();
while !bank4.is_complete() {
bank4.register_tick(&Hash::new_unique());
bank4.register_unique_tick();
}

bank_to_incremental_snapshot_archive(
Expand Down Expand Up @@ -1746,7 +1746,7 @@ mod tests {
.transfer(lamports_to_transfer, &mint_keypair, &key2.pubkey())
.unwrap();
while !bank0.is_complete() {
bank0.register_tick(&Hash::new_unique());
bank0.register_unique_tick();
}

let slot = 1;
Expand All @@ -1755,7 +1755,7 @@ mod tests {
.transfer(lamports_to_transfer, &key2, &key1.pubkey())
.unwrap();
while !bank1.is_complete() {
bank1.register_tick(&Hash::new_unique());
bank1.register_unique_tick();
}

let full_snapshot_slot = slot;
Expand Down Expand Up @@ -1794,7 +1794,7 @@ mod tests {
"Ensure Account1's balance is zero"
);
while !bank2.is_complete() {
bank2.register_tick(&Hash::new_unique());
bank2.register_unique_tick();
}

// Take an incremental snapshot and then do a roundtrip on the bank and ensure it
Expand Down Expand Up @@ -1844,13 +1844,13 @@ mod tests {
.transfer(lamports_to_transfer, &mint_keypair, &key2.pubkey())
.unwrap();
while !bank3.is_complete() {
bank3.register_tick(&Hash::new_unique());
bank3.register_unique_tick();
}

let slot = slot + 1;
let bank4 = Arc::new(Bank::new_from_parent(bank3, &collector, slot));
while !bank4.is_complete() {
bank4.register_tick(&Hash::new_unique());
bank4.register_unique_tick();
}

// Ensure account1 has been cleaned/purged from everywhere
Expand Down Expand Up @@ -1917,13 +1917,13 @@ mod tests {
let (genesis_config, mint_keypair) = create_genesis_config(sol_to_lamports(1_000_000.));
let bank0 = Arc::new(Bank::new_for_tests(&genesis_config));
while !bank0.is_complete() {
bank0.register_tick(&Hash::new_unique());
bank0.register_unique_tick();
}

let slot = 1;
let bank1 = Arc::new(Bank::new_from_parent(bank0, &collector, slot));
while !bank1.is_complete() {
bank1.register_tick(&Hash::new_unique());
bank1.register_unique_tick();
}

let all_snapshots_dir = tempfile::TempDir::new().unwrap();
Expand All @@ -1948,7 +1948,7 @@ mod tests {
.transfer(sol_to_lamports(1.), &mint_keypair, &key1.pubkey())
.unwrap();
while !bank2.is_complete() {
bank2.register_tick(&Hash::new_unique());
bank2.register_unique_tick();
}

bank_to_incremental_snapshot_archive(
Expand Down
1 change: 1 addition & 0 deletions scripts/check-dev-context-only-utils.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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...

)

# convert to comma separeted (ref: https://stackoverflow.com/a/53839433)
Expand Down