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(dan_layer)!: generate and add checkpoint signatures #4261

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Jul 4, 2022

Description

  • feat(dan_layer): generate and add checkpoint signatures
  • feat(explorer): update text explorer to include signatures
  • feat(base_layer/wallet): update wallet command file formats
  • feat(base_layer/core): wires up committee signatures to grpc with conversions
  • feat(base_layer/core): adds SignerSignature struct for CommitteeSignatures
  • feat(base_layer/wallet): include committee signatures for checkpoint GRPC requests

Motivation and Context

DAN committees need to collect signatures to include in checkpoints. This PR adds the initial implementation of that.
The signature challenge is:

e = H_1(signer_public_key || public_nonce || H_2(contract_id||commitment||merkle_root||checkpoint_number))

Although constructing a challenge with actual data is a TODO as that involves a bit of a refactor.

This PR also adds the SignerSignature struct that includes the signing public key along with the Schnorr signature tuple <R, s>. This is so that we know which if the validators signed. There may be an aggregate signature scheme that allows this.

How Has This Been Tested?

Manually, initial checkpoint contains signature.

@sdbondi sdbondi force-pushed the dan-layer-committee-checkpoint-signatures branch from 30feb7d to 1e28548 Compare July 4, 2022 15:24
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.

This looks good.
have two questions,
including one on the challenge.
Why make the challenge hash (xx||hash(xx))?
Surely we don't need the double hash, we can just concatenate all the data into a single hash once?

}

message SignerSignature {
bytes signer = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
bytes signer = 1;
bytes signer_pubkey = 1;

let request = grpc::CreateInitialAssetCheckpointRequest {
// TODO: contract id
contract_id: Vec::from_hex(asset_public_key)?,
merkle_root,
committee,
committee_signatures: None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I take it this is still test/initial code?
Surely the initial checkpoint needs to be signed by the committee?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this is in the collectibles app, which is a little in limbo at the moment

stringhandler
stringhandler previously approved these changes Jul 5, 2022
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

utACK.

@stringhandler
Copy link
Collaborator

@SWvheerden I don't see a problem with the double hash. The "true" challenge is hashed separately and then the Nonce and Pub Keys are committed to in the signature to prevent reuse.

@SWvheerden
Copy link
Collaborator

SWvheerden commented Jul 5, 2022

I don't see a problem with the double hash. The "true" challenge is hashed separately and then the Nonce and Pub Keys are committed to in the signature to prevent reuse.

Dont get me wrong, I think it's perfectly fine as is, was wondering why double hash it, a single hash is supposed to be perfectly random.

@sdbondi
Copy link
Member Author

sdbondi commented Jul 5, 2022

@SWvheerden basically it encapsulates that a SignerSignature must always commit to the public key and nonce, saves the caller from generating a nonce and potentially forgetting to include it in the challenge. The inner challenge will be domain separated.

@SWvheerden
Copy link
Collaborator

@SWvheerden basically it encapsulates that a SignerSignature must always commit to the public key and nonce, saves the caller from generating a nonce and potentially forgetting to include it in the challenge. The inner challenge will be domain separated.

Yeah signatures always need to commit to the pubkey and nonce, multisigs need to commit to the hash of the nonce, but that's not required here as its a single signer.
So the whole reason for having a part inner hashed as well, is to have it be domain separated?

@sdbondi
Copy link
Member Author

sdbondi commented Jul 5, 2022

@SWvheerden So the whole reason for having a part inner hashed as well, is to have it be domain separated?

Yeah more or less.
From the perspective of the SignerSignature, you don't need to provide a hash but just an arbitrary length byte slice.
We choose to hash first so that it is domain-separated. I don't see any reason not to do that.

@aviator-app aviator-app bot merged commit 0f581ca into tari-project:development Jul 6, 2022
@sdbondi sdbondi deleted the dan-layer-committee-checkpoint-signatures branch July 6, 2022 08:31
aviator-app bot pushed a commit that referenced this pull request Jul 7, 2022
Description
---
* feat(base_layer): added contract acceptance signature validation
* feat(dan_layer): created a new `AcceptanceManager` to centralize the creation, signing and submission of contract acceptances. It's used now both for manual and auto acceptances in the validator node.
* refactor(dan_layer): the method `get_current_contract_outputs` in `BaseNodeClient` now return `UtxoMinedInfo` objects. Needed for retrieving the `commitment` of the contract constitution, used in the acceptance signature
* refactor(integration_test): unifying of duplicated acceptance steps
* fix(integration_test): some fixtures files and steps are outdated

Motivation and Context
---
Contract acceptances need to be signed by the validator node submitting them. Also, the base layer should validate that the signature is valid.

Similar to [recent PRs](#4261), the acceptance signatures follow:
`e = H_1(signer_public_key || public_nonce || H_2(contract_id||constitution_commitment))`

How Has This Been Tested?
---
* New unit test to check the base layer validation of acceptance signatures
* For the validation node part, the existing integration test do not raise the signature error
aviator-app bot pushed a commit that referenced this pull request Jul 12, 2022
Description
---
* New base layer validation of checkpoint signatures: verifies that ALL the signatures present in the checkpoint are valid
* Unified the error type for invalid signatures across all contract output types

Motivation and Context
---
The base layer needs to check if all the signatures in a checkpoint are valid.

This PR is based on previous work on checkpoints (see #4261  and #4285). The checkpoint signature is calculated as:
`e = H_1(signer_public_key || public_nonce || H_2(contract_id||commitment||merkle_root||checkpoint_number))`

But take into account that the `commitment` is still a mock value as of now we still don't have a method for creating a shared value.

How Has This Been Tested?
---
New unit test checks that invalid signatures are detected
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