Skip to content

Fix plutus script redeemer pointer indexing - #1288

Open
carbolymer wants to merge 1 commit into
masterfrom
mgalazyn/fix/experimental-redeemer-pointer-indexing
Open

Fix plutus script redeemer pointer indexing#1288
carbolymer wants to merge 1 commit into
masterfrom
mgalazyn/fix/experimental-redeemer-pointer-indexing

Conversation

@carbolymer

@carbolymer carbolymer commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Fix plutus script redeemer pointer indexing: proposal pointers now follow the transaction's insertion order and certificate pointers count unwitnessed certificates, in both the experimental and the deprecated transaction builders. Remove the unused StakeCredential field from the WitTxCert constructor. Add property tests checking every redeemer pointer against the ledger's own resolution.

Context

Redeemer pointers for certificates and proposals were computed against the wrong order. Proposals were sorted by their Ord instance before index assignment, but the ledger resolves Proposing purposes by the transaction's insertion order (OSet -> StrictSeq positional lookup). Unwitnessed certificates (e.g. plain stake registrations) were dropped before index assignment, but the ledger counts every certificate's position in the full sequence, so a plutus-witnessed certificate placed after an unwitnessed one received a shifted pointer and failed phase-2 validation. The certificate bug existed in both the experimental builder and the deprecated createTransactionBody path (reachable via makeTransactionBodyAutoBalance).

Also hardened getVotes against gaps in the voter map: it now uses a total lookup with a placeholder witness on a miss, rather than a lookup that silently skips a missing voter and shifts every later voter's redeemer index.

How this was fixed

  • Preserve insertion order in compareWitnesses for proposals.
  • Keep unwitnessed certificates as index-slot placeholders in both builders.
  • Remove the unused StakeCredential field from WitTxCert (breaking).
  • Correct the redeemer-index ordering documentation on indexTxProposalProcedures/indexWitnessedTxProposalProcedures.

How to trust this PR

Property tests assert every witnessable category's redeemer pointer against the ledger's own Indexable/redeemerPointer resolution: container-level oracles for all six categories, old-API bridge twins, and an end-to-end createTransactionBody property with per-certificate redeemer binding. Each fix was mutation-verified (reverting the fix makes the corresponding property fail).

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
  • Changelog fragment added in .changes/

@carbolymer carbolymer self-assigned this Aug 11, 2026
@carbolymer carbolymer changed the title Fix redeemer pointer indexing in the experimental witness machinery Fix plutus script redeemer pointer indexing Aug 11, 2026
@carbolymer
carbolymer force-pushed the mgalazyn/fix/experimental-redeemer-pointer-indexing branch from 604df2f to ab72b80 Compare August 11, 2026 15:40
@carbolymer
carbolymer marked this pull request as ready for review August 11, 2026 15:41
Copilot AI lite review requested due to automatic review settings August 11, 2026 15:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes Plutus redeemer pointer indexing to match ledger resolution across both the experimental transaction builder and the deprecated createTransactionBody path, and adds regression/property tests to prevent pointer-index drift (including a documented breaking change to WitTxCert).

Changes:

  • Align proposal redeemer indexing with transaction insertion order; ensure certificate indexing counts unwitnessed certificates as index slots.
  • Harden vote extraction against missing voter witnesses so indices cannot shift due to dropped entries.
  • Add new property tests that compare API-assigned redeemer pointers against the ledger’s Indexable/redeemerPointer behavior, and record the breaking change in a changelog fragment.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
cardano-api/test/cardano-api-test/Test/Cardano/Api/Transaction/Body/Plutus/RedeemerIndex.hs New property suite asserting redeemer pointer/index agreement with the ledger across all witnessable categories (incl. deprecated path).
cardano-api/test/cardano-api-test/Test/Cardano/Api/Experimental.hs Adds focused regression properties for proposal insertion-order indexing and certificate “count unwitnessed” indexing in makeUnsignedTx.
cardano-api/test/cardano-api-test/cardano-api-test.hs Registers the new redeemer-index test group.
cardano-api/src/Cardano/Api/Tx/Internal/Body.hs Old-API path: keep unwitnessed certificates as index-slot placeholders when extracting witnessables.
cardano-api/src/Cardano/Api/Tx.hs Adjusts export positioning/comment for extractWitnessableCertificates.
cardano-api/src/Cardano/Api/Experimental/Tx/Internal/BodyContent/New.hs Experimental path: include unwitnessed cert placeholders and make vote extraction total (no gaps).
cardano-api/src/Cardano/Api/Experimental/Plutus/Internal/IndexedPlutusScriptWitness.hs Updates ordering to preserve insertion order for certs/proposals and removes unused StakeCredential from WitTxCert (breaking).
cardano-api/cardano-api.cabal Adds test-suite dependency and test module entry for new property suite.
.changes/20260811_cardano_api_redeemer_pointer_indexing.yml Changelog fragment classifying the change as bugfix + breaking + tests.
Suppressed comments (1)

