feat: commitment proofs #6463
feat: commitment proofs #6463
Conversation
ghpbot-tari-project
added
P-acks_required
Test Results (CI) 3 files 123 suites 39m 40s ⏱️ Results for commit 54f290c. |
Test Results (Integration tests) 2 files + 2 1 errors 9 suites +9 11m 33s ⏱️ + 11m 33s For more details on these parsing errors and failures, see this check. Results for commit 54f290c. ± Comparison against base commit 310a470. |
There was a problem hiding this comment.
This looks good as a proof of view access. Not necessarily proof of ownership.
We need to move the proof generation to inside the key manager. The services should not work with the private keys. There is a lot of complexity there and it should be kept there.
The ownership proof will only currently work for your own wallet as it searches only your own wallet for utxos.
| } | ||
| }, | ||
| VerifyCommitmentProof(args) => { | ||
| match output_service.get_unspent_outputs().await { |
There was a problem hiding this comment.
this will only allow you to verify your own commitments
There was a problem hiding this comment.
If you want to verify any output you probably need a new method that will get that from a base node
| @@ -130,7 +133,7 @@ impl<TBackend, TWalletConnectivity, TKeyManagerInterface> | |||
| where | |||
| TBackend: OutputManagerBackend + 'static, | |||
| TWalletConnectivity: WalletConnectivityInterface, | |||
| TKeyManagerInterface: TransactionKeyManagerInterface, | |||
| TKeyManagerInterface: SecretTransactionKeyManagerInterface, | |||
There was a problem hiding this comment.
| TKeyManagerInterface: SecretTransactionKeyManagerInterface, | |
| TKeyManagerInterface: TransactionKeyManagerInterface, |
Dont include this trait. You are not suppose to expose the private keys here
| } | ||
| } | ||
|
|
||
| async fn create_commitment_proof( |
There was a problem hiding this comment.
ship most of the work to the key manager.
Its the only place where we should work with private keys.
|
@SWvheerden thanks for the review! This was originally based on the design in #6240, but certainly looks like it needs to be changed to be properly useful. |
There was a problem hiding this comment.
Looks nice, and I agree that we should keep the secret keys in the key manager.
I initially thought that we should guard against someone just pulling an existing range proof from the blockchain and present it as an ownership proof, but then realized that the transcript is unique.
What is interesting, is that we could create this proof even if the actual output may have a revealed value proof.
Yep, this was very intentional. It's not possible to collide a commitment proof with a range proof. This is the case even in the "other direction", where an adversary tricks a user into including a malicious message in a commitment proof in an attempt to produce a valid range proof. |
Description
Adds commitment proofs, which prove knowledge of the opening of a commitment with an optional minimum value assertion.
Closes #6282. Supersedes #6348.
Motivation and Context
This PR adds commitment proofs. These proofs show that the prover knows the opening of a given commitment, optionally shows that the commitment binds to at least a specified minimum value, and bind to an arbitrary message to mitigate replay attacks.
Notably, they do not necessarily assert spend authority.
How Has This Been Tested?
This needs to be tested manually.
What process can a PR reviewer use to test or verify this change?
Test proof verification succeeds against valid values. Test proof verification fails against different commitments, minimum values, and messages.