Replace `Set TxIn` with `[TxIn]` to fix `StateMachine.getInput` #594

ghost · 2022-07-14T06:31:29Z

This PR tries to fix the problem with StateMachine.getInput by properly querying the redeemer of the input by its index.

Pre-submit checklist:

Branch
- Tests are provided (if possible)
- Commit sequence broadly makes sense
- Key commits have useful messages
- Formatting, PNG optimization, etc. are updated
PR
- Self-reviewed the diff
- Useful pull request description
- Reference the ADR in the PR and reference the PR in the ADR (if revelant)
- Reviewer requested

ghost · 2022-07-14T06:33:04Z

plutus-chain-index-core/src/Plutus/ChainIndex/Tx.hs

-                    , _citxInputs = txInputs
+                    , _citxInputs = Set.toList txInputs


This is incorrect but to fix it we need to fix the Tx from plutus-core. Or create our own Tx with [TxIn] instead of Set TxIn.

Can you add a TODO and a story for this? I guess it will need to be done once next-node is merged in main.

No need imho, this is good enough.

ghost · 2022-07-14T06:35:00Z

plutus-chain-index-core/src/Plutus/Contract/CardanoAPI.hs

-        inputs =
+        -- We need to sort the inputs as the order is important
+        -- to find the corresponding redeemers
+        inputs = sort $


Here is the correct order of inputs as we sort cardano-api's TxIn with an indented Ord instance.

Good catch, I would have thought they would already be in the right order!

ghost · 2022-07-14T06:36:57Z

plutus-contract/src/Plutus/Contract/StateMachine.hs

-      listToMaybe $ mapMaybe Tx.inScripts $ filter (\Tx.TxIn{Tx.txInRef} -> outRef == txInRef) $ Set.toList $ _citxInputs tx
+    -- We retrieve the correspondent redeemer according to the index of txIn in the list
+    let findRedeemer (ix, _) = Map.lookup (Tx.RedeemerPtr Tx.Spend ix) (_citxRedeemers tx)
+    Ledger.Redeemer r <- listToMaybe $ mapMaybe findRedeemer $ filter (\(_, Tx.TxIn{Tx.txInRef}) -> outRef == txInRef) $ zip [0..] $ _citxInputs tx


Here is where we use the knowledge of inputs order to get the redeemer. I hope I got the idea correctly that index of TxIn is binded to the redeemer pointer.

Source: IntersectMBO/cardano-node#3389 (comment)

Maybe instead of relying on their order it's better to include the index. Set TxIn ~> Set (TxInIndx, TxIn), that would make it more explicit.

The fact that we now have a list instead of a set makes it clear enough that the order is significant in my opinion.

plutus-ledger/src/Ledger/Tx/CardanoAPI.hs

andreabedini · 2022-07-14T06:47:41Z

plutus-chain-index-core/src/Plutus/ChainIndex/Tx.hs

  where
-    withHash (ConsumeScriptAddress val red dat) =
+    -- TODO: the index of the txin is probably incorrect as we take it from the set.


Is this going to have bad consequences?

My understanding is that this PR makes a move from a completely broken state to a semi-broken state. Instead of not getting redeemers at all, we will start choosing them but with a big chance of selecting the incorrect ones.

Speaking of fromOnChainTx in particular, I think withHash function was useless already since we use the Tx here, and the inputs have an empty TxInType here as I understand. And we might skip the ConsumeScriptAddress case at all always return mempty.

As long as the redeemer pointer points to the right input, I think this is totally fine.

To show that there is a relation between the list and the indexes, I think it's better to do the Set.toList once and pass the list to validators.

As long as the redeemer pointer points to the right input, I think this is totally fine.

There is a problem here that we take a set that could break the order. I looked at usages of the fromOnChainTx and there is no anything that directly relies on that order. But would be nice to fix Tx type.

sjoerdvisscher · 2022-07-14T07:46:28Z

plutus-chain-index-core/src/Plutus/ChainIndex/Tx.hs

  where