cardano-api/src/Cardano/Api/Experimental/Plutus/Internal/IndexedPlutusScriptWitness.hs:121

  • Same issue as certificates: returning LT for any two WitProposal values violates ordering laws required by sortBy. If you want to preserve insertion order for proposals, return EQ and rely on the stability of sortBy to keep the original order.
    -- Proposals are also stored in an `OSet` and resolved positionally
    -- (`StrictSeq.findIndexL`), same as `WitTxCert` above.
    (WitProposal{}, WitProposal{}) -> LT

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cardano-api/src/Cardano/Api/Tx/Internal/Body.hs
(WitProposal propA, WitProposal propB) -> compare propA propB
-- Proposals are also stored in an `OSet` and resolved positionally
-- (`StrictSeq.findIndexL`), same as `WitTxCert` above.
(WitProposal{}, WitProposal{}) -> LT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I missed this.

, wit
)
| (Exp.Certificate cert, Just (stakeCred, wit)) <- getCertificates txCerts
[ (WitTxCert cert, maybe AnyKeyWitnessPlaceholder snd mCredAndWit)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in when we transition to Dijsktra this is when all certificates need a witness. In Conway stake registration certificates don't need any witnesses.

https://github.com/IntersectMBO/cardano-ledger/blob/f8d6ead7c84e87b175efe3259fa838210be6c2d2/eras/conway/impl/src/Cardano/Ledger/Conway/TxCert.hs#L801-L806

@carbolymer carbolymer Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking that. To make the types more precise, without overcomplicating things I think the path forward here is to turn our current definition

newtype TxCertificates era
  = TxCertificates
  {unTxCertificates :: OMap (Exp.Certificate era) (Maybe (StakeCredential, AnyWitness era))}
  deriving (Show, Eq)

into GADT:

data TxCertificates era where
  TxCertificatesConway 
   :: OMap (Exp.Certificate ConwayEra) (Maybe (StakeCredential, AnyWitness ConwayEra))
   -> TxCertificates ConwayEra
  TxCertificatesDijkstra 
   :: OMap (Exp.Certificate DijkstraEra) (StakeCredential, AnyWitness DijkstraEra)
   -> TxCertificates DijkstraEra

After hardforking to Dijkstra we could drop the old era constructor and go back to newtype.

TxCertificates era is however used in compatible API, so this would mean duplicating the newtype definition there.

@carbolymer carbolymer Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative would be to add a type family:

type family CertWitness era where
  CertWitness ShelleyEra = Maybe (StakeCredential, AnyWitness ConwayEra)
  ...
  CertWitness ConwayEra = Maybe (StakeCredential, AnyWitness ConwayEra)
  CertWitness DijkstraEra = (StakeCredential, AnyWitness ConwayEra)

But it would pollute experimental API with older eras information, which is exactly what we want to avoid in experimental api.

@carbolymer
carbolymer force-pushed the mgalazyn/fix/experimental-redeemer-pointer-indexing branch from ab72b80 to 3bde01c Compare August 12, 2026 09:16
Proposal redeemer pointers were computed against Ord-sorted order
instead of the ledger's OSet insertion order, and certifying pointers
did not count unwitnessed certificates' index slots. Both now follow
the ledger's positional resolution, with regression tests pinning the
behaviour and property tests checking every witnessable category's
pointer against the ledger's own Indexable resolution.

The same fix is applied to the deprecated legacy transaction builder
(createTransactionBody); end-to-end and legacy-bridge regression
properties now cover the deprecated path too.

Also harden getVotes against voter-map gaps, remove the unused
StakeCredential field from the Witnessable WitTxCert constructor, and
correct the redeemer-index ordering documentation.
@carbolymer
carbolymer force-pushed the mgalazyn/fix/experimental-redeemer-pointer-indexing branch from 2eb11a2 to e238d9e Compare August 12, 2026 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants