refactor: better network global #6014
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghpbot-tari-project
added
P-acks_required
Test Results (Integration tests) 2 files + 2 12 suites +12 1h 23m 51s ⏱️ + 1h 23m 51s For more details on these failures, see this check. Results for commit a8d3dda. ± Comparison against base commit dee1892. ♻️ This comment has been updated with latest results. |
SWvheerden
reviewed
Dec 5, 2023
SWvheerden
previously approved these changes
Dec 5, 2023
ghpbot-tari-project
removed
the
P-reviews_required
Co-authored-by: SW van Heerden <[email protected]>
ghpbot-tari-project
added
the
P-reviews_required
ghpbot-tari-project
removed
the
P-reviews_required
AaronFeickert
requested changes
Dec 5, 2023
| let mut current_network = CURRENT_NETWORK.lock()?; | ||
| *current_network = network; | ||
| Ok(()) | ||
| // let mut current_network = Network::current(); |
There was a problem hiding this comment.
Suggested change
| // let mut current_network = Network::current(); |
Comment on lines
+160
to
+161
| // assert_eq!(Network::current(), Network::Esmeralda); | ||
|
|
There was a problem hiding this comment.
Suggested change
| // assert_eq!(Network::current(), Network::Esmeralda); |
| // assert_eq!(Network::current(), Network::Esmeralda); | ||
|
|
||
| // Generate a mainnet hash | ||
| // Network::set_current(Network::MainNet); |
There was a problem hiding this comment.
Suggested change
| // Network::set_current(Network::MainNet); |
|
|
||
| // They should be distinct | ||
| assert_ne!(hash_mainnet, hash_stagenet); | ||
|
|
|
|
||
| // Valid networks (we can't test against other binary configurations) | ||
| for network in [Network::LocalNet, Network::Igor, Network::Esmeralda] { | ||
| assert!(set_network_if_choice_valid(network).is_ok()); |
There was a problem hiding this comment.
This fails for me locally on the first trip through the loop. The issue is that Network::set_current returns an error once the value has been initialized. I don't think this behavior is correct.
SWvheerden
pushed a commit
that referenced
this pull request
Dec 8, 2023
Description --- Refactors the handling of network-based hashing operations introduced in #5980 to better handle read operations. Adds a sanity test for hash independence. Supersedes #6014. Closes #6003. Motivation and Context --- Recent work in #5980 binds the current network into consensus hashing. It uses a `Mutex`, which has two subtle issues. First, it allows the network to be set multiple times, which should not occur. Second, it locks on reads, which is unnecessary and inefficient. This PR updates how the current network is handled. It adds `Network::get_current_or_default` that will return either the current network (if it has been set) or the default network (if it has not). It adds `Network:set_current` that attempts to set the network; if it has been set before, this operation will fail. Note that calling `Network::get_current_or_default` does _not_ set the network for this purpose. It modifies the API for consensus hashing to add a wrapper that uses the current network. This wraps functionality allowing for specification of a network, which is useful for testing. On top of this new API, a new test is added that checks for distinct hashes using the same input but different networks. This is not comprehensive, but will detect obvious issues. How Has This Been Tested? --- Existing tests pass. A new test passes. What process can a PR reviewer use to test or verify this change? --- Check that the tests do what they claim. Check that the updates to consensus hashing properly introduce the expected wrapping functionality. Check that the updated network API does what it is supposed to.
|
Closing this for: #6004 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Removes the mutex and allows tests to run in parallel
Motivation and Context
Built on #6004
The previous versions of these test relied on setting the Network globally. If other tests are running at the same time, these tests can fail because the network is switched. I investigated using ThreadLocalStorage and TaskLocalStorage from tokio, but these could result threads having different networks set and it would be hard to confirm that the entire process was using the correct network.
I opted instead to ignore the
set_networktest and add a parameter to the DomainSeparatedHasher to allow a different network to be tested.How Has This Been Tested?
Ran cargo test
What process can a PR reviewer use to test or verify this change?
This fixes tests and provides a cleaner interface
Breaking Changes