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

Asset state transition node ID mutability #7

Closed
claudiosdc opened this issue Feb 8, 2021 · 8 comments · Fixed by #8
Closed

Asset state transition node ID mutability #7

claudiosdc opened this issue Feb 8, 2021 · 8 comments · Fixed by #8
Assignees
Labels
bug Something isn't working *consensus* Issues affecting distributed concensus epic Epic task covering multiple steps of implementation refactoring Code refactoring
Milestone

Comments

@claudiosdc
Copy link
Contributor

During my tests and investigation of RGB node's internals, I have noticed a particular behavior that does not seem to be there by design but rather to be an undesirable side effect: the fact that the node ID of an asset state transition data structure will mutate as its internal assignments are concealed/revealed.

As observed, the aforementioned behavior contributes for some data inconsistencies, one of which has already ben reported as an issue (#130) and for which a pull request (#131) with a proposed fix has already been opened. Another one of such data inconsistencies is described below.

During the accept phase of an asset transfer operation, the state transition added to the receiving RGB node's stash has a node ID (which is shown as its filename) different from the node ID of the new entry added to the known_allocations field of the asset data structure maintained in the RGB node's cache (which can be retrieved by means of the fungible list -l command), even though they refer to the same asset state transition.

This particular data inconsistency is due to the fact that the asset state transition extracted from the consignment data structure has the seal of the assignment used to allocate the transferred assets to the receiving endpoint revealed before the state transition is added to the stash. In contrast, the original state transition (with the concealed seal) is used when adding the new asset allocation to the asset data structured maintained in the cache.

That same misbehavior seems to apply to asset state extensions, even though there are no evidences to confirm it as that functionality does not seem to be fully implemented.

@dr-orlovsky
Copy link
Member

You are absolutely right, node id must not depend on whether any of the state transition data are concealed or revealed: it must internally conceal all the data and compute the ide as a hash of the data serialized from the concealed state. If the behavior is different, that is a bug.

@dr-orlovsky
Copy link
Member

Confirm that the bug exists. The code causing it is here: RGB-WG/rgb-node#130 (comment) and here https://github.com/rgb-org/rgb-core/blob/master/src/contract/nodes.rs#L625-L639

Instead of using strict_encode for the consensus commit procedure the structs must explicitly implement CommitEncode and use commit encode (not strict!) methods on their fields (these methods will conceal & merkelize owned rights and metadata: https://github.com/rgb-org/rgb-core/blob/master/src/contract/metadata.rs#L75-L77 and https://github.com/rgb-org/rgb-core/blob/master/src/contract/data.rs#L51-L53)

@rajarshimaitra can you pls

  • work on the PR fixing this issue
  • do it agains master branch and version-specific branches (each one of them): we need to backport the issue fix to older releases
  • write tests checking that (a) specific conceal procedures do not modify commitment id value and (b) both metadata and owned rights are merkelized before being commit-encoded?

@rajarshimaitra
Copy link
Contributor

Working on this next.

@claudiosdc
Copy link
Contributor Author

Working on this next.

@rajarshimaitra, I have been working on a solution for this issue myself, and, yesterday, I was able to finalize a working solution. I do not have all the code change organized for a PR yet; I was planning to do it tomorrow (I usually do not work on Sundays). I just thought that you might be interested.

@rajarshimaitra
Copy link
Contributor

@claudiosdc Great. Feel free to open the PR at your suitability and I will review it.

UkolovaOlga referenced this issue in LNP-BP/devcalls Feb 15, 2021
10.02.2021 Agenda:
RGB QA

Issues from https://github.com/orgs/rgb-org/projects/11:

1. Properly handle result from 'validate' request to Stash daemon - RGB-WG/rgb-node#132
2. Asset state transition node ID mutability
    - https://github.com/rgb-org/rgb-node/issues/133
    - RGB-WG/rgb-node#131
    - Asset transfer validation is ineffective: RGB-WG/rgb-node#130
3. Question about fungible asset known allocations semantics - RGB-WG/rgb-node#134
4. Transfer change allocation not being registered - RGB-WG/rgb-node#129
5. Transaction output duplicated by 'fungible transfer' - RGB-WG/rgb-node#127
@claudiosdc
Copy link
Contributor Author

@rajarshimaitra, I have just opened the PR with the proposed fix. It's PR #6 of the rgb-core repository.

@rajarshimaitra
Copy link
Contributor

Noted. I will be out of town tomorrow. Will review asap.

@dr-orlovsky
Copy link
Member

Moving this issue to RGB Core repo

@dr-orlovsky dr-orlovsky transferred this issue from RGB-WG/rgb-node Feb 18, 2021
@dr-orlovsky dr-orlovsky added this to the v0.4.0 milestone Mar 4, 2021
@dr-orlovsky dr-orlovsky added bug Something isn't working epic Epic task covering multiple steps of implementation refactoring Code refactoring [CONSENSUS-CRITICAL] labels Mar 4, 2021
@dr-orlovsky dr-orlovsky added *consensus* Issues affecting distributed concensus and removed [CONSENSUS-CRITICAL] labels Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working *consensus* Issues affecting distributed concensus epic Epic task covering multiple steps of implementation refactoring Code refactoring
Projects
None yet
3 participants