-
Notifications
You must be signed in to change notification settings - Fork 427
Improve test determinism with configurable connect style and deterministic hash maps #4296
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4563,7 +4563,29 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>( | |
| let mut nodes = Vec::new(); | ||
| let chan_count = Rc::new(RefCell::new(0)); | ||
| let payment_count = Rc::new(RefCell::new(0)); | ||
| let connect_style = Rc::new(RefCell::new(ConnectStyle::random_style())); | ||
|
|
||
| let connect_style = Rc::new(RefCell::new(match std::env::var("LDK_TEST_CONNECT_STYLE") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some way to make this more discoverable? Maybe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point to add more docs. It is a bit confronting that we add and document an env var that underlines that there are non-deterministic unit tests, instead of making them deterministic. We could also think about alternatives such as a 'deterministic_test' cfg flag, which we can then also use to add for example sorting when we iterate over hashmap/set keys?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added more docs and also added a commit for deterministic hash table iteration. |
||
| Ok(val) => match val.as_str() { | ||
| "BEST_BLOCK_FIRST" => ConnectStyle::BestBlockFirst, | ||
| "BEST_BLOCK_FIRST_SKIPPING_BLOCKS" => ConnectStyle::BestBlockFirstSkippingBlocks, | ||
| "BEST_BLOCK_FIRST_REORGS_ONLY_TIP" => ConnectStyle::BestBlockFirstReorgsOnlyTip, | ||
| "TRANSACTIONS_FIRST" => ConnectStyle::TransactionsFirst, | ||
| "TRANSACTIONS_FIRST_SKIPPING_BLOCKS" => ConnectStyle::TransactionsFirstSkippingBlocks, | ||
| "TRANSACTIONS_DUPLICATIVELY_FIRST_SKIPPING_BLOCKS" => { | ||
| ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks | ||
| }, | ||
| "HIGHLY_REDUNDANT_TRANSACTIONS_FIRST_SKIPPING_BLOCKS" => { | ||
| ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks | ||
| }, | ||
| "TRANSACTIONS_FIRST_REORGS_ONLY_TIP" => ConnectStyle::TransactionsFirstReorgsOnlyTip, | ||
| "FULL_BLOCK_VIA_LISTEN" => ConnectStyle::FullBlockViaListen, | ||
| "FULL_BLOCK_DISCONNECTIONS_SKIPPING_VIA_LISTEN" => { | ||
| ConnectStyle::FullBlockDisconnectionsSkippingViaListen | ||
| }, | ||
| _ => panic!("Unknown ConnectStyle '{}'", val), | ||
| }, | ||
| Err(_) => ConnectStyle::random_style(), | ||
| })); | ||
|
|
||
| for i in 0..node_count { | ||
| let dedicated_entropy = DedicatedEntropy(RandomBytes::new([i as u8; 32])); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,75 @@ | |
| pub use hashbrown::hash_map; | ||
|
|
||
| mod hashbrown_tables { | ||
| #[cfg(feature = "std")] | ||
| #[cfg(all(feature = "std", not(test)))] | ||
| mod hasher { | ||
| pub use std::collections::hash_map::RandomState; | ||
| } | ||
| #[cfg(all(feature = "std", test))] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, test determinism is great, but test coverage is better. I'm a bit skeptical that the solution to "sometimes my tests are flaky due to hashmap iteration order" is to fix the hashmap iteration order, rather than fixing the tests. Having an option in the env like you do above to fix hashmap iteration order to make debugging easier seems reasonable, however.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. It's not about flakey tests btw, but about making test output comparable across runs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, I've def had it make tests flaky before :) |
||
| mod hasher { | ||
| #![allow(deprecated)] // hash::SipHasher was deprecated in favor of something only in std. | ||
| use core::hash::{BuildHasher, Hasher}; | ||
|
|
||
| /// A [`BuildHasher`] for tests that supports deterministic behavior via environment variable. | ||
| /// | ||
| /// When `LDK_TEST_DETERMINISTIC_HASHES` is set, uses fixed keys for deterministic iteration. | ||
| /// Otherwise, delegates to std's RandomState for random hashing. | ||
| #[derive(Clone)] | ||
| pub enum RandomState { | ||
| Std(std::collections::hash_map::RandomState), | ||
| Deterministic, | ||
| } | ||
|
|
||
| impl RandomState { | ||
| pub fn new() -> RandomState { | ||
| if std::env::var("LDK_TEST_DETERMINISTIC_HASHES").map(|v| v == "1").unwrap_or(false) | ||
| { | ||
| RandomState::Deterministic | ||
| } else { | ||
| RandomState::Std(std::collections::hash_map::RandomState::new()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Default for RandomState { | ||
| fn default() -> RandomState { | ||
| RandomState::new() | ||
| } | ||
| } | ||
|
|
||
| /// A hasher wrapper that delegates to either std's DefaultHasher or a deterministic SipHasher. | ||
| pub enum RandomStateHasher { | ||
| Std(std::collections::hash_map::DefaultHasher), | ||
| Deterministic(core::hash::SipHasher), | ||
| } | ||
|
|
||
| impl Hasher for RandomStateHasher { | ||
| fn finish(&self) -> u64 { | ||
| match self { | ||
| RandomStateHasher::Std(h) => h.finish(), | ||
| RandomStateHasher::Deterministic(h) => h.finish(), | ||
| } | ||
| } | ||
| fn write(&mut self, bytes: &[u8]) { | ||
| match self { | ||
| RandomStateHasher::Std(h) => h.write(bytes), | ||
| RandomStateHasher::Deterministic(h) => h.write(bytes), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl BuildHasher for RandomState { | ||
| type Hasher = RandomStateHasher; | ||
| fn build_hasher(&self) -> RandomStateHasher { | ||
| match self { | ||
| RandomState::Std(s) => RandomStateHasher::Std(s.build_hasher()), | ||
| RandomState::Deterministic => { | ||
| RandomStateHasher::Deterministic(core::hash::SipHasher::new_with_keys(0, 0)) | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| } | ||
| #[cfg(not(feature = "std"))] | ||
| mod hasher { | ||
| #![allow(deprecated)] // hash::SipHasher was deprecated in favor of something only in std. | ||
|
|
||
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.
I'm confused how this works in our no-std tests, like how CI is passing
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.
no-stdtests are actually (secretly) built withstdenabled. Its useful lol.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.
Interesting. What was the original reason for that?
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.
IIRC cargo actually forces it on us, but we take advantage of it. Its possible we screwed up our feature flags at some point and accidentally did it, but either way its useful.