thread local chain type vs global chain type#3327
Conversation
This makes testing more explicit and significantly more robust.
quentinlesceller
left a comment
There was a problem hiding this comment.
Found no issues. So 👍. Couple minor comments.
|
|
||
| /// PoW test mining and verifier context | ||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] | ||
| pub enum PoWContextTypes { |
| /// The mining parameter mode | ||
| pub static ref CHAIN_TYPE: RwLock<ChainTypes> = | ||
| RwLock::new(ChainTypes::Mainnet); | ||
| thread_local! { |
There was a problem hiding this comment.
nice didn't know about that macro.
| #[test] | ||
| fn too_large_block() { | ||
| global::set_mining_mode(ChainTypes::AutomatedTesting); | ||
| global::set_local_chain_type(ChainTypes::AutomatedTesting); |
There was a problem hiding this comment.
test_setup()or global::set_local_chain_type(global::ChainTypes::AutomatedTesting);?
Overall test_setup() don't seems necessary no?
There was a problem hiding this comment.
Good catch. I just want to use test_setup() to avoid all the repeated calls to set_local_chain_type() as this should be common across all the tests.
| #[test] | ||
| fn compact_block_single_tx_serialized_size() { | ||
| global::set_mining_mode(ChainTypes::AutomatedTesting); | ||
| global::set_local_chain_type(ChainTypes::AutomatedTesting); |
There was a problem hiding this comment.
Same comment as above.
| #[test] | ||
| fn empty_compact_block_serialized_size() { | ||
| global::set_mining_mode(ChainTypes::AutomatedTesting); | ||
| global::set_local_chain_type(ChainTypes::AutomatedTesting); |
There was a problem hiding this comment.
Same comment as above.
| } | ||
| fn default_mineable_max_weight() -> usize { | ||
| global::max_block_weight() | ||
| consensus::MAX_BLOCK_WEIGHT |
There was a problem hiding this comment.
Ah good catch. This was necessary because max_block_weight() relies on chain_type internally. So we have a default value that cannot be used until we know (and have configured) the chain_type itself.
The default config needs to be genuine defaults, without any dependencies on other, possibly external, configuration.
In many tests this actually used Mainnet chain_type to determine default_max_weight before we set chain_type to AutomatedTesting because some other config value was read.
| None => { | ||
| if GLOBAL_CHAIN_TYPE.is_init() { | ||
| let chain_type = GLOBAL_CHAIN_TYPE.borrow(); | ||
| set_local_chain_type(chain_type); |
There was a problem hiding this comment.
Out of curiosity, why do we need to set a per thread value here? Wouldn't returning GLOBAL_CHAIN_TYPE.borrow() enough?
There was a problem hiding this comment.
Yeah we technically don't need to use the thread local in the case where the global is set.
But I think its better for -
- consistency (tests share the same code path for more code)
- possibly performance as once the value is "cached" in the thread we no longer need to acquire a read lock on the global
RwLockwhen reading this.
Our global
CHAIN_TYPEconfig has been driving me crazy for a while now.We have tests that do not reliably run in parallel in separate threads due to the global
CHAIN_TYPEbeing overwritten by other tests. Some tests requireMainnetas we test specific PoW params etc.This PR breaks out the following -
Reads via
get_chain_type()always go via the thread localCHAIN_TYPE.This will be lazily initialized based on
GLOBAL_CHAIN_TYPEis not explicitly set.init_global_chain_type()to be called (once) during node startupOneTimeutil internallyget_chain_type()reads the thread local, lazily inits it from global chain typeset_mining_mode()toset_local_chain_type()set_local_chain_type()within the context of a single test threadPOW_CONTEXT_TYPEandPowContextType(seemingly unused)TODO
set_mining_mode()called in various tests)Related: mimblewimble/grin-wallet#409