SCP-2945: Only script outputs in unbalanced transactions #57

sjoerdvisscher · 2021-10-27T13:09:57Z

Pre-submit checklist:

Branch
- Tests are provided (if possible)
- Commit sequence broadly makes sense
- Key commits have useful messages
- Relevant tickets are mentioned in commit messages
- Formatting, materialized Nix files, PNG optimization, etc. are updated
PR
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested

sjoerdvisscher · 2021-10-27T15:05:46Z

plutus-ledger/src/Ledger/Constraints/OffChain.hs

@@ -370,7 +392,7 @@ updateUtxoIndex
    => m ()
 updateUtxoIndex = do
    ScriptLookups{slTxOutputs} <- ask
-    unbalancedTx . utxoIndex <>= fmap Tx.toTxOut slTxOutputs
+    unbalancedTx . utxoIndex <>= Map.mapMaybe toScriptOutput slTxOutputs


This is silently dropping public key outputs. I think that is OK, but I might be wrong. An alternative could be to let slTxOutputs also only be ScriptOutputs. Then callers of the unspentOutputs function would need to choose what to do in the case of public key outputs.

I tried this but that breaks lookupTxOutRef.

The public key inputs case for lookupTxOutRef is used for the MustSpendPubKeyOutput constraint. Given that we can't instruct the wallet to spend a specific public key output, maybe we should get rid of this constraint altogether? Although it still seems useful in the on-chain code, and you can definitely build a valid transaction that satisfies it. Just not with cardano-wallet. So I think dropping the public key outputs here with mapMaybe is OK.

And cardano-wallet might support it in the future, right?

j-mueller

Interesting, I thought this would break the state machine initialisation code. Presumably it still works because the wallet selects the right input anyway.

j-mueller · 2021-10-29T06:57:54Z

plutus-ledger/src/Ledger/Constraints/OffChain.hs

@@ -370,7 +392,7 @@ updateUtxoIndex
    => m ()
 updateUtxoIndex = do
    ScriptLookups{slTxOutputs} <- ask
-    unbalancedTx . utxoIndex <>= fmap Tx.toTxOut slTxOutputs
+    unbalancedTx . utxoIndex <>= Map.mapMaybe toScriptOutput slTxOutputs


The public key inputs case for lookupTxOutRef is used for the MustSpendPubKeyOutput constraint. Given that we can't instruct the wallet to spend a specific public key output, maybe we should get rid of this constraint altogether? Although it still seems useful in the on-chain code, and you can definitely build a valid transaction that satisfies it. Just not with cardano-wallet. So I think dropping the public key outputs here with mapMaybe is OK.

koslambrou · 2021-11-03T13:27:18Z

plutus-ledger/src/Ledger/Constraints/OffChain.hs

+data ScriptOutput =
+    ScriptOutput
+        { scriptOutputValidatorHash :: ValidatorHash
+        , scriptOutputValue         :: Value
+        , scriptOutputDatumHash     :: DatumHash
+        }
+    deriving stock (Eq, Generic, Show)
+    deriving anyclass (FromJSON, ToJSON, OpenApi.ToSchema)
+
+toScriptOutput :: ChainIndexTxOut -> Maybe ScriptOutput
+toScriptOutput (Tx.ScriptChainIndexTxOut _ validatorOrHash datumOrHash v)
+    = Just $ ScriptOutput (either id validatorHash validatorOrHash) v (either id datumHash datumOrHash)
+toScriptOutput Tx.PublicKeyChainIndexTxOut{}
+    = Nothing
+
+fromScriptOutput :: ScriptOutput -> ChainIndexTxOut
+fromScriptOutput (ScriptOutput vh v dh) =
+    Tx.ScriptChainIndexTxOut (Address.scriptHashAddress vh) (Left vh) (Left dh) v
+
+instance Pretty ScriptOutput where
+    pretty ScriptOutput{scriptOutputValidatorHash, scriptOutputValue} =
+        hang 2 $ vsep ["-" <+> pretty scriptOutputValue <+> "addressed to", pretty scriptOutputValidatorHash]
+


Maybe put this in Ledger.Tx together with ChainIndexTxOut?

# Conflicts: # playground-common/src/PSGenerator/Common.hs # plutus-contract/src/Plutus/Contract/Wallet.hs # plutus-use-cases/test/Spec/PubKey.hs

sjoerdvisscher · 2021-11-12T13:59:16Z

plutus-contract/src/Wallet/Emulator/Wallet.hs

