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

[BUG] - Possible corruption of datum by CLI #4433

Closed
Tracked by #4144 ...
ArturWieczorek opened this issue Sep 13, 2022 · 6 comments · Fixed by #4886
Closed
Tracked by #4144 ...

[BUG] - Possible corruption of datum by CLI #4433

ArturWieczorek opened this issue Sep 13, 2022 · 6 comments · Fixed by #4886
Assignees
Labels
bug Something isn't working comp: cardano-cli era: babbage type: bug Something is not working user type: internal Created by an IOG employee

Comments

@ArturWieczorek
Copy link
Contributor

Internal/External
Internal
Area
cardano-cli - Tx with datum.

Summary
Originally this bug was opened in cardano-db-sync repository:
IntersectMBO/cardano-db-sync#1214

The observed errors could be caused by a CLI issue. It looks like Tx is created with the wrong Datum and db-sync does the correct thing of extracting the bytes as they are.

The test is defined here:
https://github.com/input-output-hk/cardano-node-tests/blob/master/cardano_node_tests/tests/test_plutus_v2_spend_raw.py#L375

it fails with:

>       assert (
            db_cbor_hex == orig_cbor_hex
        ), "Datum bytes in db-sync doesn't correspond to the original datum"
E       AssertionError: Datum bytes in db-sync doesn't correspond to the original datum
E       assert 'd8799f41ffd8799fd8799fd8799fd8799f581cc279a3fb3b4e62bbc78e288783b58045d4ae82a18867d8352d02775affd8799fd8799fd8799f581c121fd22e0b57ac206fefc763f8bfa0771919f5218b40691eea4514d0ffffffffd87a80ffd87a80ff1a002625a0d8799fd879801a000f4240d8799f1a000fa92effffff' == 'd8798441ffd87982d87982d87982d87981581cc279a3fb3b4e62bbc78e288783b58045d4ae82a18867d8352d02775ad87981d87981d87981581c121fd22e0b57ac206fefc763f8bfa0771919f5218b40691eea4514d0d87a80d87a801a002625a0d87983d879801a000f4240d879811a000fa92e'

We use https://github.com/input-output-hk/cardano-node-tests/blob/8baf63e9e4c66c02c903f9e[…]e8/cardano_node_tests/tests/data/plutus/typed-finite.datum.cbor which is the datum used in the original reports 1 and 2.

We use this to create the Tx:

cardano-cli transaction build-raw --fee 219173 --out-file test_datum_bytes_in_dbsync_ci0_lnh_step1_tx.body --tx-in "d41022da83eef84dd9b9616f4ab5060b5471546aa94139132a885ac257715ec9#0" --tx-out "addr_test1wqag3rt979nep9g2wtdwu8mr4gz6m4kjdpp5zp705km8wys6t2kla+2400250" --tx-out-inline-datum-cbor-file cardano_node_tests/tests/data/plutus/typed-finite.datum.cbor --tx-out "addr_test1vq2ymsu4ahfnnvwe9nz748rcra5r3ykxse7xlgsqkyzc3wg5njxz7+2000000" --tx-out "addr_test1vzpz9xpkwqra8jtcs2ucy3lgzcwyc8wv8nn0q8fk0eujdpqkqj3ac+2995380577" --babbage-era

We then check that the original datum and datum in db-sync produce the same hash:

cardano-cli transaction hash-script-data --script-data-cbor-file cardano_node_tests/tests/data/plutus/typed-finite.datum.cbor
cardano-cli transaction hash-script-data --script-data-cbor-file test_datum_bytes_in_dbsync_ci0_lnh_db_datum.cbor

which it does, so there is clearly some pre-processing done in the CLI. Then we check if the bytes in db-sync match the original datum, which it does not.

Even though the bytes don't match, the hash do match, according to CLI:

$ cardano-cli transaction hash-script-data --script-data-cbor-file original.cbor
53af3173e58b420b9a2c638278668ee76c6d7d78ebf7b1aee09038fbd67762b5

$ cardano-cli transaction hash-script-data --script-data-cbor-file dbsync.cbor
53af3173e58b420b9a2c638278668ee76c6d7d78ebf7b1aee09038fbd67762b5

Files attached (added txt extension because GH blocked the upload)
original_cbor.txt
dbsync_cbor.txt

@kderme wrote unit tests for this that fail without the fix IntersectMBO/cardano-db-sync#1255 and create the tx programmaticaly, without cli.

System info (please complete the following information):

  • Node version 1.35.3
  • CLI version 1.35.3
@LudvikGalois
Copy link
Contributor

The initial version uses fixed length arrays, whilst the dbsync version uses variable length arrays with explicit "stop" markers. The data contained in both is identical.

