Fixing node_id mutability issue by incomplete concealements

[dr-orlovsky]

I believe this should be the least intrusive change fixing #7 according to guidelines from https://github.com/LNP-BP/LNPBPs/discussions/88. Conceal trait is a part of client-side-validation and MUST not be used for anything other than commitments. For confidentiality purposes we already use AutoConceal trait, which I believe should be renamed into IntoConfidential in order to avoid further confusion – and probably we should rename Conceal to CommitConceal ofr the same sake (however this will be a separate commits outside of the scope of this minimalistic PR).

[rajarshimaitra]

Concept ACK.

Also mentioning the following changes here for documenting TODOs.

• UsingConceal strategy here
  https://github.com/rgb-org/rgb-core/blob/830cc11130ed25580745435f67ddf4ff185083ef/src/contract/assignments.rs#L406-L408
  as per https://user-images.githubusercontent.com/372034/108346320-894b6f00-71df-11eb-98b6-360750df3add.png

• Define CommitEncode with explicite commit_encode() for each members of Transition, Extension, and Genesis nodes.

• Tests to ensure behavior.

[dr-orlovsky]

@rajarshimaitra you are right, will cherry-pick commits from @claudiosdc PR #6

[dr-orlovsky]

The finalization of this PR is pending LNP-BP/rust-lnpbp#179

[dr-orlovsky]

Finalized & ready for the review.

Will merge #18 to make sure that the node_id is now immutable.

Many tests fail since the commitment algorithm has changed. Would like @rajarshimaitra to check the tests which he authored to make sure that the logic is correct – and then update test constants with new commitments.

[dr-orlovsky]

Hooray!

test contract::nodes::test::test_transition_node_id ... ok

[dr-orlovsky]

BTW, we have a new rgb command-line tool which helps creating consignments out of YAML definitions (and verse back) - this should be useful for a detailed test cases

[rajarshimaitra]

Concept ACK. Will update the test cases.

I feel the encoding and commitment API is too numerous at this point, and I am pretty sure I myself will lose track of which is for what within few weeks. So it would be better if we can make some documentation somewhere explaining the purpose of each for future reference and to help out future developers.

[claudiosdc]

The changes seem to be consistent and, in fact, they solve the Transition node ID mutability.

I am just suggesting a small change that is actually a code cleanup. Please see bellow.

[claudiosdc]

Suggested change

	impl CommitEncodeWithStrategy for Assignments {
	type Strategy = commit_strategy::UsingStrict;
	type Strategy = commit_strategy::UsingConceal;
	}

These lines should be deleted since they are not in effect. The implementation of the CommitEncode trait for the Assignments type is not needed. The new Assignments::consensus_commits() associated function is being used to do the required processing instead.

[dr-orlovsky]

There is an important difference between commitment encoding and consensus commitment. The first produces a vector of serialized data: the actual message we are comitting to. The second is produced from the first and is always a well-defined structure (like hash) that can be verified against the original message. You should not have consensus commit without commitment encoding.

[dr-orlovsky]

So:

1. We conceal the data (of they must or should be concealed according to the spec)
2. We commit-encode (serialize) the resulting concealed data as a byte string
3. We produce a commitment to those data (MerkleNode or other tagged hash form)
4. We merklize the tree of such MerkleNode commitments
5. We serialize the root of the Merkle tree
6. We take that serialized root as a consensus commit (since its already a hash and a commitment)

Thus we use concealment, encoding, commitment , merklization, encoding and shallow commitment for all our collections.

[dr-orlovsky]

Ok, you are right, I double-checked the code and we cant produce any single commitment from the assignments; it is created only on the level above when we have assignments of all types. I think Assignments name here is very confusing, since this structure covers only assignments of certain type.

[claudiosdc]

BTW, we have a new rgb command-line tool which helps creating consignments out of YAML definitions (and verse back) - this should be useful for a detailed test cases

That sounds good. Where can we get it from?

[dr-orlovsky]

@rajarshimaitra you are welcome to do any docs; I will also wirk on them. Some initial comments: #8 (comment)

[dr-orlovsky]

@claudiosdc its right in here: src/bin/rgb.rs. cargo build --all-features will produce binary jn ./target/debug/rgb. cargo install --all-features --path . will install it on the global path such you can simply use rgb command in a command-line

[claudiosdc]

Several test functions are failing.

test bech32::test::test_bech32_contract_id ... FAILED
test contract::assignments::test::test_ancestor_encoding_simple ... FAILED
test contract::assignments::test::test_ancestor_encoding_complex ... FAILED
test contract::metadata::test::test_commitencoding_field ... FAILED
test contract::assignments::test::test_commitencode_assignments ... FAILED
test contract::nodes::test::test_autoconceal_node ... FAILED
test contract::nodes::test::test_genesis_commit_ne_strict ... FAILED
test contract::nodes::test::test_genesis_impl ... FAILED
test contract::nodes::test::test_id_serde ... FAILED
test contract::nodes::test::test_node_attributes ... FAILED

They should be reviewed and adjusted as appropriately.

[dr-orlovsky]

@claudiosdc yes, that what we discussed with @rajarshimaitra above:

Many tests fail since the commitment algorithm has changed. Would like @rajarshimaitra to check the tests which he authored to make sure that the logic is correct – and then update test constants with new commitments.

[dr-orlovsky]

@claudiosdc removed CommitEncode for Assignments as you suggested and renamed AutoConceal trait with the last commit. Since @rajarshimaitra will need more time on completing test updates and this is really needed for a lot of downstream work plan to merge this PR today with LNP-BP/rust-lnpbp#179 if you have no other objections.

[claudiosdc]

Since @rajarshimaitra will need more time on completing test updates and this is really needed for a lot of downstream work plan to merge this PR today with LNP-BP/rust-lnpbp#179 if you have no other objections.

Everything looks good for me now (except for the test functions). So, yes, I am good with the merge.