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(base_layer/core): add domain hashing wrapper for consensus encoding #4381

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Aug 3, 2022

Description

  • adds DomainSeparatedConsensusHasher
  • renames ConsensusHashWriter to ConsensusHasher
  • removes Write implementation from ConsensusHasher
  • adds TransactionHashDomain hash domain
  • uses domain hashing for transaction metadata signature and script challenges

Motivation and Context

Creates an ergonomic and 0-alloc hasher for consensus encoded values.

Usage:

  DomainSeparatedConsensusHasher::<TransactionHashDomain>::new("metadata_signature")
            .chain(public_commitment_nonce)
            .chain(script)
            .chain(features)
            .chain(sender_offset_public_key)
            .chain(commitment)
            .chain(covenant)
            .chain(encrypted_value)
            .finalize()

How Has This Been Tested?

New unit test, existing tests use hashing

@hansieodendaal
Copy link
Contributor

hansieodendaal commented Aug 3, 2022

This looks nice.

I think one of the issues with the failing integration tests is the hash calculation in

  • buildMetaChallenge(
  • buildScriptChallenge(
  • hashOutput(features, commitment, script, sender_offset_public_key)

from what I could pick up. I was busy chasing some other failures after ^^ have been corrected before I closed #4373.

@hansieodendaal
Copy link
Contributor

Sorry, finger trouble closing this

@sdbondi
Copy link
Member Author

sdbondi commented Aug 3, 2022

@hansieodendaal lol no problem :) Oh right, I'll need to update cucumber tests. Thanks

stringhandler
stringhandler previously approved these changes Aug 3, 2022
@sdbondi sdbondi force-pushed the core-consensus-domain-hashing branch 5 times, most recently from 0aada87 to b14bf72 Compare August 4, 2022 05:32
@sdbondi
Copy link
Member Author

sdbondi commented Aug 4, 2022

Cucumber metadata signature is still broken, it seems to be the encrypted value is incorrect in the transactionBuilder. Going to leave that for another PR.

@sdbondi sdbondi force-pushed the core-consensus-domain-hashing branch from 20661f6 to 455d572 Compare August 4, 2022 07:14
* development:
  test(comms/core): fix broken DNS resolution test (tari-project#4385)
Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

LGTM. See comment.

fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
self.digest.update(buf);
/// This private struct wraps a Digest and implements the Write trait to satisfy the consensus encoding trait..
#[derive(Clone)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we provide a blanket Write impl on the underlying DomainHasher?

Copy link
Member Author

@sdbondi sdbondi Aug 4, 2022

Choose a reason for hiding this comment

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

In this case we can't because the API for Write is meant to allow "chunked" writes (network sockets, files etc) and the DomainSeparatedHasher does not allow this.

c = a||b
H.chain(a).chain(b) != H.chain(c)

This is the main reason why I chose to make the write interface internal to the hasher, because we know that ConseususEncoding writes each element in its entirety and our internal Write implementation never tells the caller to wait to write.

@aviator-app aviator-app bot merged commit ad11ec5 into tari-project:development Aug 4, 2022
@sdbondi sdbondi deleted the core-consensus-domain-hashing branch August 4, 2022 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants