LNP-BP / client_side_validation

Standard implementation of client-side-validation APIs
https://docs.rs/client_side_validation
Apache License 2.0
22 stars 19 forks source link

CommitmentId produces different results in Wasm32 target #133

Closed crisdut closed 11 months ago

crisdut commented 1 year ago

Hi @dr-orlovsky,

I noticed that the CommitmentId::commitment_id function returns different values ​​according to the target compilation.

I discovered this while I was testing rgb-schemata, I even created a test that can help you with the bug investigation:

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn show_schema_id() {
        let expected = "5YWfKW3CqANHsKqpxy3HaCpt5bgvsMXHUuXiHpoynEYG";
        let schema = nia_schema();
        let id = schema.schema_id().to_string();
        assert_eq!(expected, id); // OK!
    }
}

#[cfg(target_arch = "wasm32")]
mod test_wasm32 {
    use super::*;
    use wasm_bindgen_test::*;
    wasm_bindgen_test_configure!(run_in_browser);

    #[wasm_bindgen_test]
    fn show_schema_id() {
        let expected = "5YWfKW3CqANHsKqpxy3HaCpt5bgvsMXHUuXiHpoynEYG";
        let schema = nia_schema();
        let id = schema.schema_id().to_string();
        assert_eq!(expected, id); // Fail!
       /*
          panicked at 'assertion failed: `(left == right)`
          left: `"5YWfKW3CqANHsKqpxy3HaCpt5bgvsMXHUuXiHpoynEYG"`,
          right: `"12vPR9qthiLpPgoytGBbeerVYFKvdCXEUmbg891HVyHV"`', src/main.rs:143:9
      */

    }
}

Hope this helps.

dr-orlovsky commented 1 year ago

Wow. Investigating!

dr-orlovsky commented 1 year ago

What I do not get: we have tests for checking ID against a predefined constant for nearly all major components in the system which use CommitmentId - and none of them are failing. Do we run tests under WASM at all? If yes, this means that the issue is specific to juist rgb-schemata and somehow to nia scheme...

crisdut commented 1 year ago

What I do not get: we have tests for checking ID against a predefined constant for nearly all major components in the system which use CommitmentId - and none of them are failing. Do we run tests under WASM at all? If yes, this means that the issue is specific to juist rgb-schemata and somehow to nia scheme...

So far, I've only tested SchemaID. By the way, I'm going to test other structures that use the commitment_id.

I using this branch: https://github.com/crisdut/rgbsc/tree/exp/schema-id

crisdut commented 1 year ago

Hi @dr-orlovsky , I have good updates about the problem:

The issue is caused by this line: https://github.com/BP-WG/bp-core/blob/master/primitives/src/lib.rs#L74

The cast between types have different behaviors according to target compilation.

Please, see the code bellow and the results:

mod test {
    #[test]
    fn check_cast_between_targets() {
        println!("U64: {} U64 (with cast): {}", u64::MAX, u64::MAX as usize);
        println!("U32: {} U32 (with cast): {}", u32::MAX, u32::MAX as usize);
    }
}
# Results in x86_64-unknown-linux-gnu:

U64: 18446744073709551615 U64 (with cast): 18446744073709551615
U32: 4294967295           U32 (with cast): 4294967295

# Results in wasm32-unknown-unknown:

U64: 18446744073709551615 U64 (with cast): 4294967295
U32: 4294967295           U32 (with cast): 4294967295

Rust Version: 1.70

Because of this, the bitcoin std library generate a different ID:

Results in x86_64-unknown-linux-gnu:

typelib Bitcoin -- extra_rapid_armada_EXDzvefCHMLgk4KXNvsXsDr2QzgPkxFFUCSEkL3S1ZJL

-- no dependencies

data LockTime         :: U32
(..)
data Sats             :: U64
data ScriptBytes      :: [Byte ^ ..0xffffffffffffffff]  <----- HERE!!!!!!
data ScriptPubkey     :: 7Bk3nVFjGbpESsetWgLCcJ4Qh7cr4cMwucYYn6f6QUUQ  <----- HERE!!!!!!
(...)
data Witness          :: [[Byte ^ ..0xffffffffffffffff] ^ ..0xffffffffffffffff]

Results in wasm32-unknown-unknown:

typelib Bitcoin -- race_ballet_golf_6GgF7biXPVNcus2FfQj2pQuRzau11rXApMQLfCZhojgi

-- no dependencies

data LockTime         :: U32
(...)
data Sats             :: U64
data ScriptBytes      :: [Byte ^ ..0xffffffff] <----- HERE!!!!!!
data ScriptPubkey     :: 3Y4AgjkFbDusgo3YqRDWv9BznDeAJEUDEPeEq1mpSkAR <----- HERE!!!!!!
(...)
data Witness          :: [[Byte ^ ..0xffffffff] ^ ..0xffffffff]

I'll test the other std libs for other inconsistent IDs and list them in this issue.

If I also find it in contracts, I suggest we migrate this issue to rgb-wallet. Otherwise, let's move to bp-core.

Hope this helps.

crisdut commented 1 year ago

Confirming, this is the only divergence between compilation targets. We need bump strict-types and strict-encoding in following repos:

Also, we can move this issue to bp-core.

BTW, I have a question: We need use U64 in this case or we can change to U32 ?

dr-orlovsky commented 1 year ago

Thank you for finding the reason of this! It is amazing how usize brings indeterminism wherever it touches...

I didn't get about strict types version bump, since it is already bumped in most of the repos or in the latest PRs there I will merge after the dev call today (once I get ACK from somebody in Bitfinex team).

Moving the issue between orgs is hard: you need to create a special repo, move issue there, than move repo between orgs, that move the issue again, than delete that repo. I think we can just close here, open new one in BP Core, and provide a link here.

Regarding u32 vs u64 - let's discuss it there, there is a number of tradeoffs with each one of them.

dr-orlovsky commented 1 year ago

It appears that given the properties of Rust compiler and the standard library, we can't fix usize issue at all. We can only have workarounds.

If I will make in amplify collections and arrays to take <const LEN: u64> instead of const LEN: usize>, it will not fix the problem but will move it just deeper: inside all calls to rust APIs I need to convert LEN into usize anyway, like

vec.get(pos /* which is u64 here */ as usize /* now becomes u32 in WASM32 */)
dr-orlovsky commented 1 year ago

In other words, with Rust we can't use u64-sized arrays/collections in a deterministic way (they will inevitable work differently on 64-bit and 32-bit platforms)

crisdut commented 1 year ago

I will close this issue, ok?

dr-orlovsky commented 1 year ago

Well, if the tests do not fail anymore. But I think we should keep it open until we are sure that everything works in other crates