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

PSBT support for deterministic bitcoin commitments (tapret and opret) #9

Merged
merged 19 commits into from
Dec 25, 2023

Conversation

dr-orlovsky
Copy link
Member

Finally completed the work on doing everything necessary for DBCs and MPCs in PSBTs. This is prerequisite for completing RGB support in PSBT and last bit of RGB wallet functionality

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

Attention: 474 lines in your changes are missing coverage. Please review.

Comparison is base (a200597) 52.7% compared to head (45841a7) 0.0%.

❗ Current head 45841a7 differs from pull request most recent head fe14cda. Consider uploading reports for the commit fe14cda to get more accurate results

Files Patch % Lines
psbt/src/csval/mpc.rs 0.0% 140 Missing ⚠️
psbt/src/data.rs 0.0% 76 Missing ⚠️
psbt/src/csval/tapret.rs 0.0% 75 Missing ⚠️
psbt/src/csval/opret.rs 0.0% 50 Missing ⚠️
psbt/src/maps.rs 0.0% 49 Missing ⚠️
psbt/src/csval/dbc.rs 0.0% 45 Missing ⚠️
derive/src/taptree.rs 0.0% 16 Missing ⚠️
derive/src/path.rs 0.0% 14 Missing ⚠️
derive/src/derive.rs 0.0% 6 Missing ⚠️
psbt/src/coders.rs 0.0% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master     #9      +/-   ##
========================================
- Coverage    52.7%   0.0%   -52.7%     
========================================
  Files          20     17       -3     
  Lines        2849   3143     +294     
========================================
- Hits         1501      0    -1501     
- Misses       1348   3143    +1795     
Flag Coverage Δ
rust 0.0% <0.0%> (-52.7%) ⬇️

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.

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

Can you check to see if these changes affect the ~250,000 MPC contract limit?

@@ -112,6 +120,13 @@ impl<'a> IntoIterator for &'a TapTree {
}

impl TapTree {
pub fn with_single_leaf(leaf: impl Into<LeafScript>) -> TapTree {
Self(vec![LeafInfo {
depth: u7::ZERO,
Copy link
Member

Choose a reason for hiding this comment

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

This is really a u7?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, since taptree can have up to 127 levels (and up to 2^127 leafs)

pub fn with_capacity(capacity: usize) -> Self {
Self {
leafs: Vec::with_capacity(capacity),
buoy: zero!(),
Copy link
Member

Choose a reason for hiding this comment

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

What is buoy?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dr-orlovsky
Copy link
Member Author

Can you check to see if these changes affect the ~250,000 MPC contract limit?

I will, but I do not see in any way how they can be affected by this code. It is BP library, not RGB, it has nothing to do with contracts. It just follows Bitcoin consensus and BIPs - that's it

@dr-orlovsky
Copy link
Member Author

I will, but I do not see in any way how they can be affected by this code. It is BP library, not RGB, it has nothing to do with contracts. It just follows Bitcoin consensus and BIPs - that's it

Sorry, wrong repo. Yes, the part you commented on is about DBCs and thus related to RGB contracts. But there is nothing which can affect it since the work is done in commit_verify repository and not here. We can't restrict it to the level of a standard library, it doesn't deal with the concept of the validity of commitments and trees and is just a helper tool to construct them (there can be more different tools in the future).

It is like libwally can't restrict the way Bitcoin consensus works

@cryptoquick
Copy link
Member

I see. This is high level and wouldn't affect consensus beyond what's specified in this library. I think I'm also confused, thank you for the clarification.

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

All questions addressed.

ACK

/// the value of tapret commitment has invalid length.
InvalidCommitment,

/// Using non-empty taptree is not supported in RGB v0.10. Please update.
Copy link
Member

Choose a reason for hiding this comment

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

Does this prerequisite remain in later versions?

Suggested change
/// Using non-empty taptree is not supported in RGB v0.10. Please update.
/// Using non-empty taptree is not supported in RGB v0.10 or later. Please update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe "is not yet supported" without a specific version? At the end this is not even an RGB library...

Copy link
Member

@crisdut crisdut Dec 18, 2023

Choose a reason for hiding this comment

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

Yes, does make sense.

We will revisit this topic when we support script-path spend, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This should be done right after v0.11 release. For now I just wanted to save time and do this non-trivial thing later when we have at least basic functionality working.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. This should be done right after v0.11 release. For now I just wanted to save time and do this non-trivial thing later when we have at least basic functionality working.

Yes, I agree with you, better focus on this topic later, because the rgb v0.11 topic is the epic of epics =)

@dr-orlovsky dr-orlovsky merged commit 584a361 into master Dec 25, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants