Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix babbage min utxo calculation #2766

Merged
merged 4 commits into from May 3, 2022
Merged

Conversation

JaredCorduan
Copy link
Contributor

Fix the bug in the Babbage minimum utxo calculation. closes #2765

This commit can be review commit by commit:

  • The first commit removes the wildcard pattern matching in updaters. We were silently ignoring updates to the AdaPerUTxOWord field.
  • The next commit creates a unit test which exhibits the bug described in Min ada requirement - Babbage #2765.
  • The next commit fixes the bug. We now correctly count the size of all the bytes in the serialized outputs.
  • The last commit swaps out the Alonzo predicate failure for violations of the min-utxo check for a more descriptive failure. In particular, the failure now tells you what the minimum value was for the given offending outputs.

In addition to the unit test added in this PR, we will soon change the value of AdaPerUTxOWord in the property tests to something non-zero.

Jared Corduan added 4 commits May 2, 2022 16:03
We should be forced to think about each protocol parameter in each
error to ensure we do not falsely assume a field in a given era is being
set but is actually being ignored.
We make a unit test that we expect to fail, namely we create a
transaction output that is large due an inline datum which should
trigger the OutputTooSmallUTxO failure.
It should not be based on the serialized size of the value, but rather
the serialized size of the entire transaction output.
The OutputTooSmallUTxO in Alonzo does not tell the user what the minimum
size actually was for the given transaction output, which is very
helpful information. We include this information in the babbage era
predicate failure.
Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

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

Looks good to me. I am serious about the adding the hash. We should do that for every new predicate failure we add from now on. Is it too late for us to that all over Babbage?

Comment on lines +111 to 115
| -- | list of supplied transaction outputs that are too small,
-- together with the minimum value for the given output.
BabbageOutputTooSmallUTxO
![(Core.TxOut era, Integer)]

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please include the Hash of the TxBody here as well. It is often impossible to determine which Tx an predicate failure arise from. This would be very useful in may paces, but at least we have the opportunity to do it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a bigger refactor than I was looking to do in this PR... at least if we want any kind of consistency... maybe we can make that a stretch goal in a follow up PR? this is helpful for us when testing, but is probably not actually helpful for most users, who get the predicate failure after locally submitting a single Tx.

Comment on lines +261 to +262
CollateralPercentage 1,
AdaPerUTxOWord (Coin 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a percentage,or is it a rational (or float) between 0 and 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a number of lovelace

@JaredCorduan JaredCorduan merged commit 84d0a35 into master May 3, 2022
@iohk-bors iohk-bors bot deleted the jc/fix-babbage-min-utxo branch May 3, 2022 15:27
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.

Min ada requirement - Babbage
2 participants