Skip to content
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

feat!: network specific domain hashers #5980

Merged
merged 10 commits into from
Dec 1, 2023

Conversation

brianp
Copy link
Contributor

@brianp brianp commented Nov 23, 2023

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

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

This will invalidate all previous hashes on the network, and require both a network reset, with full data directory deletion.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Nov 23, 2023
Copy link

github-actions bot commented Nov 23, 2023

Test Results (CI)

1 259 tests   1 259 ✔️  10m 30s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit ca9c9e9.

♻️ This comment has been updated with latest results.

@brianp brianp changed the title feature: network specific domain hashers feat!: network specific domain hashers Nov 23, 2023
Copy link

github-actions bot commented Nov 23, 2023

Test Results (Integration tests)

  2 files  12 suites   1h 24m 32s ⏱️
32 tests 27 ✔️ 0 💤   5
41 runs  28 ✔️ 0 💤 13

For more details on these failures, see this check.

Results for commit ca9c9e9.

♻️ This comment has been updated with latest results.

@brianp brianp force-pushed the network-domain-hashers branch from a648abf to a0d3b96 Compare November 27, 2023 20:21
@SWvheerden SWvheerden marked this pull request as ready for review November 28, 2023 05:35
SWvheerden
SWvheerden previously approved these changes Nov 28, 2023
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 28, 2023
@brianp brianp force-pushed the network-domain-hashers branch from b931592 to f503b87 Compare November 29, 2023 10:18
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 29, 2023
@@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be any critical consequences if this were to panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 brianp force-pushed the network-domain-hashers branch from f503b87 to a44f059 Compare November 30, 2023 14:48
@brianp brianp force-pushed the network-domain-hashers branch from a44f059 to ca9c9e9 Compare November 30, 2023 14:51
Copy link
Collaborator

@AaronFeickert AaronFeickert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@AaronFeickert
Copy link
Collaborator

AaronFeickert commented Nov 30, 2023

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.

@brianp
Copy link
Contributor Author

brianp commented Nov 30, 2023

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.

@AaronFeickert
Copy link
Collaborator

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.

Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to merge this as is, the test I will mark as broken.
I tested this locally and it works.

@SWvheerden SWvheerden merged commit d7ab283 into tari-project:development Dec 1, 2023
13 of 14 checks passed
@brianp brianp deleted the network-domain-hashers branch December 4, 2023 07:36
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
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add network to domain seperated hashes
4 participants