IntersectMBO / IntersectMBO/cardano-api

Transaction builders accept duplicate TxIns with differing witnesses, producing transactions rejected with ExtraRedeemers

Open
#1,324 0 comments 0 reactions 1 assignee Claimed by @carbolymer View on GitHub
bug
Dominant language
Haskell
Stars
40
Forks
30
Avg merge
2d 5h
Merged PRs (30d)
30

Description

## Problem

All three transaction builders accept an input list that repeats the same `TxIn` with two different witnesses, and silently build a transaction the ledger can never accept:

- `createCompatibleTx` (`Cardano.Api.Compatible.Tx`, `witnessableTxIns`)
- the experimental builder (`Cardano.Api.Experimental.Tx.Internal.BodyContent.New`, `extractWitnessableTxIns`)
- the deprecated legacy builder (`Cardano.Api.Tx.Internal.Body`, `extractWitnessableTxIns`)

All three `nub` the whole `(TxIn, witness)` pair, so two entries that share a `TxIn` but differ in witness both survive.
The redeemer pointer machinery then assigns consecutive indices by list position, while the transaction body stores inputs as a `Set`, which collapses the duplicate to a single slot.
Every spending redeemer after the duplicate points one slot too far.

## Impact

This is reachable from cardano-cli today.
`--tx-in TXID#0 --tx-in-script-file a.plutus ... --tx-in TXID#0 --tx-in-script-file b.plutus` passes the parser, and nothing between optparse and the builders validates uniqueness.
Collateral inputs, by contrast, are already deduplicated with `nubOrd` in the same code path.

The observable outcome depends on the command:

- `transaction build` and `build-estimate` fail client side, during execution-unit evaluation, with `ScriptErrorRedeemerPointsToUnknownScriptHash` wrapped in a `TxBodyScriptExecutionError`.
- `transaction build-raw` and the compatible builder serialise the transaction without complaint; it is then rejected at submission with `ExtraRedeemers` (UTXOW, phase 1).
- One edge case validates: when the duplicated `TxIn` carries one script witness and one key placeholder and sorts last among the distinct inputs, the cardinalities line up, the transaction is accepted, and the other supplied witness is silently ignored. The index-set equality check guarantees the surviving redeemer is the correct one, so a redeemer can never execute against the wrong script.

This is a usability bug: confusing failures, or a silently ignored witness. Funds are not at risk.

## Proposed fix

Two options, in order of preference:

1. Make duplicates unrepresentable: key the inputs by `TxIn` in an insertion-ordered map (`OMap`), the way `TxCertificates` and `TxProposalProcedures` already work.
Spending inputs are the only witnessable category still passed around as a plain association list.
Rejecting a duplicate with a different witness then happens at map construction, which is how `mkTxVotingProcedures` already treats a duplicate voter ("This would cause ignoring some of the votes").
2. Keep the list and validate before constructing the body: add an error constructor per builder error type (for example `CompatibleTxDuplicateTxIn TxIn` in `CompatibleTxError`), plus analogous constructors for the experimental and legacy builders.

Either way:

- consider deduplicating or validating at the cardano-cli layer too, mirroring the existing `nubOrd` collateral handling
- extend the redeemer pointer property suite (`Test.Cardano.Api.Transaction.Body.Plutus.RedeemerIndex`) with duplicate-input cases
- pin `compareWitnesses` with a property while in there: the #1288 review flagged that the `WitTxCert` comparator returns `LT` for every pair, which degenerates to insertion order under a stable sort. Insertion order happens to be what certificates need, but that should be a tested invariant rather than a coincidence.

## Related PRs and issues

Positional indexing has broken once per witnessable category; this issue covers the last one:

- #1288 fixed proposal pointers (indexed by `Ord` instead of `OSet` insertion order) and unwitnessed certificate slots in the legacy builder, and added the ledger-oracle property suite
- #1140 (fixes #1138) unified certificate indexing after unwitnessed certificates were mishandled during deduplication
- #1136 fixed `mapScriptWitnessesCertificates` silently dropping key-witnessed certificates
- #798: transaction bodies with more than one script witness per purpose failed to validate (cardano-api 10.12)
- #697 centralised the indexing logic that all three builders now share
- IntersectMBO/cardano-cli#297: `MissingRedeemers` on Plutus stake delegation, a user-facing symptom from the same pointer machinery
- IntersectMBO/cardano-cli#861 added redeemer-to-input mapping to `transaction view`, because pointers are opaque to users

## Context

Split out of #1282 so the fix can cover the experimental, legacy and compatible APIs together.
Review comment: https://github.com/IntersectMBO/cardano-api/pull/1282#discussion_r3780453761

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.