@@ -268,7 +268,7 @@ validateTxAndAddFees feeCfg slotCfg ownTxOuts utx = do
    -- Balance and sign just for validation
    tx <- handleBalanceTx ownTxOuts utx
    signedTx <- handleAddSignature tx
-    let utxoIndex        = Ledger.UtxoIndex $ unBalancedTxUtxoIndex utx <> (toTxOut <$> ownTxOuts)
+    let utxoIndex        = Ledger.UtxoIndex $ fmap toTxOut $ (U.fromScriptOutput <$> unBalancedTxUtxoIndex utx) <> ownTxOuts


@j-mueller After thinking some more this here seems to be the clue that there is no failure. The PubKeys are only unavailable in the lookups. But the wallet knows how to lookup its own outputs (ownTxOuts here) so getUnspentOutput just works!

getUnspentOutput works, and it gives you an unspent output from the wallet. But there is no guarantee that the wallet selects the same unspent output when balanceTx is called a second time, for the transaction that creates the token by spending this specific output.

The UTXO could have been spent in the meantime, or maybe the wallet decides that there is a different collection of UTXOs that is better suited in the second call to balanceTx.

Because of this, I'm inclined to think that we have to go back to creating a script output (with the PubKey contract) in a separate transaction before we can set up the state machine. What do you think @sjoerdvisscher @silky ?

@j-mueller I agree, but that’s a separate issue I think? So we can merge this one?

Yes, let's merge it!

Only script outputs in unbalanced transactions

af6041d

sjoerdvisscher force-pushed the SCP-2945-only-script-inputs-in-utx branch from af38edd to af6041d Compare October 27, 2021 13:55

sjoerdvisscher requested a review from j-mueller October 27, 2021 15:00

sjoerdvisscher commented Oct 27, 2021

View reviewed changes

Don't use ScriptLookups internals in PubKey tests

ce9c6dc

j-mueller approved these changes Oct 29, 2021

View reviewed changes

koslambrou reviewed Nov 3, 2021

View reviewed changes

sjoerdvisscher added 3 commits November 4, 2021 15:46

Merge branch 'main' into SCP-2945-only-script-inputs-in-utx

2cc8057

# Conflicts: # playground-common/src/PSGenerator/Common.hs # plutus-contract/src/Plutus/Contract/Wallet.hs # plutus-use-cases/test/Spec/PubKey.hs

Fix merge issue

e7f3418

Merge branch 'main' into SCP-2945-only-script-inputs-in-utx

334c5eb

sjoerdvisscher commented Nov 12, 2021

View reviewed changes

Merge branch 'main' into SCP-2945-only-script-inputs-in-utx

7b57967

sjoerdvisscher merged commit 0709726 into main Nov 22, 2021

sjoerdvisscher deleted the SCP-2945-only-script-inputs-in-utx branch November 22, 2021 09:34

sjoerdvisscher mentioned this pull request Jan 26, 2022

SCP-3305 SCP-3263 fixed Ledger.Constraints.Offchain.updateUtxoIndex #275

Merged

8 tasks

bwbush added a commit that referenced this pull request Jan 27, 2022

SCP-3305 reverted #57

d23de94

bwbush added a commit that referenced this pull request Jan 27, 2022

SCP-3305 reverted #57

d08145b

bwbush added a commit that referenced this pull request Jan 28, 2022

SCP-3305 reverted #57 (#275)

ea1bfc6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCP-2945: Only script outputs in unbalanced transactions #57

SCP-2945: Only script outputs in unbalanced transactions #57

sjoerdvisscher commented Oct 27, 2021 •

edited

Loading

sjoerdvisscher Oct 27, 2021

sjoerdvisscher Oct 28, 2021

j-mueller Oct 29, 2021

sjoerdvisscher Oct 29, 2021

j-mueller Oct 29, 2021

j-mueller left a comment

j-mueller Oct 29, 2021

koslambrou Nov 3, 2021

sjoerdvisscher Nov 12, 2021

j-mueller Nov 22, 2021

j-mueller Nov 22, 2021

sjoerdvisscher Nov 22, 2021

j-mueller Nov 22, 2021

SCP-2945: Only script outputs in unbalanced transactions #57

SCP-2945: Only script outputs in unbalanced transactions #57

Conversation

sjoerdvisscher commented Oct 27, 2021 • edited 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

j-mueller 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

sjoerdvisscher commented Oct 27, 2021 •

edited

Loading