Refactor witnesses indexing functions to have the indexing logic in one place #697

carbolymer · 2024-11-28T13:12:22Z

Changelog

- description: |
    Refactor witnesses indexing functions to have the indexing logic in one place
# 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

It was annoying to look up how witnesses are indexed: the logic was copied into multiple places, away from the datatypes which store them.

This PR moves them into one place, simplifies witness updating code in Cardano.Api.Fees and fixes one place which had wrong witness ordering.

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

carbolymer · 2024-11-28T13:18:07Z

cardano-api/internal/Cardano/Api/Tx/Body.hs

-      | let proposalsList = toList proposalProcedures
-      , (ix, proposal) <- zip [0 ..] proposalsList


This one was incorrect. proposals from mScriptWitnesses not present in proposalProcedures were not added to the list. This issue was fixed in: #602, but it looks like this place was ommited by accident.

Jimbo4350 · 2024-11-28T14:44:32Z

cardano-api/internal/Cardano/Api/Tx/Body.hs

+  -> [(ScriptWitnessIndex, TxIn, Witness WitCtxTxIn era)]
+txInsToIndexed txins =
+  [ (ScriptWitnessIndexTxIn ix, txIn, witness)
+  | (ix, (txIn, BuildTxWith witness)) <- zip [0 ..] $ orderTxIns txins


This will now pattern match on key witnesses because we don't pattern match on ScriptWitness:

scriptWitnessesTxIns txins = [ (ScriptWitnessIndexTxIn ix, AnyScriptWitness witness) | -- The tx ins are indexed in the map order by txid (ix, (_, BuildTxWith (ScriptWitness _ witness))) <- zip [0 ..] (orderTxIns txins)

We need to be extremely careful with the refactors here.

Oh good catch. However, I've copied the indexing logic from Fees.hs:1464:

[ (txin, BuildTxWith <$> wit') | -- The tx ins are indexed in the map order by txid (ix, (txin, BuildTxWith wit)) <- zip [0 ..] (orderTxIns txins) , let wit' = case wit of KeyWitness{} -> Right wit ScriptWitness ctx witness -> ScriptWitness ctx <$> witness' where witness' = substituteExecUnits (ScriptWitnessIndexTxIn ix) witness ]

and there indices were generated for both ScriptWitness and KeyWitness!

Which is correct then?

substituteExecutionUnits would strip TxIns with KeyWitneses from TxBodyContent if we index only scripts. That sounds wrong.
But giving script indices to key witnesses does not seem right as well.

I think we should keep both: calculate indices for scripts only and update only script inputs.

Ok so the actual issue is we do need all the indexes but they are not all necessarily ScriptWitnessIndexes. Some may be witnessed by a key and technically shouldn't have a corresponding ScriptWitnessIndex. However I'm not seeing a simple way to disambiguate this without making the code unnecessarily complicated.

Jimbo4350

LGTM I would consider renaming the indexing functions however. We need to do a follow up PR to prevent the abuse of ScriptWitnessIndex.

cardano-api/internal/Cardano/Api/Fees.hs

Jimbo4350 · 2024-11-29T14:04:53Z

cardano-api/internal/Cardano/Api/Tx/Body.hs

+  -> [(ScriptWitnessIndex, TxIn, Witness WitCtxTxIn era)]
+txInsToIndexed txins =
+  [ (ScriptWitnessIndexTxIn ix, txIn, witness)
+  | (ix, (txIn, BuildTxWith witness)) <- zip [0 ..] $ orderTxIns txins


Ok so the actual issue is we do need all the indexes but they are not all necessarily ScriptWitnessIndexes. Some may be witnessed by a key and technically shouldn't have a corresponding ScriptWitnessIndex. However I'm not seeing a simple way to disambiguate this without making the code unnecessarily complicated.

cardano-api/internal/Cardano/Api/Tx/Body.hs

Jimbo4350 · 2024-11-29T16:59:35Z

cardano-api/internal/Cardano/Api/Tx/Body.hs

@@ -1232,6 +1268,20 @@ deriving instance Eq (TxCertificates build era)

 deriving instance Show (TxCertificates build era)

+-- | Index certificates with witnesses by the order they appear in the list (in the transaction). If there
+-- are multiple witnesses for the credential, the last one is returned.


If there are multiple witnesses for the credential, the last one is returned

Can you elaborate on this? What does this mean/why is this ok?

Actually, I am not sure. I've preserved the original logic without thinking much about it:

mapScriptWitnessesCertificates ( TxCertificates supported certs (BuildTxWith witnesses) ) = let mappedScriptWitnesses :: [(StakeCredential, Either (TxBodyErrorAutoBalance era) (Witness WitCtxStake era))] mappedScriptWitnesses = [ (stakecred, ScriptWitness ctx <$> witness') | -- The certs are indexed in list order (ix, cert) <- zip [0 ..] certs , stakecred <- maybeToList (selectStakeCredentialWitness cert) , ScriptWitness ctx witness <- maybeToList (List.lookup stakecred witnesses) , let witness' = substituteExecUnits (ScriptWitnessIndexCertificate ix) witness ] in TxCertificates supported certs . BuildTxWith <$> traverse ( \(sCred, eScriptWitness) -> case eScriptWitness of Left e -> Left e Right wit -> Right (sCred, wit) ) mappedScriptWitnesses

so this bit:

ScriptWitness ctx witness <- maybeToList (List.lookup stakecred witnesses)

returns only the first witness. The comment mentioning the last witness is wrong.

However:

TxCertificates :: ShelleyBasedEra era -> [Certificate era] -> BuildTxWith build [(StakeCredential, Witness WitCtxStake era)] -- ^ There can be more than one script witness per stake credential -> TxCertificates build era

So the fact that we only use the first witness for the stake credential, feels wrong here.

Actually it should only matter in fee calculation in estimateTransactionKeyWitnessCount, because we were counting witnesses there. So if we used only first witness for the stake credential, the fee was underestimated.

I've included all witnesses for the stake now.

Jimbo4350 · 2024-11-29T17:06:54Z

cardano-api/internal/Cardano/Api/Tx/Body.hs

-          List.lookup stakecred witnesses
+    scriptWitnessesCertificates txc =
+      [ (ix, AnyScriptWitness witness)
+      | (ix, _, _, ScriptWitness _ witness) <- txCertificatesToIndexed txc


It's clearer here that we are using the indexing functions in two contexts:

Script witness execution unit substitution

Retrieval of script witnesses

I would argue having these as separate functions would make this code clearer. However that is not a task for this PR.

In the case of 1. we keep the key witnesses because we are updating the tx body with the set of all things being witnessed. Because of this we abuse ScriptWitnessIndex and pair it with things that are actually key witnessed.

Jimbo4350 · 2024-11-29T17:15:22Z

cardano-api/internal/Cardano/Api/Tx/Body.hs

+  -> [(ScriptWitnessIndex, L.ProposalProcedure (ShelleyLedgerEra era), ScriptWitness WitCtxStake era)]
+txProposalProceduresToIndexed TxProposalProceduresNone = []
+txProposalProceduresToIndexed txpp@(TxProposalProcedures _ (BuildTxWith witnesses)) = do
+  let allProposalsList = toList $ convProposalProcedures txpp


IIRC you were suggesting a pattern synonym to prevent users from having to call convProposalProcedures?

Yes. I was suggesting:

Not exporting TxProposalProcedures constructors and prefix them with Unsafe

Add the following unidirectional pattern:

pattern TxProposalProcedures :: OSet (L.ProposalProcedure (ShelleyLedgerEra era)) -> TxProposalProcedures build era pattern TxProposalProcedures oset <- (convProposalProcedures -> oset)

(export of convProposalProcedures will be unnecessary)
3. Make usage of mkTxProposalProcedures the only way to construct the value.

That would make API safer to use.

LGTM, feel free to add it in this PR or a separate one. Be sure to make any breaking changes clear in the next release notes.

…ne place

Jimbo4350

LGTM! If you have a branch integrated in cardano-cli, I would ask Martin to run tests related to plutus scripts from the branch to be safe.

carbolymer · 2025-01-03T12:25:08Z

Backported to 10.1 and tested in: https://github.com/IntersectMBO/cardano-node-tests/actions/runs/12596557732

carbolymer requested review from dcoutts, Jimbo4350, erikd, newhoggy, smelc, palas, kevinhammond, CarlosLopezDeLara and disassembler as code owners November 28, 2024 13:12

carbolymer commented Nov 28, 2024

View reviewed changes

carbolymer force-pushed the mgalazyn/refactor/move-wit-indexing-logic-into-one-place branch from 4d209e2 to c50555d Compare November 28, 2024 14:33

Jimbo4350 reviewed Nov 28, 2024

View reviewed changes

carbolymer mentioned this pull request Nov 29, 2024

Add certificates support in createCompatibleSignedTx. #691

Merged

3 tasks

Jimbo4350 approved these changes Nov 29, 2024

View reviewed changes

Refactor witnesses indexing functions to have the indexing logic in o…

cba75e6

…ne place

carbolymer force-pushed the mgalazyn/refactor/move-wit-indexing-logic-into-one-place branch 2 times, most recently from 0d22df3 to 3f14ab0 Compare December 30, 2024 20:46

Fix review remarks

c18d177

carbolymer force-pushed the mgalazyn/refactor/move-wit-indexing-logic-into-one-place branch from 3f14ab0 to c18d177 Compare December 31, 2024 17:36

Jimbo4350 approved these changes Jan 2, 2025

View reviewed changes

carbolymer added this pull request to the merge queue Jan 3, 2025

Merged via the queue into master with commit a1a40fa Jan 3, 2025
70 checks passed

carbolymer deleted the mgalazyn/refactor/move-wit-indexing-logic-into-one-place branch January 3, 2025 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor witnesses indexing functions to have the indexing logic in one place #697

Refactor witnesses indexing functions to have the indexing logic in one place #697

carbolymer commented Nov 28, 2024

carbolymer Nov 28, 2024 •

edited

Loading

Jimbo4350 Nov 28, 2024

carbolymer Nov 28, 2024 •

edited

Loading

carbolymer Nov 28, 2024

carbolymer Nov 28, 2024 •

edited

Loading

Jimbo4350 Nov 29, 2024

Jimbo4350 left a comment

Jimbo4350 Nov 29, 2024

Jimbo4350 Nov 29, 2024

carbolymer Dec 30, 2024

carbolymer Dec 30, 2024

carbolymer Dec 30, 2024

Jimbo4350 Nov 29, 2024

Jimbo4350 Nov 29, 2024

carbolymer Dec 30, 2024 •

edited

Loading

Jimbo4350 Jan 2, 2025

Jimbo4350 left a comment

carbolymer commented Jan 3, 2025

		\| let proposalsList = toList proposalProcedures
		, (ix, proposal) <- zip [0 ..] proposalsList

Refactor witnesses indexing functions to have the indexing logic in one place #697

Refactor witnesses indexing functions to have the indexing logic in one place #697

Conversation

carbolymer commented Nov 28, 2024

Changelog

Context

Checklist

carbolymer Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

carbolymer commented Jan 3, 2025

carbolymer Nov 28, 2024 •

edited

Loading

carbolymer Nov 28, 2024 •

edited

Loading

carbolymer Nov 28, 2024 •

edited

Loading

carbolymer Dec 30, 2024 •

edited

Loading