I haven't looked to much further into why this happens, but my assumption is that the underlying CBOR library being used wants to make streaming data possible and therefore uses variable length arrays everywhere when serialising data to CBOR.

Personally, I don't think this is a "real" bug with the CLI. This is morally equivalent to you supplying {"foo": "bar"} and it storing {"foo":"bar"}.

@LudvikGalois
Copy link
Contributor

Personally, I don't think this is a "real" bug with the CLI. This is morally equivalent to you supplying {"foo": "bar"} and it storing {"foo":"bar"}.

Actually, perhaps it is a real issue. Let me see how much work is involved to fix it.

@kderme
Copy link
Contributor

kderme commented Sep 27, 2022

whilst the dbsync version uses variable length arrays with explicit "stop" markers

after the fix db-sync does no serialization, it uses the bytes as they come from the wire.

On the other end, cli should do no serialization as weill, when dealing with CBOR file inputs. It should get the bytes as they come from the file. Otherwise, even though the Datum is the same, it can cause hash mismatches from that the user expects.

@AndrewWestberg
Copy link

AndrewWestberg commented Feb 3, 2023

I came across this bug in building a transaction with cardano-cli. In order to sign a plutus transaction with a hardware device like ledger, the cbor MUST be canonical. cardano-hw-cli will decline to sign it if it is not.

A transaction built by cardano-cli will produce a variable length array no matter what.

{
        5: [
            [0, 2, 122_0([]), [3366240_2, 907580139_2]],
            [
                1,
                0,
                121_0([_
                    h'3ab25c853f0f188f43b34b2df5cb98737a4de7e19028ec9d1a414a55',
                    5,
                    h'4e45574d5f',
                ]),
                [5312215_2, 1428546648_2],
            ],
        ],
    }

The underscore represents a variable length array.

The json datum file that produced this is

{
  "constructor": 0,
  "fields": [
    {
      "bytes": "3ab25c853f0f188f43b34b2df5cb98737a4de7e19028ec9d1a414a55"
    },
    {
      "int": 5
    },
    {
      "bytes": "4e45574d5f"
    }
  ]
}

To get around this and have cardano-cli use a fixed-length array so I can sign the transaction with a ledger hardware device, I attempted to pass in the data as cbor with --tx-out-inline-datum-cbor-file data/next_datum.cbor

This cbor file is clearly using a fixed-length array as there is no underscore in the array.

$ cat data/next_datum.cbor| xxd -p -c 10000 
d87983581c3ab25c853f0f188f43b34b2df5cb98737a4de7e19028ec9d1a414a5505454e45574d5f

$ cbor-diag --to diag <<< d87983581c3ab25c853f0f188f43b34b2df5cb98737a4de7e19028ec9d1a414a5505454e45574d5f
121_0([
    h'3ab25c853f0f188f43b34b2df5cb98737a4de7e19028ec9d1a414a55',
    5,
    h'4e45574d5f',
])

Even in this circumstance, cardano-cli manipulated my input data and creates an indeterminate-length array in the final transaction.

@AndrewWestberg
Copy link

Here's the reference bug I opened on the VacuumLabs side. Basically, either IOG or VL needs to fix it. Either IOG needs to output canonical cbor that the VL code will choose to leave alone, or the VL code needs to be smarter and ignore any un-canonical cbor outside of the txBody.

Either way, this is frustrating as a user stuck between two companies. This isn't the first time we've had a disconnect between them over canonical cbor in transactions.

vacuumlabs/cardano-hw-cli#155

@Jimbo4350
Copy link
Contributor

Related: IntersectMBO/cardano-ledger#2943

iohk-bors bot added a commit that referenced this issue Feb 17, 2023
4831: Refactor `genesis create` command usage in  cardano testnet r=Jimbo4350 a=Jimbo4350

Implement the following three modules:
1. Testnet.Commands.Genesis - Houses the refactoring of cli commands concerning genesis generation

2. Testnet.Commands.Governance - Houses the refactoring of cli commands concerning governance 

3. Testnet.Options - Module that will house all the different `TestnetOptions` types. The aim is to only have one `TestnetOptions` type in the future.

4886: Preserve `ScriptData` bytes with `HashableScriptData` r=Jimbo4350 a=Jimbo4350

Resolves: #4433

Co-authored-by: Jordan Millar <[email protected]>
@github-project-automation github-project-automation bot moved this from 🌱 Backlog to 🪴 Curation in Cardano Node Product Backlog Feb 28, 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 comp: cardano-cli era: babbage type: bug Something is not working user type: internal Created by an IOG employee
Projects
None yet
6 participants