Skip to content

Rewamp workflows on generating commitment ids#152

Merged
dr-orlovsky merged 26 commits into
masterfrom
v0.11
Feb 26, 2024
Merged

Rewamp workflows on generating commitment ids#152
dr-orlovsky merged 26 commits into
masterfrom
v0.11

Conversation

@dr-orlovsky

@dr-orlovsky dr-orlovsky commented Feb 1, 2024

Copy link
Copy Markdown
Member

This is the result of self-audit for the commitment ID constructions. In v0.10 we had a very non-transparent commitment workflow with multiple redundant hashing, absence of clear tagged hash structure etc. This PR linearizes commitment workflows, streamlines and simplifies related APIs.

This is consensus-breaking since generated commitments will differ.

Summary of changes:

  • All commitments are now created with a single procedure
  • CommitEncode now implemented only for types generating the final commitment id. This prevents multiple bugs when commit encode was calling strict encoding, and the commitment-encode code in the subtypes was never executed
  • Fixed relation between commit-encode and conceal procedures
  • MPC Merkle trees now commit to depth, cofactor and other metadata
  • New CommitEncoder type which logs all commitment workflow and is able to document it right from the runtime

@dr-orlovsky dr-orlovsky added bug Something isn't working epic Epic task *security* Issues affecting safety/security (include undefined behaviours) *consensus* Issues affecting distributed concensus refactoring Refactoring of the existing code labels Feb 1, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Feb 1, 2024
@codecov

codecov Bot commented Feb 1, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 75.11521% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 76.7%. Comparing base (c604733) to head (5359d84).

❗ Current head 5359d84 differs from pull request most recent head b7df64a. Consider uploading reports for the commit b7df64a to get more accurate results

Files Patch % Lines
commit_verify/src/id.rs 50.0% 26 Missing ⚠️
commit_verify/src/merkle.rs 72.2% 15 Missing ⚠️
commit_verify/derive/src/derive.rs 81.8% 4 Missing ⚠️
commit_verify/src/mpc/block.rs 81.8% 4 Missing ⚠️
commit_verify/derive/src/params.rs 75.0% 3 Missing ⚠️
commit_verify/derive/tests/base.rs 96.9% 1 Missing ⚠️
commit_verify/src/mpc/atoms.rs 75.0% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##           master    #152     +/-   ##
========================================
- Coverage    82.6%   76.7%   -5.9%     
========================================
  Files          20      19      -1     
  Lines        2076    1710    -366     
========================================
- Hits         1714    1311    -403     
- Misses        362     399     +37
Flag Coverage Δ
rust 76.7% <75.1%> (-5.9%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cryptoquick
cryptoquick previously approved these changes Feb 1, 2024
View reviewed changes

@cryptoquick cryptoquick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are obviously major improvements
ACK

@oacdutra

oacdutra commented Feb 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Hi @dr-orlovsky,

I found an error related with this implementation in rgb-core, see here: RGB-WG/rgb-core#204 (comment)

@dr-orlovsky

dr-orlovsky commented Feb 10, 2024

Copy link
Copy Markdown
Member Author

@crisdut The only way to solve the issue seems to do another re-wamp. WIP.

@dr-orlovsky dr-orlovsky marked this pull request as ready for review February 12, 2024 00:41
@dr-orlovsky dr-orlovsky merged commit a959681 into master Feb 26, 2024
@dr-orlovsky dr-orlovsky self-assigned this Feb 26, 2024
@compiler-errors compiler-errors mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cryptoquick cryptoquick cryptoquick left review comments

@oacdutra oacdutra Awaiting requested review from oacdutra

@nicbus nicbus Awaiting requested review from nicbus

@zoedberg zoedberg Awaiting requested review from zoedberg

Assignees

@dr-orlovsky dr-orlovsky

Labels

bug Something isn't working *consensus* Issues affecting distributed concensus epic Epic task refactoring Refactoring of the existing code *security* Issues affecting safety/security (include undefined behaviours)

Projects

Releases & roadmap
Status: Done

Milestone

v0.11.0

Development

Successfully merging this pull request may close these issues.

3 participants

@dr-orlovsky @oacdutra @cryptoquick