-
Notifications
You must be signed in to change notification settings - Fork 106
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
Generate test chains that pass basic chain consistency tests #2221
Conversation
// after the vec strategy generates blocks, update the previous block hashes | ||
vec.prop_map(|mut vec| { | ||
let mut previous_block_hash = None; | ||
for block in vec.iter_mut() { | ||
if let Some(previous_block_hash) = previous_block_hash { | ||
block.header.previous_block_hash = previous_block_hash; | ||
} | ||
previous_block_hash = Some(block.hash()); | ||
} | ||
vec.into_iter().map(Arc::new).collect() | ||
}) | ||
.boxed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change fixes up the previous block hashes
/// Overrides for arbitrary [`LedgerState`]s. | ||
#[derive(Debug, Clone, Copy)] | ||
pub struct LedgerStateOverride { | ||
/// Regardless of tip height and network, every block has features from this | ||
/// network upgrade. | ||
pub network_upgrade_override: Option<NetworkUpgrade>, | ||
|
||
/// Every block has exactly one coinbase transaction. | ||
/// Transactions are always coinbase transactions. | ||
pub always_has_coinbase: bool, | ||
|
||
/// Every chain starts at this block. Single blocks have this height. | ||
pub height_override: Option<Height>, | ||
|
||
/// Every chain starts with a block with this previous block hash. | ||
/// Single blocks have this previous block hash. | ||
pub previous_block_hash_override: Option<block::Hash>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes it easier to generate different kinds of chains.
if let Some(previous_block_hash_override) = | ||
ledger_state.previous_block_hash_override | ||
{ | ||
previous_block_hash = previous_block_hash_override; | ||
} else if ledger_state.height == Height(0) { | ||
previous_block_hash = GENESIS_PREVIOUS_BLOCK_HASH; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code move makes header generation consistent with block generation
/// Make sure our genesis strategy generates blocks with the correct coinbase | ||
/// height and previous block hash. | ||
#[test] | ||
fn block_genesis_strategy() -> Result<()> { | ||
zebra_test::init(); | ||
|
||
let strategy = LedgerState::genesis_strategy(None).prop_flat_map(Block::arbitrary_with); | ||
|
||
proptest!(|(block in strategy)| { | ||
prop_assert_eq!(block.coinbase_height(), Some(Height(0))); | ||
prop_assert_eq!(block.header.previous_block_hash, GENESIS_PREVIOUS_BLOCK_HASH); | ||
}); | ||
|
||
Ok(()) | ||
} | ||
|
||
/// Make sure our partial chain strategy generates a chain with the correct coinbase | ||
/// heights and previous block hashes. | ||
#[test] | ||
fn partial_chain_strategy() -> Result<()> { | ||
zebra_test::init(); | ||
|
||
let strategy = LedgerState::genesis_strategy(None) | ||
.prop_flat_map(|init| Block::partial_chain_strategy(init, 3)); | ||
|
||
proptest!(|(chain in strategy)| { | ||
let mut height = Height(0); | ||
let mut previous_block_hash = GENESIS_PREVIOUS_BLOCK_HASH; | ||
for block in chain { | ||
prop_assert_eq!(block.coinbase_height(), Some(height)); | ||
prop_assert_eq!(block.header.previous_block_hash, previous_block_hash); | ||
height = Height(height.0 + 1); | ||
previous_block_hash = block.hash(); | ||
} | ||
}); | ||
|
||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests make sure the new test code actually works
let height = block::Height(ledger_state.tip_height.0 + 1); | ||
Self::arbitrary_with(Some(height)) | ||
Self::arbitrary_with(Some(ledger_state.height)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change lets us start generating blocks at genesis, rather than block 1
@@ -12,19 +12,24 @@ use crate::tests::Prepare; | |||
|
|||
use super::*; | |||
|
|||
const MAX_PARTIAL_CHAIN_BLOCKS: usize = 100; | |||
const MAX_PARTIAL_CHAIN_BLOCKS: usize = 102; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sure we go past the non-finalized state limit
type Value = ( | ||
Arc<Vec<PreparedBlock>>, | ||
<BinarySearch as ValueTree>::Value, | ||
Network, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change lets us use the right network to initialize the finalized state
// Disable NU5 for now | ||
// `genesis_strategy(None)` re-enables the default Nu5 override | ||
let ledger_strategy = LedgerState::genesis_strategy(Canopy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oxarbitrage this is the NU5 change you'll need to add in your PR after you merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, I am merging as is so we can continue the work in #2185.
Will make modifications in another PR if needed.
Motivation
In #2185, we want to generate random block chains that pass basic chain consistency tests, like:
Solution
The code in this pull request has:
Review
@oxarbitrage can review this PR for #2185