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

Implementation of Revealed Merge operation. #23

Merged
merged 11 commits into from Mar 14, 2021
Merged

Implementation of Revealed Merge operation. #23

merged 11 commits into from Mar 14, 2021

Conversation

rajarshimaitra
Copy link
Contributor

Objective: To merge two equivalent state data of differing confidentiality. The resulting merge will produce the most revealed state combining the confidentiality of provided two input states.

Application: Upon receiving a new consignment, rgb-node should try to merge the incoming data with already known state data, before committing the consignment into stash. Outlined here RGB-WG/rgb-node#136 (comment)

Intended functionality is similar to what I tried to do in this draft RGB-WG/rgb-node#137. But this will be more structured.

This is the required modification needed in rgb-core in order to have merging capability in rgb-node. Once this gets finalized and merged, I will work on extending the functionality into the node.

Note: This is probably critical, as doing a faulty merge can result in fund loss. I have added some basic unit tests to ensure basic operations are working. But more edge cases might need to be considered. Suggestions welcome.

There are some #TODO and #Question comments in the code, which i would like some suggestions on.

@dr-orlovsky dr-orlovsky added bug Something isn't working enhancement New feature or request labels Mar 4, 2021
@dr-orlovsky dr-orlovsky added this to Proposed solution in Quality Assesment Mar 4, 2021
@dr-orlovsky dr-orlovsky moved this from Proposed solution to Solutions in review in Quality Assesment Mar 4, 2021
@dr-orlovsky dr-orlovsky added this to the v0.4.0 milestone Mar 4, 2021
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Overall this is correct, but in terms of API I would think on applying the procedure to the &mut self and returning bool instead of Result. This will simplify the code and remove unnecessary allocations. Finally, merge procedure assumes that we acquire new knowledge and it is fine to apply it to the original data without cloning them (they are anyway in-memory copy only and a dedicated save procedure is needed for their persistence).

src/contract/assignments.rs Outdated Show resolved Hide resolved
src/contract/nodes.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Mar 6, 2021

Overall this is correct, but in terms of API I would think on applying the procedure to the &mut self and returning bool instead of Result.

It feels to me Error message will be useful for this API. As we will impl the same trait in a cascading fashion to multiple structures, we would need error information to see merge operation on which structure is failing and why. I feel meaningful error message is a must for this type of API otherwise it will be a nightmare to debug failures.

Considering your comments I am changing the trait function definition as merge_revelaed(&mut self, &self) -> Result<bool, String>, Both to add boolean return and error message if it returns false.

I was also thinking of adding a merge(&Self, &Self) -> Self helper function to the trait. It would do reallocation and create a new merged structure. But this requires Copy for state data types. Would this be of any help? Otherwise, I will skip it.

Update: removing above helper method as state data cant be Copy.

@rajarshimaitra
Copy link
Contributor Author

Updated with review changes.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Agree on returning error, but this should be a dedicated error type, not a string. Stings as errors are very bad because of two reasons:

  1. they require allocation and not copyable
  2. they do not conform std::error::Error trait which make using them inside other errors huge pain

merge::{ pub struct UnmatchedError; } is Copy and just a u8, while String("long error description") is Clone and allocation in the heap, passed as u64 pointer around.

Second, we need to change trait signature once again, to avoid cloning everything around inside....

src/contract/assignments.rs Outdated Show resolved Hide resolved
@@ -150,6 +151,25 @@ impl ToMerkleSource for OwnedRightsInner {
}
}

impl MergeRevealed for OwnedRights {
Copy link
Member

Choose a reason for hiding this comment

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

Why we have it here while the rest of implementations are in a standalone file merge.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because OwnedRightsInner has pub(super) visibility defined. Because I am using OwnedRightsInner to check the consistency of two OwnedRights, it had to be implemented in the assignment.rs.

If we can change the visibility of OwnedRightsInner to pub, then we can move this to merge.rs.

@@ -150,6 +151,25 @@ impl ToMerkleSource for OwnedRightsInner {
}
}

impl MergeRevealed for OwnedRights {
fn merge_revealed(&mut self, other: &Self) -> Result<bool, String> {
if OwnedRightsInner(self.clone())
Copy link
Member

Choose a reason for hiding this comment

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

We should not use clone while we did the func &mut self to avoid cloning=memory allocations! to_merke_source does not need to own the data, so probably it will be better to have MergeRevealed implemented for inner type...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes understood. Changing the trait signature to take ownership of the data to avoid any cloning. I hope taking ownership of both the data will not cause many problems downstream.

src/contract/assignments.rs Outdated Show resolved Hide resolved
src/stash/merge.rs Outdated Show resolved Hide resolved
src/stash/merge.rs Outdated Show resolved Hide resolved
src/stash/merge.rs Outdated Show resolved Hide resolved
src/stash/merge.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

Updated as per review comments.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

LGTM with the quick view, seems that everything is in place. Will be able to dig deeper Friday.

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Mar 10, 2021

In the meanwhile: I Iike NodeType enum, can you pls move it to the src/contract/node.rs and do a pub trait Node { fn node_type(&self) -> NodeType } thing (plus its impls for Genesis, Node and Extension)? As a separate PR probably.

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Mar 11, 2021

In the meanwhile: I Iike NodeType enum, can you pls move it to the src/contract/node.rs and do a pub trait Node { fn node_type(&self) -> NodeType } thing (plus its impls for Genesis, Node and Extension)? As a separate PR probably.

Silly me. This enum was already there https://github.com/rgb-org/rgb-core/blob/53f797c5f97d45630f73816a855f80cb1b72dce9/src/schema/nodes.rs#L49-L67

node_type() is also defined for Node trait.

updating this PR to use schema::NodeType.

UkolovaOlga added a commit to LNP-BP/devcalls that referenced this pull request Mar 11, 2021
Agenda:
- Double consignments updates
- Disclosures: RGB-WG/rgb-core#26
- Multiple asset transfers RGB-WG/rgb-node#148
- Revealed-merge procedure RGB-WG/rgb-core#23
- Command-line tools:
     * PSBT
     * RGB
     * Invoice
@dr-orlovsky
Copy link
Member

Pls see my last two commits: there were still number of unnecessary allocations which I removed. Not happy with the rest still, but do not see how to avoid collection allocations, so merging to get v0.4 released

@dr-orlovsky dr-orlovsky merged commit df9b4d6 into RGB-WG:master Mar 14, 2021
Quality Assesment automation moved this from Solutions in review to Done Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants