Fix signing of a transaction in `compatible shelley transaction signed-transaction` command by not using an incomplete body for signing #1057

carbolymer · 2025-02-06T16:59:29Z

Changelog

- description: |
    Update [cardano-api-10.10.0.0](https://github.com/IntersectMBO/cardano-api/blob/master/cardano-api/CHANGELOG.md#101000)
    Fix signing of a transaction in `compatible shelley transaction signed-transaction` command. Previously two different transaction bodies were used for the resulting transaction and the signature - now it's used the same for both purposes.
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
   - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

This PR contains two commits:

the actual fix
the split of Cardano.CLI.Compatible.Transaction into separate modules aligning with cardano-cli design.

Fixes:

Fail to submit update proposal in Tx that was build with compatible shelley transaction signed-transaction #1054

Requires:

Split compatible transaction building and signing cardano-api#750

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

Jimbo4350 · 2025-02-18T12:52:05Z

cardano-cli/cardano-cli.cabal

@@ -325,7 +325,7 @@ test-suite cardano-cli-test
    base16-bytestring,
    bech32 >=1.1.0,
    bytestring,
-    cardano-api:{cardano-api, gen, internal},


Jimbo4350

LGTM but I have a few questions

Jimbo4350 · 2025-02-18T13:04:46Z

cardano-cli/src/Cardano/CLI/Read.hs

+  :: ()
+  => ShelleyBasedEra era
+  -> Maybe NetworkId
+  -> A.TxBody era


🤦‍♂️ It confused me at first but I'm just noticing this type. What do we gain by wrapping the ledger type here? Why not directly use the ledger type?

That we don't have to use ShelleyLedgerEra type family and we can hide some calls to shelleyBasedEraConstraints behind our lens. A little convenience for some boilerplate.

If we decide to not use this TxBody type, we should deprecate the module Cardano.Api.Ledger.Lens then to avoid confusion.

Jimbo4350 · 2025-02-18T13:06:28Z

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs

@@ -279,69 +277,29 @@ runCompatibleTransactionCmd
        )
        sbe

-    let certsRefInputs =


I assume we don't need reference inputs because QA does not use them?

We do - but removed code was only used to build a body only for creating signatures. But it's problematic: it's a slightly different body than the one we're using in building compatible transaction. So the transaction building logic is duplicated in two places, and we had the bug with incorrect body hash used for the signatures.

Jimbo4350 · 2025-02-18T13:06:46Z

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs

    let txCerts = mkTxCertificates sbe certsAndMaybeScriptWits

-    -- this body is only for witnesses
-    apiTxBody <-


Jimbo4350 · 2025-02-18T13:07:37Z

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs

+      firstExceptT CompatiblePParamsConversionError . hoistEither $
+        createCompatibleTx sbe ins allOuts fee protocolUpdates votes txCerts
+
+    let txBody = ledgerTx ^. A.txToTxBodyL sbe


We need to directly use the ledger tx body type instead.

https://github.com/IntersectMBO/cardano-cli/pull/1057/files#r1959731654

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs

Jimbo4350

Fix signing how? Again, break this up in to sensible commits to make it easy for the reviewer to understand what has changed as there are lots of style related changes.

This avoids of implementation drift between cardano-api and cardano-cli. We were building two transaction bodies - one for just signatures and one is the resulting transaction. Thos bodies over time became different and resulted in a bug producing invalid transaction signatures. Now it's only one body used for both purposes.

carbolymer self-assigned this Feb 6, 2025

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch 6 times, most recently from 1897e91 to efd2bed Compare February 11, 2025 20:13

carbolymer changed the title ~~Add test: compatible shelley transaction signed-transaction results in different witnesses than legacy command~~ Fix signing of a transaction in compatible shelley transaction signed-transaction command. Feb 12, 2025

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch from 5de3eba to 754d7db Compare February 12, 2025 14:09

carbolymer marked this pull request as ready for review February 12, 2025 14:11

carbolymer requested review from Jimbo4350, newhoggy, smelc, palas, kevinhammond and disassembler as code owners February 12, 2025 14:11

carbolymer marked this pull request as draft February 13, 2025 17:08

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch 3 times, most recently from b92af75 to 64e7f9e Compare February 17, 2025 20:50

carbolymer marked this pull request as ready for review February 17, 2025 20:52

Jimbo4350 reviewed Feb 18, 2025

View reviewed changes

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch from 64e7f9e to fa52c7d Compare February 20, 2025 15:38

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs Fixed Show fixed Hide fixed

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch 2 times, most recently from 64e7f9e to b27c0c0 Compare February 20, 2025 15:50

github-advanced-security bot found potential problems Feb 20, 2025

View reviewed changes

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs Fixed Show fixed Hide fixed

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch 2 times, most recently from 24a3f28 to c62048c Compare February 20, 2025 16:25

Jimbo4350 requested changes Feb 26, 2025

View reviewed changes

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch 3 times, most recently from 99bc559 to d4c8068 Compare February 27, 2025 11:53

carbolymer requested a review from Jimbo4350 February 27, 2025 11:54

carbolymer changed the title ~~Fix signing of a transaction in compatible shelley transaction signed-transaction command.~~ Fix signing of a transaction in compatible shelley transaction signed-transaction command by not using an incomplete body for signing Feb 27, 2025

Jimbo4350 approved these changes Feb 27, 2025

View reviewed changes

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch from d4c8068 to d12f2e1 Compare February 28, 2025 13:21

carbolymer added 3 commits February 28, 2025 14:35

Move mkShelleyBootstrapWitnesses to Cardano.CLI.Read to make it reusable

6337e21

Add reference test for compatible transaction building

95f0ea5

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch 2 times, most recently from bddf201 to e3bdb30 Compare February 28, 2025 13:37

carbolymer requested review from a team as code owners February 28, 2025 13:37

carbolymer enabled auto-merge February 28, 2025 13:37

Update cardano-api-10.10.0.0

5e20af4

carbolymer force-pushed the mgalazyn/test/compatible-tx-sign-different-witnesses branch from e3bdb30 to 5e20af4 Compare February 28, 2025 13:40

carbolymer added this pull request to the merge queue Feb 28, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 28, 2025

carbolymer added this pull request to the merge queue Feb 28, 2025

Merged via the queue into master with commit c1b0f43 Feb 28, 2025
25 of 26 checks passed

carbolymer deleted the mgalazyn/test/compatible-tx-sign-different-witnesses branch February 28, 2025 15:06

carbolymer mentioned this pull request Mar 4, 2025

Fail to submit update proposal in Tx that was build with compatible shelley transaction signed-transaction #1054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix signing of a transaction in `compatible shelley transaction signed-transaction` command by not using an incomplete body for signing #1057

Fix signing of a transaction in `compatible shelley transaction signed-transaction` command by not using an incomplete body for signing #1057

carbolymer commented Feb 6, 2025 •

edited

Loading

Jimbo4350 Feb 18, 2025

Jimbo4350 left a comment

Jimbo4350 Feb 18, 2025 •

edited

Loading

carbolymer Feb 18, 2025 •

edited

Loading

Jimbo4350 Feb 18, 2025

carbolymer Feb 18, 2025 •

edited

Loading

Jimbo4350 Feb 18, 2025

Jimbo4350 Feb 18, 2025

carbolymer Feb 18, 2025

Jimbo4350 left a comment •

edited

Loading

Fix signing of a transaction in compatible shelley transaction signed-transaction command by not using an incomplete body for signing #1057

Fix signing of a transaction in compatible shelley transaction signed-transaction command by not using an incomplete body for signing #1057

Conversation

carbolymer commented Feb 6, 2025 • edited Loading

Changelog

Context

Checklist

Jimbo4350 Feb 18, 2025

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

Jimbo4350 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

carbolymer Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Feb 18, 2025

Choose a reason for hiding this comment

carbolymer Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Feb 18, 2025

Choose a reason for hiding this comment

Jimbo4350 Feb 18, 2025

Choose a reason for hiding this comment

carbolymer Feb 18, 2025

Choose a reason for hiding this comment

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

Fix signing of a transaction in `compatible shelley transaction signed-transaction` command by not using an incomplete body for signing #1057

Fix signing of a transaction in `compatible shelley transaction signed-transaction` command by not using an incomplete body for signing #1057

carbolymer commented Feb 6, 2025 •

edited

Loading

Jimbo4350 Feb 18, 2025 •

edited

Loading

carbolymer Feb 18, 2025 •

edited

Loading

carbolymer Feb 18, 2025 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading