Emurgo / cardano-serialization-lib

This is a library, written in Rust, for serialization & deserialization of data structures used in Cardano's Haskell implementation of Alonzo along with useful utility functions.
Other
230 stars 124 forks source link

plutus script witness deserialization issue #665

Closed Scitz0 closed 5 months ago

Scitz0 commented 5 months ago

When a CLI generated cbor tx including witness set is parsed by CSL its converted into a single plutus script set where each object in the set is tagged with the version type. When later serialized back into cbor the result doesn't necessarily match original witness set cbor. The way this is handled currently causes an issue for us.

The problem

  1. User build a tx on CLI with a witness set with the following cbor (pretty-printed).
    {
        5: [[1, 0, 121_0([]), [36144_1, 12270270_2]]],
        6: [
            h'58f40100003323232323232323223222533300632323232533300a3370e900018059baa001132323300100100222533301000114a026464a66601e64646600200200444a66602a00229404c8c94ccc050cdc78010088a511330040040013019002375c602e00260106eacc01cc038008528899802002000980a00118090009bac300f301030103009300f300900516300130080042300e300f001300100122533300b00114bd70099806180498068009980100118070008a4c26cac6eb80048c010dd5000ab9a5573aaae7955cfaba05742ae893011e581c5004124d89094b99e7daefef098efa7abd68c9cd1040572b927912fc0001',
        ],
    }
  2. This is passed to our wallet for vkey signature.
  3. CSL is used to deserialize tx
  4. Vkey witness is added
  5. CSL tx object is serialized back into cbor for submission. New witness set below, please note the empty plutus v1 set.
    {
        0: [
            [
                h'bff63f02a88dd00d42e1ee8cb1bc31780802f8b427d967599c207d3a0fd21325',
                h'2fa62396872f12cfdb43bbb991745d7f6800cf8f17b0e87cbdc9fde59ed5ec1a2c06423d495c41647a255aab03e9b065dbf4e441fd6fda629506a35674a01909',
            ],
        ],
        3: [],
        6: [
            h'58f40100003323232323232323223222533300632323232533300a3370e900018059baa001132323300100100222533301000114a026464a66601e64646600200200444a66602a00229404c8c94ccc050cdc78010088a511330040040013019002375c602e00260106eacc01cc038008528899802002000980a00118090009bac300f301030103009300f300900516300130080042300e300f001300100122533300b00114bd70099806180498068009980100118070008a4c26cac6eb80048c010dd5000ab9a5573aaae7955cfaba05742ae893011e581c5004124d89094b99e7daefef098efa7abd68c9cd1040572b927912fc0001',
        ],
        5: [[1, 0, 121_0([]), [36144_1, 12270270_2]]],
    }
  6. Submit fail due to fee to low as fee was calculated without item 3 in witness set.

Solution Don't add empty array for any of the plutus script items.

Scitz0 commented 5 months ago

I just noticed in the conway CDDL that the type is now specified as: nonempty_set<plutus_v1_script> instead of [* plutus_v1_script ].

Is this catered for in alpha code of CSL 12 so it's not an issue anymore after we deploy this version?

lisicky commented 5 months ago

Hi @Scitz0 ! In the next CSL 12 alpha it is going to be nonempty_set<plutus_v1_script>, but in the current non alpha CSL PlutusV1 witness can be as empty set. But if it block you we can consider temporal fix for CSL 11 in FixedTransaction type, if not it's better to wait next CSL release. But I have warning about mixing different tools, we don't have guarantee that CBOR from tool X will be the same as CBOR from tool Y after serialization round-trip. In the CSL there is FixedTransaction type to avoid this problem when you add signatures to witness set, but seems we didn't cover the case from this issue. Anyway if it block you let us know and we will release new CSL 11.5.1 with fix otherwise you can wait next CSL 12 alpha

Scitz0 commented 5 months ago

If ok in upcoming CSL 12 I would say let it be, that's close enough on the horizon for us.

As a wallet builder using this beloved CSL lib but not having control over transactions passed to us from dapps to sign I'm afraid we can't control who builds the tx. Due to this we have had to make several workarounds in the past as well to make sure it's all compatible, arrays vs maps and much more.

I understand from your point as view as a lib that you can't always cater for this.

vsubhuman commented 5 months ago

Due to this we have had to make several workarounds in the past as well to make sure it's all compatible, arrays vs maps and much more.

I understand from your point as view as a lib that you can't always cater for this.

@Scitz0, if you create an issue any time this happens ever again, we will try to make sure it gets prioritised

Scitz0 commented 5 months ago

Sounds great, will make sure to do so. I will now close this issue as resolved by CSL 12.

lisicky commented 5 months ago

Hi @Scitz0 CSL 12.0.0 alpha.16 on npm and crates.io. You can check

Scitz0 commented 5 months ago

Hi @Scitz0 CSL 12.0.0 alpha.16 on npm and crates.io. You can check

Thanks, 8.8.0 related or just bug fixes?

lisicky commented 5 months ago

8.8.0 related