Transition node ID mutability fix

[claudiosdc]

This is a proposed fix for issue #133 of the rgb-node repository.

This essentially replaces the implementation of the CommitEncode trait for the Transition structure. However, for this to be done, several adjustments needed to be made.

That same fix is also applied to the Extension structure, which is very similar to the Transition structure.

NOTE: the implementation of the CommitEncode trait for the Assignments structure (which is part of this pull request) can be simplified if an improvement is made in the client_side_validation crate. That improvement has already been proposed via pull request #178 of the LNP-BP/rust-lnpbp repository.

[rajarshimaitra]

Made the preliminary review. Tests passing and required behaviour is simulated.

Below are some approach comments.

[rajarshimaitra]

We already have Merkelization strategy defined here https://github.com/LNP-BP/rust-lnpbp/blob/8e5e1a715efba0704b38fe7d2a24ce71e7b2b23f/client_side_validation/src/client_side_validation.rs#L84

So instead of redefining the merle derivation again, we should be just able to use the same strategy.

[claudiosdc]

Yes, I am aware of that. However, for us to be able to use it we would need a change to be made to the client_side_validation crate. As a matter of fact, I have already proposed that change, via PR #178 of the LNP-BP/rust-lnpbp repository.

So, instead of waiting for the upstream upgrade, I have decided to "replicate" the implementation of the CommitEncode trait for vectors of a generic type, with the intention to replace it once the proposed client_side_validation crate change is accepted.

[rajarshimaitra]

I didn't understand why this was required.

[claudiosdc]

It was required because the way it was implemented it was not working. Let me explain.

The code:

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

ultimately will trigger the execution of this other code:

impl<T> CommitEncode for amplify::Holder<T, UsingConceal>
where
    T: Conceal,
    <T as Conceal>::Confidential: CommitEncode,
{
    fn commit_encode<E: io::Write>(self, e: E) -> usize {
        self.into_inner().conceal().commit_encode(e)
    }
}

If you pay attention, you will notice that this implementation has a constraint that the type returned by the conceal() method of the type in question (Void in our particular case here) needs to implement the CommitEncode trait.

However, if you look at the implementation of the conceal() method for the Void structure (shown below), you will notice that it returns the very own type Void, which leads to a circular condition.

https://github.com/rgb-org/rgb-core/blob/5984c301c583161643cbecd3ae20405399f43ab4/src/contract/data.rs#L44-L50

So, in the end, the CommitEncode trait is never implemented for the Void structure. Not the way it was originally implemented. The proposed change corrects that.

[dr-orlovsky]

If it was never implemented than rust compiler would produce a error – since we are calling this method from the code

[rajarshimaitra]

I feel like this extra step of conceal_seal() is redundant. The output of conceal_seal() will be either ConfidentialSeal or Confidential and applying another conceal() will always result in a Confidential state, so the below Reveled, ConfidentialSeal and ConfidentailAmount code branch will never hit.

Instead as per the discussion in the last call, we should modify the conceal() definition of OwnedState to conceal the correct parts. Which is not being done currently.

Then the commit_encode() definition can simply use the UsingConceal strategy, which should then give the correct results.

cc: @dr-orlovsky for the modification of the conceal() operation. My guess is it should always return Confidential state. This is explicitly being done now in this PR with the commit_encode() and conceal_seal() method, but it can be achieved implicitly with modification of conceal() method.

[claudiosdc]

I feel like this extra step of conceal_seal() is redundant. The output of conceal_seal() will be either ConfidentialSeal or Confidential and applying another conceal() will always result in a Confidential state, so the below Reveled, ConfidentialSeal and ConfidentailAmount code branch will never hit.

Instead as per the discussion in the last call, we should modify the conceal() definition of OwnedState to conceal the correct parts. Which is not being done currently.

Your analysis (as per the paragraphs above) is correct. The match statement should indeed have only one active branch for the OwnedState::Confidential case.

As for the use of a separate conceal_seal() method, I did it that way because I was not sure if the conceal() method could be changed (to conceal not only the value, as it does today, but also the seal) without breaking any other part of the code.

So, if you guys tell me that I could simply change the conceal() method instead, I will make the change.

[claudiosdc]

Then the commit_encode() definition can simply use the UsingConceal strategy, which should then give the correct results.

Unfortunately, the above statement is not correct. We need to explicitly implement the CommitEncode trait for the OwnedState<STATE> type for the same reason why we needed to do it for the Void type. Please see my comments above.

[dr-orlovsky]

Thank you for the careful work on the details! Left few comments.
I still believe this can be done with less code (I prefer to reduce consensus-critical code down to a minimum), with modifications to conceal methods, if required. I will try to play with your PR and see how it can be improved in this regard

[dr-orlovsky]