-    withHash (ConsumeScriptAddress val red dat) =
+    -- TODO: the index of the txin is probably incorrect as we take it from the set.


As long as the redeemer pointer points to the right input, I think this is totally fine.

To show that there is a relation between the list and the indexes, I think it's better to do the Set.toList once and pass the list to validators.

sjoerdvisscher · 2022-07-14T08:41:48Z

plutus-chain-index-core/src/Plutus/Contract/CardanoAPI.hs

-        inputs =
+        -- We need to sort the inputs as the order is important
+        -- to find the corresponding redeemers
+        inputs = sort $


Good catch, I would have thought they would already be in the right order!

sjoerdvisscher · 2022-07-14T08:45:24Z

plutus-contract/src/Plutus/Contract/StateMachine.hs

-      listToMaybe $ mapMaybe Tx.inScripts $ filter (\Tx.TxIn{Tx.txInRef} -> outRef == txInRef) $ Set.toList $ _citxInputs tx
+    -- We retrieve the correspondent redeemer according to the index of txIn in the list
+    let findRedeemer (ix, _) = Map.lookup (Tx.RedeemerPtr Tx.Spend ix) (_citxRedeemers tx)
+    Ledger.Redeemer r <- listToMaybe $ mapMaybe findRedeemer $ filter (\(_, Tx.TxIn{Tx.txInRef}) -> outRef == txInRef) $ zip [0..] $ _citxInputs tx


The fact that we now have a list instead of a set makes it clear enough that the order is significant in my opinion.

Replace Set TxIn with [TxIn] to fix StateMachine.getInput

a98252a

ghost requested review from sjoerdvisscher, koslambrou and andreabedini July 14, 2022 06:31

ghost commented Jul 14, 2022

View reviewed changes

fixes

d2ad561

ghost mentioned this pull request Jul 14, 2022

Make getCardanoTxInputs return TxInType #582

Closed

8 tasks

andreabedini reviewed Jul 14, 2022

View reviewed changes

Evgenii Akentev added 2 commits July 14, 2022 12:07

fix imports

dc4ef2b

fix imports

9680a91

sjoerdvisscher approved these changes Jul 14, 2022

View reviewed changes

update purs

d69d049

ghost merged commit b7c773c into main Jul 15, 2022

ghost deleted the fix-statemachine-getinput branch July 15, 2022 13:29

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `Set TxIn` with `[TxIn]` to fix `StateMachine.getInput` #594

Replace `Set TxIn` with `[TxIn]` to fix `StateMachine.getInput` #594

ghost commented Jul 14, 2022 •

edited by ghost

Loading

ghost Jul 14, 2022

koslambrou Jul 14, 2022

sjoerdvisscher Jul 14, 2022

ghost Jul 14, 2022

sjoerdvisscher Jul 14, 2022

ghost Jul 14, 2022

sjoerdvisscher Jul 14, 2022

andreabedini Jul 14, 2022

ghost Jul 14, 2022

ghost Jul 14, 2022 •

edited by ghost

Loading

sjoerdvisscher Jul 14, 2022

ghost Jul 15, 2022

sjoerdvisscher Jul 14, 2022

sjoerdvisscher Jul 14, 2022

sjoerdvisscher Jul 14, 2022

Replace Set TxIn with [TxIn] to fix StateMachine.getInput #594

Replace Set TxIn with [TxIn] to fix StateMachine.getInput #594

Conversation

ghost commented Jul 14, 2022 • edited by ghost Loading

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

Choose a reason for hiding this comment

ghost Jul 14, 2022 • edited by ghost Loading

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

Replace `Set TxIn` with `[TxIn]` to fix `StateMachine.getInput` #594

Replace `Set TxIn` with `[TxIn]` to fix `StateMachine.getInput` #594

ghost commented Jul 14, 2022 •

edited by ghost

Loading

ghost Jul 14, 2022 •

edited by ghost

Loading