IntersectMBO / cardano-ledger

The ledger implementation and specifications of the Cardano blockchain.
Apache License 2.0
256 stars 158 forks source link

Fail on duplicate certificates #4566

Open mkoura opened 6 months ago

mkoura commented 6 months ago

Internal/External Internal

Area Other Any other topic (Delegation, Ranking, ...).

Summary In Babbage transaction, the order of certificates is retained, even when the certificates are repeated. I can eg. build a tx that contain following certs in this order

  1. stake address registration cert
  2. deregistration cert for the stake address above
  3. stake address registration cert from 1.
  4. deregistration cert from 2.
  5. stake address registration cert from 1.

The transaction can be submitted and the stake address is registered.

Certs in Babbage tx:

    "certificates": [
        {
            "stake address registration": {
                "keyHash": "e24c2f4946e87eae96ace9c362b3c6a0d40a0c3a04592a591824180a"
            }
        },
        {
            "stake address deregistration": {
                "keyHash": "e24c2f4946e87eae96ace9c362b3c6a0d40a0c3a04592a591824180a"
            }
        },
        {
            "stake address registration": {
                "keyHash": "e24c2f4946e87eae96ace9c362b3c6a0d40a0c3a04592a591824180a"
            }
        },
        {
            "stake address deregistration": {
                "keyHash": "e24c2f4946e87eae96ace9c362b3c6a0d40a0c3a04592a591824180a"
            }
        },
        {
            "stake address registration": {
                "keyHash": "e24c2f4946e87eae96ace9c362b3c6a0d40a0c3a04592a591824180a"
            }
        }
    ],

However in Conway tx, the repeated certs are stripped. Therefore when submitting the tx with certificates order listed above, the stake address is not registered.

Certs in Conway tx:

    "certificates": [
        {
            "Stake address registration": {
                "stake credential": {
                    "keyHash": "4f2792103fe76cb96a0c5fb89dcd3f36441a2f9c697dc783ed915039"
                }
            }
        },
        {
            "Stake address deregistration": {
                "stake credential": {
                    "keyHash": "4f2792103fe76cb96a0c5fb89dcd3f36441a2f9c697dc783ed915039"
                }
            }
        }
    ],

Steps to reproduce Steps to reproduce the behavior:

  1. build a tx

cardano-cli conway transaction build --tx-in "57e67b2775839788da91936645f98caac34a157a2f01dfc3e5334659160758cd#0" --certificate-file "test_addr_registration_certificate_order[build-submit_cli]_ci0_ykz_addr0_stake_reg.cert" --certificate-file "test_addr_registration_certificate_order[build-submit_cli]_ci0_ykz_addr0_stake_dereg.cert" --certificate-file "test_addr_registration_certificate_order[build-submit_cli]_ci0_ykz_addr0_stake_reg.cert" --certificate-file "test_addr_registration_certificate_order[build-submit_cli]_ci0_ykz_addr0_stake_dereg.cert" --certificate-file "test_addr_registration_certificate_order[build-submit_cli]_ci0_ykz_addr0_stake_reg.cert" --change-address addr_test1qz8el0prsndp9y00tvv4aykmjhrteg450e3f72v6n8rxl6ymn7p2shwt24atpgw7fzshqdjjl7c9cfr3skrkjq5n098sf5dk3h --witness-override 2 --out-file "test_addr_registration_certificate_order[build-submit_cli]_ci0_ykz_tx.body" --testnet-magic 42

  1. sign and submit the tx, see that the stake address was not registered
  2. View the tx, see that repeated certificates were stripped

Expected behavior A clear and concise description of what you expected to happen.

System info (please complete the following information):

Screenshots and attachments Conway and Babbage transactions: tx_certificates_issue.tar.gz

mkoura commented 6 months ago

This is a dup of https://github.com/IntersectMBO/cardano-cli/issues/645

carbolymer commented 6 months ago

The representation of certificates in ledger has changed between Babbage and Conway. In Babbage it's a StrictSeq allowing for duplicate certificates to be stored in the transaction body. In Conway it is OSet which stores only unique certificates. This needs to be fixed in ledger.

lehins commented 6 months ago

This needs to be fixed in ledger.

The change to OSet is the fix in ledger, because duplicate certificates in the same transaction are not supported.

Hopefully we'll have enough time in ledger to change for Conway the functionality of "being stripped" to "deserialization failure". This is conceptually a pretty hard task due to annotator, but we are aware of it and have some plans on how to tackle it

lehins commented 1 month ago

Proper fix for this is postponed till the next era, since that is fundamentally a much harder task to fail deserialization on duplicates with our current approach. There is a concrete plan making this happen in the next era. However, if you look at the change in CDDL from Babbage:

https://github.com/IntersectMBO/cardano-ledger/blob/ac9d9d6086de6fc20d7f777e39dc2c674f8c5f66/eras/babbage/impl/cddl-files/babbage.cddl#L58

to the one in Conway:

https://github.com/IntersectMBO/cardano-ledger/blob/ac9d9d6086de6fc20d7f777e39dc2c674f8c5f66/eras/conway/impl/cddl-files/conway.cddl#L87

You will see that certificates where changed from an array to a set. This means that according to the binary specification duplciates are not allowed. This also means by dropping duplicates we are in adherence to the specification, because CDDL leaves it up to implementation how to deal with duplicates in sets and maps.

In other words this is not a bug. That does not mean we could not make the UX a bit better, and we will in the next era by failing instead of silently dropping elements. I'll keep this ticket opened, so we don't forget to do it, but I'll change the title to reflect the actual task.

Ticket for the actual cause of this problem: #4009