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

cardano-cli not in line with cip-0021 tag258 implementation - causing errors with hw wallets #567

Closed
gitmachtl opened this issue Jan 13, 2024 · 6 comments

Comments

@gitmachtl
Copy link
Contributor

gitmachtl commented Jan 13, 2024

$ cardano-cli version
cardano-cli 8.17.0.0 - linux-x86_64 - ghc-8.10
git rev a4a8119b59b1fbb9a69c79e1e6900e91292161e7

While i am testing HW-Wallet stuff for conway-era, i came across a wrong implementation of the new Tag(258) for conway transactions.

The decision was made - also in combination with HW-Wallets - to have the new Tag(258) everywhere or nowhere in the Tx. When generating a transaction for a dRep registration the output from cardano-cli looks like this example:

{
    "type": "Unwitnessed Tx ConwayEra",
    "description": "Ledger Cddl Format",
    "cborHex": "84a50081825820975f13c5c33baa7cd4c02b22255c76be2c83356764df8b61955f6929c28652760001818258390074976c54afaf444f7cd499bd8519aac6592b13b22b9d5817f0da5c5203d205532089ad2f7816892e2ef42849b7b52788e41b3fd43a6e01cf1a00415db6021a0002aa69031a011995e004d901028184108200581cfd1e9fb13ef9a4bdd0c7e47aef0bfc065eca6c4ac9b613861ccf20cb1a001e848082782168747470733a2f2f6d792d69702e61742f746573742f6877647265702e6a736f6e5820fd1b4e7fd844a8e2805bfda173f6461fd59812fcad4ce8a69b5ab427f7226cf7a0f5f6"
}
[
    {
        0: [
            [
                h'975f13c5c33baa7cd4c02b22255c76be2c83356764df8b61955f6929c2865276',
                0,
            ],
        ],
        1: [
            [
                h'0074976c54afaf444f7cd499bd8519aac6592b13b22b9d5817f0da5c5203d205532089ad2f7816892e2ef42849b7b52788e41b3fd43a6e01cf',
                4283830_2,
            ],
        ],
        2: 174697_2,
        3: 18453984_2,
        4: 258_1([
            [
                16,
                [
                    0,
                    h'fd1e9fb13ef9a4bdd0c7e47aef0bfc065eca6c4ac9b613861ccf20cb',
                ],
                2000000_2,
                [
                    "https://my-ip.at/test/hwdrep.json",
                    h'fd1b4e7fd844a8e2805bfda173f6461fd59812fcad4ce8a69b5ab427f7226cf7',
                ],
            ],
        ]),
    },
    {},
    true,
    null,
]

The Tag(258) was only added to the certificate part of the transaction.

Now after hw-cli takes over to use it for hw-witnessing, the corrected Tx looks like this:

{
    "type": "Unwitnessed Tx ConwayEra",
    "description": "Ledger Cddl Format",
    "cborHex": "84a500d9010281825820975f13c5c33baa7cd4c02b22255c76be2c83356764df8b61955f6929c28652760001818258390074976c54afaf444f7cd499bd8519aac6592b13b22b9d5817f0da5c5203d205532089ad2f7816892e2ef42849b7b52788e41b3fd43a6e01cf1a00415db6021a0002aa69031a011995e004d901028184108200581cfd1e9fb13ef9a4bdd0c7e47aef0bfc065eca6c4ac9b613861ccf20cb1a001e848082782168747470733a2f2f6d792d69702e61742f746573742f6877647265702e6a736f6e5820fd1b4e7fd844a8e2805bfda173f6461fd59812fcad4ce8a69b5ab427f7226cf7a0f5f6"
}
[
    {
        0: 258_1([
            [
                h'975f13c5c33baa7cd4c02b22255c76be2c83356764df8b61955f6929c2865276',
                0,
            ],
        ]),
        1: [
            [
                h'0074976c54afaf444f7cd499bd8519aac6592b13b22b9d5817f0da5c5203d205532089ad2f7816892e2ef42849b7b52788e41b3fd43a6e01cf',
                4283830_2,
            ],
        ],
        2: 174697_2,
        3: 18453984_2,
        4: 258_1([
            [
                16,
                [
                    0,
                    h'fd1e9fb13ef9a4bdd0c7e47aef0bfc065eca6c4ac9b613861ccf20cb',
                ],
                2000000_2,
                [
                    "https://my-ip.at/test/hwdrep.json",
                    h'fd1b4e7fd844a8e2805bfda173f6461fd59812fcad4ce8a69b5ab427f7226cf7',
                ],
            ],
        ]),
    },
    {},
    true,
    null,
]

So it added also Tag(258) for the first section (inputs) to be in line with CIP-0021 and the CDDL.

The witnesses are added to the Tx correctly with cardano-cli conway transaction assemble command, but caused an error at the end, because the Tx is now 3 bytes (the Tag258 itself) larger than the predicted Tx size for the fee calculation.

So cardano-cli should respect the Tag(258) implementation according to CIP-0021/CDDL and use the Tags everywhere or nowhere so we have the correct Tx right from the beginning.

CIP-0021: cardano-foundation/CIPs#687

@janmazak
Copy link

A note on reasons for all/nothing approach to tags in HW wallets: Optionality in transaction serialization is costly for HW wallets because it adds to code size and leads to breaking changes in various low-level APIs. It also does not seem to serve any useful purpose to have some of the tags, but not others in a single tx --- it is said the tags will become mandatory in some future era.

It would be best if cardano-cli allowed the user (via some command-line argument) to have control over the tags, at least an option to not include them at all, and to include all of the possible ones. (I don't see any other way to have it interoperable with who knows how many dapps and other tools that have a different release cycle.)

@carbolymer
Copy link
Contributor

This looks like an issue related with ledger format. @lehins can you pass it to your team please?

@gitmachtl
Copy link
Contributor Author

@lehins any updates from your side?

@lehins
Copy link
Contributor

lehins commented Jan 23, 2024

@gitmachtl thank you for reporting this issue, and for pinging me again about it. This is fixed in: IntersectMBO/cardano-ledger#4003
I am pretty sure this change will get into the next 8.8 release

Whenever there is an optional new format in cbor cardano-cli/cardano-api and cardano-ledger should be using the new format by default for encoding. This is necessary to promote deprecation of the old format in the future. In this case the change is tag 258.

Optionality in transaction serialization is costly for HW wallets because it adds to code size and leads to breaking changes in various low-level APIs

@janmazak, same can be said about all changes to serialization. It is costly for ledger too. However, we need to ensure backwards compatibility at least for an era or two. Because if we don't do this, the backwards incompatible changes (like addition of a 258 tag or change to a TxOut format like in Babbage, etc) will prevent older transactions from being deserializable in the new era.

It would be best if cardano-cli allowed the user (via some command-line argument) to have control over the tags, at least an option to not include them at all, and to include all of the possible ones.

This is technically infeasible.

I don't see any other way to have it interoperable with who knows how many dapps and other tools that have a different release cycle.

I don't see how having a cli flag would solve this for dapps.

The bottom line is there are two alternative formats that ledger supports: previous and the new. This is needed for all of the tooling that builds transactions to be able to transition to the new format. All of the software that requires to be able to decode transactions, like cardano-ledger and HWs, they must support the full CDDL specification.

Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 120 days.

@github-actions github-actions bot added the Stale label Feb 23, 2024
Copy link

This issue was closed because it has been stalled for 120 days with no activity. Remove stale label or comment or this will be closed in 60 days.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants