feat!: network specific domain hashers #5980
Merged
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
brianp
changed the title
brianp
commented
Nov 23, 2023
brianp
force-pushed
the
network-domain-hashers
branch
from
a648abf
to
a0d3b96
Compare
SWvheerden
previously approved these changes
Nov 28, 2023
ghpbot-tari-project
removed
the
P-reviews_required
AaronFeickert
requested changes
Nov 28, 2023
brianp
force-pushed
the
network-domain-hashers
branch
from
b931592
to
f503b87
Compare
ghpbot-tari-project
added
the
P-reviews_required
| @@ -41,8 +42,9 @@ where D: Default | |||
| { | |||
| #[allow(clippy::new_ret_no_self)] | |||
| pub fn new(label: &'static str) -> ConsensusHasher<D> { | |||
| let network = *CURRENT_NETWORK.lock().unwrap(); | |||
There was a problem hiding this comment.
Would there be any critical consequences if this were to panic?
There was a problem hiding this comment.
I think it would result in an unusable service. Which is actually a pretty good scenario. I'd rather the service crash than do something like mangle or produce bad transactions.
brianp
force-pushed
the
network-domain-hashers
branch
from
f503b87
to
a44f059
Compare
We need to regenerate the faucets asap, but we won't do it in this PR. So we're temporarily disabling them.
brianp
force-pushed
the
network-domain-hashers
branch
from
a44f059
to
ca9c9e9
Compare
|
The changes look good, but on further thought, is it straightforward to write a sanity check that hashers for different networks are independent? That is, if you hash the same input data against two different networks, the output should at least be distinct. |
|
Straight forward enough it shouldn't be skipped. I've written the test now and will push it once I see the CI results and what else needs changing. |
Sounds good, thanks! I'd be surprised if the test uncovered any problems, but at least it's there in case of any future regressions. |
SWvheerden
approved these changes
Dec 1, 2023
This was referenced Dec 2, 2023
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.
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
Add a lazy static value that allows us to access what the current network is globally, so we can use the network byte to create network & domain-specific hashers.
Motivation and Context
This prevents the spending of duplicated UTXO's across networks as the hashes would compute differently based on the network byte.
Closes: #5652
How Has This Been Tested?
CI
Breaking Changes
This will invalidate all previous hashes on the network, and require both a network reset, with full data directory deletion.