If it was never implemented than rust compiler would produce a error – since we are calling this method from the code

[dr-orlovsky]

By doing a manual encoding we are creating a chance for diverging strict and commit encodings in the future. The method should continue to use strict encoding, but on a copy of the data, after calling conceal_all

[claudiosdc]

If it was never implemented than rust compiler would produce a error – since we are calling this method from the code

In reference to the above comment, the compiler does not produce any error in that regard because, in fact, that code is not being called. This can be verified by simply commenting out those line of codes (shown below) and recompiling the library. You will notice that the library will compile with no error.

/*impl CommitEncodeWithStrategy for Void {
    type Strategy = commit_strategy::UsingConceal;
}*/

[dr-orlovsky]

Not sure why the manual implementation of the trait will change the fact that the code is not called from inside rgb-core. It is not called since in our tests we do not use void type of data AFAIR (and it is not used in RGB20 either).

[claudiosdc]

Not sure why the manual implementation of the trait will change the fact that the code is not called from inside rgb-core. It is not called since in our tests we do not use void type of data AFAIR (and it is not used in RGB20 either).

OK, fair enough. Then, as an alternative to prove my point, add the following test function to the (original) rgb-core/src/contract/data.rs file, and try to compile the library.

    #[test]
    fn test_void_encoding() {
        let v = Void;

        let mut e = vec![];
        let r = v.commit_encode(&mut e);

        assert_eq!(r, 0);
    }

You should then get the following error:

error[E0599]: no method named `commit_encode` found for struct `contract::data::Void` in the current scope
   --> src/contract/data.rs:749:19
    |
40  | pub struct Void;
    | ---------------- method `commit_encode` not found for this
...
749 |         let r = v.commit_encode(&mut e);
    |                   ^^^^^^^^^^^^^ method not found in `contract::data::Void`
    |
    = help: items from traits can only be used if the trait is implemented and in scope
    = note: the following trait defines an item `commit_encode`, perhaps you need to implement it:
            candidate #1: `lnpbp::client_side_validation::CommitEncode`

Then, you can go a step further and make the following changes to "force" that the CommitEncode trait be applied to type Void (via CommitEncodeWithStrategy).

+#[derive(Clone, Debug, PartialOrd, Ord, PartialEq, Eq, AsAny)]
+pub struct ConfidentialVoid {v: Void}
+
+impl CommitEncode for ConfidentialVoid {
+    fn commit_encode<E: io::Write>(self, _e: E) -> usize {0}
+}
+impl From<ConfidentialVoid> for Void {
+    fn from(c: ConfidentialVoid) -> Self {
+        c.v
+    }
+}
+
 impl Conceal for Void {
-    type Confidential = Void;
+    type Confidential = ConfidentialVoid;
 
     fn conceal(&self) -> Self::Confidential {
-        self.clone()
+        ConfidentialVoid {v: self.clone()}
     }

Now, the library can be compiled with no error, and the test function can then be called (see example below).

cargo test -p rgb-core test_void_encoding

Explaining the change above: when we replace the Confidential type of Void with something other than Void itself (the new ConfidentialVoid type in this case), we break (what I have referred to in one of my previous comments) the "circular condition" imposed by the code bellow.

    impl<T> CommitEncode for amplify::Holder<T, UsingConceal>
    where
        T: Conceal,
        <T as Conceal>::Confidential: CommitEncode,
    {
        fn commit_encode<E: io::Write>(self, e: E) -> usize {
            self.into_inner().conceal().commit_encode(e)
        }
    }

Where for the CommitEncode trait to be implemented for T (Void in our case), T must implement the Conceal trait (and that part is OK, since Void does implement it), and the Confidential type defined in the Conceal implementation for T (in other words, the type returned by the conceal() method of T) must implement the CommitEncode trait. Here is where the problem lies. If we substitute T for Void we can state the previous condition as: for the CommitEncode trait to be implemented for Void, Void must implement the CommitEncode trait, since Void::conceal() returns Void.

[claudiosdc]

@dr-orlovsky and @rajarshimaitra, I just wanted to add that the issue with the implementation of the CommitEncode trait for the Void type that we have discussed above also applies to other RGB node types, namely Assignments and OwnedState<STATE>. That is the reason why we do need an explicit implementation of the CommitEncode trait for those types.

[dr-orlovsky]

After certain considerations will concept NACK on the changes. I think we need to take another attempt on fixing this after considering all the matters from https://github.com/LNP-BP/LNPBPs/discussions/88 and generally implement (a) fix for commit-encoding with conceal-all on the assignment data first and (b) merklization for commit-encoding later as a separate & independent PRs without changing anything in the client_side_validation crate

[dr-orlovsky]

Closing in favor of merged #8 (it also includes cherry-picked commits from this PR)