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: enable revealed-value proofs #5983

Merged

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Nov 23, 2023

Description

Enabled revealed-value proofs for all output types, which are be controlled via the consensus constants. Range-proof types are mapped to output types.

From the merge mining proxy/miner log, coinbase output showing range_proof_type: RevealedValue and Proof: None:

image

Note: The faucets changed due to RangeProofType now being parsed with snake case - "range_proof_type":"RevealedValue" changed to "range_proof_type":"revealed_value" on every output line.

Motivation and Context

See #5968

How Has This Been Tested?

Added unit tests
Unit tests pass
Cucumber tests pass
System-level tests [TO BE COMPLETED]

What process can a PR reviewer use to test or verify this change?

Code walkthrough
Review the additional unit tests
Run system-level tests

Breaking Changes

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

Enabled revealed-value proofs for all output types, which are be controlled
via the consensus constants. Range proof types are mapped to output types.
@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 253 tests   1 253 ✔️  10m 22s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit ee25d1e.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 23, 2023

Test Results (Integration tests)

  2 files  +  2  11 suites  +11   55m 15s ⏱️ + 55m 15s
31 tests +31  29 ✔️ +29  0 💤 ±0  2 +2 
34 runs  +34  30 ✔️ +30  0 💤 ±0  4 +4 

For more details on these failures, see this check.

Results for commit ee25d1e. ± Comparison against base commit 89b19f6.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added the CR-too_long Changes Requested - Your PR is too long label Nov 24, 2023
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, I think we just need to make the defaults revealed value so we can test them

applications/minotari_miner/src/config.rs Outdated Show resolved Hide resolved
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 27, 2023
@SWvheerden SWvheerden merged commit f3f5879 into tari-project:development Nov 27, 2023
13 of 14 checks passed
@hansieodendaal hansieodendaal deleted the ho_revealed_coinbases branch November 27, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR-too_long Changes Requested - Your PR is too long P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants