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

v2.1: Fix flaky test_transaction_result_does_not_affect_bankhash (backport of #3916) #4578

Merged
merged 2 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
49 changes: 43 additions & 6 deletions ledger/src/blockstore_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2306,7 +2306,8 @@ pub mod tests {
crate::{
blockstore_options::{AccessType, BlockstoreOptions},
genesis_utils::{
create_genesis_config, create_genesis_config_with_leader, GenesisConfigInfo,
create_genesis_config, create_genesis_config_with_leader,
create_genesis_config_with_mint_keypair, GenesisConfigInfo,
},
},
assert_matches::assert_matches,
Expand All @@ -2333,6 +2334,7 @@ pub mod tests {
pubkey::Pubkey,
rent_debits::RentDebits,
signature::{Keypair, Signer},
signer::SeedDerivable,
system_instruction::SystemError,
system_transaction,
transaction::{Transaction, TransactionError},
Expand All @@ -2349,6 +2351,7 @@ pub mod tests {
vote_transaction,
},
std::{collections::BTreeSet, sync::RwLock},
test_case::test_case,
trees::tr,
};

Expand Down Expand Up @@ -3411,14 +3414,19 @@ pub mod tests {
}
}

#[test]
fn test_transaction_result_does_not_affect_bankhash() {
#[test_case(true; "rent_collected")]
#[test_case(false; "rent_not_collected")]
fn test_transaction_result_does_not_affect_bankhash(fee_payer_in_rent_partition: bool) {
solana_logger::setup();
let GenesisConfigInfo {
genesis_config,
mint_keypair,
..
} = create_genesis_config(1000);
} = if fee_payer_in_rent_partition {
create_genesis_config(1000)
} else {
create_genesis_config_with_mint_keypair(Keypair::from_seed(&[1u8; 32]).unwrap(), 1000)
};

fn get_instruction_errors() -> Vec<InstructionError> {
vec![
Expand Down Expand Up @@ -3484,7 +3492,7 @@ pub mod tests {
Ok(())
});

let mock_program_id = solana_sdk::pubkey::new_rand();
let mock_program_id = Pubkey::new_unique();

let (bank, _bank_forks) = Bank::new_with_mockup_builtin_for_tests(
&genesis_config,
Expand Down Expand Up @@ -3548,13 +3556,42 @@ pub mod tests {

let entry = next_entry(&bank.last_blockhash(), 1, vec![tx]);
let bank = Arc::new(bank);
let _result = process_entries_for_tests_without_scheduler(&bank, vec![entry]);
let result = process_entries_for_tests_without_scheduler(&bank, vec![entry]);
assert!(result.is_ok()); // No failing transaction error - only instruction errors
bank.freeze();

<<<<<<< HEAD
assert_eq!(blockhash_ok, bank.last_blockhash());
assert!(bankhash_ok != bank.hash());
if let Some(bankhash) = bankhash_err {
assert_eq!(bankhash, bank.hash());
=======
// Transaction success/failure should not affect block hash ...
assert_eq!(
ok_bank_details
.bank_hash_components
.as_ref()
.unwrap()
.last_blockhash,
bank_details
.bank_hash_components
.as_ref()
.unwrap()
.last_blockhash
);
// AND should not affect bankhash IF the rent is collected during freeze.
assert_eq!(ok_bank_details == bank_details, fee_payer_in_rent_partition);
Copy link

Choose a reason for hiding this comment

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

This is the assertion we need to do as noted by what we have in master:

assert_eq!(ok_bank_details == bank_details, fee_payer_in_rent_partition);

which you can also see is what was added in the initial PR:
https://github.com/anza-xyz/agave/pull/3916/files#diff-281d947e593a76f3c3380804162dc8cf426fcc33e0ae1192589c8193221cd1e2R3626

// Different types of transaction failure should not affect bank hash
if let Some(prev_bank_details) = &err_bank_details {
assert_eq!(
*prev_bank_details,
bank_details,
"bank hash mismatched for tx error: {:?}",
get_instruction_errors()[err]
);
} else {
err_bank_details = Some(bank_details);
>>>>>>> c5473e4b3 (Fix flaky test_transaction_result_does_not_affect_bankhash (#3916))
}
bankhash_err = Some(bank.hash());
});
Expand Down
18 changes: 17 additions & 1 deletion ledger/src/genesis_utils.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,29 @@
pub use solana_runtime::genesis_utils::{
bootstrap_validator_stake_lamports, create_genesis_config_with_leader, GenesisConfigInfo,
};
use {
solana_runtime::genesis_utils::create_genesis_config_with_leader_with_mint_keypair,
solana_sdk::{pubkey::Pubkey, signature::Keypair},
};

// same as genesis_config::create_genesis_config, but with bootstrap_validator staking logic
// for the core crate tests
pub fn create_genesis_config(mint_lamports: u64) -> GenesisConfigInfo {
create_genesis_config_with_leader(
mint_lamports,
&solana_sdk::pubkey::new_rand(),
&Pubkey::new_unique(),
bootstrap_validator_stake_lamports(),
)
}

pub fn create_genesis_config_with_mint_keypair(
mint_keypair: Keypair,
mint_lamports: u64,
) -> GenesisConfigInfo {
create_genesis_config_with_leader_with_mint_keypair(
mint_keypair,
mint_lamports,
&Pubkey::new_unique(),
bootstrap_validator_stake_lamports(),
)
}
32 changes: 29 additions & 3 deletions runtime/src/genesis_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use {
pubkey::Pubkey,
rent::Rent,
signature::{Keypair, Signer},
signer::SeedDerivable,
stake::state::StakeStateV2,
system_program,
},
Expand Down Expand Up @@ -169,15 +170,40 @@ pub fn create_genesis_config_with_leader(
validator_pubkey: &Pubkey,
validator_stake_lamports: u64,
) -> GenesisConfigInfo {
let mint_keypair = Keypair::new();
let voting_keypair = Keypair::new();
// Use deterministic keypair so we don't get confused by randomness in tests
let mint_keypair = Keypair::from_seed(&[
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24,
25, 26, 27, 28, 29, 30, 31,
])
.unwrap();

create_genesis_config_with_leader_with_mint_keypair(
mint_keypair,
mint_lamports,
validator_pubkey,
validator_stake_lamports,
)
}

pub fn create_genesis_config_with_leader_with_mint_keypair(
mint_keypair: Keypair,
mint_lamports: u64,
validator_pubkey: &Pubkey,
validator_stake_lamports: u64,
) -> GenesisConfigInfo {
// Use deterministic keypair so we don't get confused by randomness in tests
let voting_keypair = Keypair::from_seed(&[
32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54,
55, 56, 57, 58, 59, 60, 61, 62, 63,
])
.unwrap();

let genesis_config = create_genesis_config_with_leader_ex(
mint_lamports,
&mint_keypair.pubkey(),
validator_pubkey,
&voting_keypair.pubkey(),
&solana_sdk::pubkey::new_rand(),
&Pubkey::new_unique(),
validator_stake_lamports,
VALIDATOR_LAMPORTS,
FeeRateGovernor::new(0, 0), // most tests can't handle transaction fees
Expand Down
Loading