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: add an encrypted value to the TransactionOutput #4148

Conversation

therustmonk
Copy link
Contributor

@therustmonk therustmonk commented May 30, 2022

Description

The PR adds the encrypted_value field (of EncryptedValue type) to the TransactionOutput struct.
The same field also added to the TransactionInput for a case when input was produced from an output.

The changes also affected the genesis block, dibbler's faucet data, hashes and integration tests.

The value is not actually encrypted and now it's stored in the following format: u64le + 16 empty bytes.

Motivation and Context

Part of the BP+ features.

How Has This Been Tested?

CI tests (updated)

@therustmonk therustmonk force-pushed the transaction-output-encrypted-value branch 5 times, most recently from 3b6da02 to 43624b2 Compare June 7, 2022 16:10
@therustmonk therustmonk marked this pull request as ready for review June 7, 2022 16:59
hansieodendaal
hansieodendaal previously approved these changes Jun 8, 2022
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 72 to 84
impl ConsensusEncoding for EncryptedValue {
fn consensus_encode<W: Write>(&self, writer: &mut W) -> Result<(), io::Error> {
self.data.consensus_encode(writer)?;
self.0.consensus_encode(writer)?;
Ok(())
}
}

impl ConsensusDecoding for EncryptedValue {
fn consensus_decode<R: Read>(reader: &mut R) -> Result<Self, io::Error> {
let data = <[u8; 24]>::consensus_decode(reader)?;
Ok(Self { data })
Ok(Self(data))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a test would be in order for the encoding/decoding

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Looks good - one question

}
/// value: u64 + tag: [u8; 16]
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, Hash)]
pub struct EncryptedValue(#[serde(with = "hex::serde")] pub [u8; SIZE]);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we use hex to serialize this value?
This essentially doubles the storage required to 48 bytes.
You could implement the Hex trait from tari_utilities if you want to convert this value to and from hex

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've added hex support to read values from the dibbler_faucet.json file. Ok, I'll try to make it more coherently with other types we use in the TransactionOutput.

Copy link
Member

Choose a reason for hiding this comment

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

👍 I think you may need serialize_with, something like this https://stackoverflow.com/a/67148611/1077579
Essentially, using hex only when the serialisation is "human readable"

@@ -0,0 +1,105 @@
use std::{fmt, marker::PhantomData};
Copy link
Contributor

Choose a reason for hiding this comment

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

The licence text is missing here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved and fixed here: tari-project/tari_utilities#43

@@ -57,6 +57,7 @@ mod committee_definition_features;
mod encrypted_value;
mod error;
mod full_rewind_result;
mod hex_patch;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used? Maybe it should move to tari_utilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did that: tari-project/tari_utilities#43

@therustmonk therustmonk force-pushed the transaction-output-encrypted-value branch 4 times, most recently from bd0723e to 3aa0c04 Compare June 16, 2022 12:53
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 16, 2022
@therustmonk therustmonk force-pushed the transaction-output-encrypted-value branch 2 times, most recently from 110a5fc to cfdbf6b Compare June 16, 2022 20:12
@therustmonk therustmonk force-pushed the transaction-output-encrypted-value branch from cfdbf6b to 8490aea Compare June 16, 2022 20:45
hansieodendaal and others added 2 commits June 17, 2022 12:49
- Added FFI methods to for the encrypted value
- Fixed the failing unit test
- Added new FFI tests
@therustmonk therustmonk requested a review from sdbondi June 17, 2022 11:31
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

LGTM - not tested

/// that will produce a decrypted value from self.
pub fn todo_decrypt(&self) -> u64 {
let mut buffer = [0u8; 8];
(&mut buffer[0..8]).copy_from_slice(&self.0.as_slice()[0..8]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(&mut buffer[0..8]).copy_from_slice(&self.0.as_slice()[0..8]);
buffer[0..8].copy_from_slice(&self.0[0..8]);

@aviator-app aviator-app bot merged commit 01b600a into tari-project:development Jun 17, 2022
sdbondi added a commit to sdbondi/tari that referenced this pull request Jun 20, 2022
* development:
  feat(wallet): new cli commands to initialise proposals and amendments (tari-project#4205)
  feat: add an encrypted value to the TransactionOutput (tari-project#4148)
  feat: use tari_crypto's updated "extended pedersen commitment factory" (tari-project#4206)
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.

3 participants