Anastasia-Labs / lucid-evolution

https://anastasia-labs.github.io/lucid-evolution/
44 stars 19 forks source link

Fee calculation error when spending UTXO with scriptRef #223

Closed SynthLuvr closed 2 months ago

SynthLuvr commented 2 months ago

Steps to reproduce:

This will cause the fee calculation to be incorrect and cannot be submitted on-chain

SynthLuvr commented 2 months ago

Example on preview network:

const address =
  "addr_test1qqja0lh0lnpwd7hewzptwpay09mgqm2k0srqsrfp89nya2h3uq0x2xemxfac2lf7axszqezaw2tpahj2d23guffx687sqz8edy";
const utxos = await lucid.utxosAt(address);
const destination = await getRandomAddress(lucid);

lucid.selectWallet.fromAddress(address, utxos);
const tx = await lucid
  .newTx()
  .pay.ToAddress(destination, {
    "20a814aa7a68d16b8d236f21a4dcb1ed1e1b359faff67287da2c33dd626561636f6e":
      1n,
  })
  .complete();

image

nikhils9 commented 2 months ago

I think I know what's being missed here. I'll get it fixed.

nikhils9 commented 2 months ago

Currently, fee calculation is incorrect for a tx which spends utxos (containing ref scripts) added by lucid's coin section. After giving some though, I believe utxos with scriptRef attached to them should not be selected by coin selection in the first place. They should be spent if explicitly intended by the user using collectFrom. If we go with this approach, the fix to this issue would be very simple otherwise it would require more complex logic at the level of coin selection and fees calculation.

I would like to know your opinions. @solidsnakedev @SynthLuvr @keyan-m @hadelive

PS: A similar argument can be made about utxos with datums attached to them. But since such utxos would most likely be return outputs from dapps, I believe its better to include them in coin selection (as is the case currently).

SynthLuvr commented 2 months ago

Sometimes we filter out UTXOs with datums from the wallet ourselves to avoid them being spent. I think by default they should be included because there are dapps that attach datums as a "receipt."

Regarding scriptRef, I think it'd be fine to spend it explicitly via collectFrom.

SynthLuvr commented 2 months ago

When using collectFrom it's still the same error image

sourabhxyz commented 2 months ago

This could be due to updated fee structure in Conway (active in preview testnet) where reference scripts now carry additional weight, more details here.

SynthLuvr commented 2 months ago

This could be due to updated fee structure in Conway (active in preview testnet) where reference scripts now carry additional weight, more details here.

Yes this is true. The fee for reading from reference scripts was already implemented in https://github.com/Anastasia-Labs/lucid-evolution/pull/195. However, this new issue is related to spending UTXOs with reference scripts attached. I don't think there should be a fee attached for spending reference scripts. However, assuming this stays in cardano-ledger, lucid-evolution will need to be updated to account for this extra fee.

sourabhxyz commented 2 months ago

@SynthLuvr thanks for explaining! I haven't looked carefully at this issue, but

However, this new issue is related to spending UTXOs with reference scripts attached.

In my understanding, this additional fee is enforced irrespective of whether you are actually making use of reference script in your tx or not. As long as spend inputs or reference inputs have script attached to them, they would contribute to fees.

SynthLuvr commented 2 months ago

In my understanding, this additional fee is enforced irrespective of whether you are actually making use of reference script in your tx or not. As long as spend inputs or reference inputs have script attached to them, they would contribute to fees.

Yes that's correct. I think this was overlooked in the cardano-ledger implementation. Given that this is related to a vulnerability that was exploited, the implementation may have been rushed. It doesn't make sense that this should be contributing to fees, because there's no vulnerability associated with spending data, only referencing data within validation logic.

Having said that, these are the current ledger rules for conway, so if these rules remain as-is, then we have to adapt and add support for the fee.

sourabhxyz commented 2 months ago

@SynthLuvr Not sure if I am following properly, but generating script context requires resolving inputs, in particular also mentioning associated script hash (if any) which thus a Plutus script can depend upon, so even if a given input's script is not being exercised for validation, it could be referred by other script.

nikhils9 commented 2 months ago

so even if a given input's script is not being exercised for validation, it could be referred by other script.

That's an interesting point. I guess even if such a scriptRef is being referred by another script (which is witnessing the transaction), the witnessing script will bear the cost of deserializing the scriptRef thereby accounting for the cost via its increased exUnits. The ledger need not presume and charge every scriptRef in inputs being spent.

nikhils9 commented 2 months ago

When using collectFrom it's still the same error image

After spending some good amount of time, I was finally able to get this working. Will need to test it further though.

To get ref scripts fee working, without it being calculated by CML, I had to resort setting fees in CML using TransactionBuilder::set_fee which brings along its own set of issues. This issue was one such instance of it where the ref fees were being calculated correctly however the estimated fees were set before the coin selection was completed which left room for fees to increase further.

SynthLuvr commented 2 months ago

That's an interesting point. I guess even if such a scriptRef is being referred by another script (which is witnessing the transaction), the witnessing script will bear the cost of deserializing the scriptRef thereby accounting for the cost via its increased exUnits. The ledger need not presume and charge every scriptRef in inputs being spent.

Right. So effectively with the current ledger-rules, we're paying extra for no reason. If a reference is being used, it'll be witnessed, and thus we'll pay the fee. But I don't see the logic for how it's currently implemented where in essence scriptRef is no different than a datum.

solidsnakedev commented 2 months ago

just to clarify the deserialization of the reference scripts (flat encoded) are done by the cardano nodes, and according to the ledger it is mandatory to count all the inputs with reference scripts, even if they're two or more inputs with the same reference script. Also I think we should do this calculation inside the recursive coin selection, and add the extra cost for each added input https://github.com/Anastasia-Labs/lucid-evolution/blob/2617572fa8a39f4f299119abc94c0f0a8ccd932a/packages/lucid/src/tx-builder/internal/CompleteTxBuilder.ts#L636-L692

solidsnakedev commented 2 months ago

There's also a CML update in the work, but I think we're much closer to get it fixed. https://github.com/dcSpark/cardano-multiplatform-lib/pull/349