Update min_ada_required() to Alonzo rules #219

rooooooooob · 2021-10-08T07:52:33Z

Following: https://github.com/input-output-hk/cardano-ledger-specs/blob/master/doc/explanations/min-utxo-alonzo.rst

Some tests in tx_builder.rs still need to be updated

Following: https://github.com/input-output-hk/cardano-ledger-specs/blob/master/doc/explanations/min-utxo-alonzo.rst Some tests in tx_builder.rs still need to be updated

rust/src/tx_builder.rs

rooooooooob · 2021-10-08T07:55:33Z

rust/src/utils.rs

+        let utxo_entry_size_without_val = 27; // in words
+        let size = bundle_size(
+            &assets,
+            &OutputSizeConstants {


These are actually fixed values in the doc so I'm not sure if it's worth it to pass in vs just hardcoding inside of bundle_size() anymore.

rooooooooob · 2021-10-08T07:57:56Z

rust/src/utils.rs

+            ada_only_utxo_size,
+            min_utxo.0
+        )))
+    } else {


The doc doesn't mention explicitly about combining data hash with the min ada-only utxo value. It just says that is 1,000,000 but if you do the size calculation on it without a data hash it's actually like ~950,000.

@Scitz0 mentioned that:

I don't fully trust the linked alonzo min utxo ledger spec.
For example, taking coinSize into consideration and looking at Mary min-utxo ledger spec document, for Ada only its calculated as:
adaOnlyUTxOSize = utxoEntrySizeWithoutVal + coinSize = 27
Switching coinSize to 2 gives an adaOnlyUTxOSize value of 29. If you multiple this with coinsPerUTxOWord you get a value of 999978 Lovelace. For an Ada-only transaction, this is a perfectly valid output to use. Tested on both TestNet and MainNet.

rooooooooob · 2021-10-08T07:58:21Z

rust/src/utils.rs

    #[test]
-    fn proper_asset_size_calculation() {


I'm not sure where this test data came from. I got all the tests I added from the doc.

I think it was added based on this issue:
#194

…ize 0 -> 2 for ada-only min add calculations

rooooooooob · 2021-10-14T05:33:56Z

rust/src/tx_builder.rs

        pool_deposit: &BigNum, // protocol parameter
        key_deposit: &BigNum,  // protocol parameter
        max_value_size: u32, // protocol parameter
        max_tx_size: u32, // protocol parameter
+        coins_per_utxo_word: &Coin, // protocol parameter


I ended up moving this to the end instead of keeping it where it was to purposefully break the API so that people won't accidentally supply the type-compatible minimum_utxo_val to coins_per_utxo_word which would result in massively different minimum ada calculations that will be off by ~30 times.

rooooooooob · 2021-10-14T05:35:32Z

rust/src/tx_builder.rs

@@ -680,8 +681,8 @@ mod tests {
            tx_builder.get_explicit_input().unwrap().checked_add(&tx_builder.get_implicit_input().unwrap()).unwrap(),
            tx_builder.get_explicit_output().unwrap().checked_add(&Value::new(&tx_builder.get_fee_if_set().unwrap())).unwrap()
        );
-        assert_eq!(tx_builder.full_size().unwrap(), 283);
-        assert_eq!(tx_builder.output_sizes(), vec![61, 65]);
+        assert_eq!(tx_builder.full_size().unwrap(), 284);


I had to bump these up by 1 due to how CBOR encodes data. Integers below I think 20 are stored in 1 byte with the value embedded in the type tag, but valid values with the supplied 1 coin per utxo word (=29 for asset-less outputs) now take 2 bytes for UINT tag + anther byte for the number 29.

rooooooooob · 2021-10-14T05:36:11Z

rust/src/tx_builder.rs

@@ -665,7 +666,7 @@ mod tests {
        );
        tx_builder.add_output(&TransactionOutput::new(
            &addr_net_0,
-            &Value::new(&to_bignum(10))
+            &Value::new(&to_bignum(29))


If anyone's wondering, 29 is the new adda only min utxo when you have coin size per utxo word = 1

rooooooooob

According to @Scitz0 :

I don't fully trust the linked alonzo min utxo ledger spec.
For example, taking coinSize into consideration and looking at Mary min-utxo ledger spec document, for Ada only its calculated as:
adaOnlyUTxOSize = utxoEntrySizeWithoutVal + coinSize = 27
Switching coinSize to 2 gives an adaOnlyUTxOSize value of 29. If you multiple this with coinsPerUTxOWord you get a value of 999978 Lovelace. For an Ada-only transaction, this is a perfectly valid output to use. Tested on both TestNet and MainNet.

For output containing a token however the formula showed in alonze spec seem fine, e.g
coinsPerUTx, Word * ( utxoEntrySizeWithoutVal + size (v) + dataHashSize (dh) )

So I made this change since apparently that is what is accepted by mainnet, not the mentioned 1_000_000 fixed value before. We might want to verify this 100% though.

Scitz0 · 2021-10-14T08:16:48Z

So I made this change since apparently that is what is accepted by mainnet, not the mentioned 1_000_000 fixed value before. We might want to verify this 100% though.

As an assurance for the value this is a screenshot from Daedalus Flight 4.3.1.

vsubhuman · 2021-11-01T08:26:50Z

Thank you, @rooooooooob! This is a great change, but it's really breaking tho, so it gonna need to be in the next version 10.0.0

pedromtcosta

Looks good!

vsubhuman

/check

vsubhuman · 2021-11-04T09:44:42Z

Changed target to another local branch alonzo-min-utxo-size to resolve conflicts and run tests first

Update min_ada_required() to Alonzo rules

40565b9

Following: https://github.com/input-output-hk/cardano-ledger-specs/blob/master/doc/explanations/min-utxo-alonzo.rst Some tests in tx_builder.rs still need to be updated

rooooooooob mentioned this pull request Oct 8, 2021

coinSize in Alonzo era #218

Closed

rooooooooob commented Oct 8, 2021

View reviewed changes

rust/src/tx_builder.rs Show resolved Hide resolved

rooooooooob commented Oct 8, 2021

View reviewed changes

fix txbuilder tests + remove 1000000 min for min utxo + change coin_s…

4da9f6e

…ize 0 -> 2 for ada-only min add calculations

rooooooooob commented Oct 14, 2021

View reviewed changes

rooooooooob marked this pull request as ready for review October 14, 2021 05:39

vsubhuman added this to the 9.2.0 milestone Oct 19, 2021

vsubhuman self-requested a review October 19, 2021 15:09

Merge branch 'master' into alonzo-min-utxo-size

abcd6c2

vsubhuman modified the milestones: 9.2.0, 10.0.0 Nov 1, 2021

vsubhuman linked an issue Nov 1, 2021 that may be closed by this pull request

coinSize in Alonzo era #218

Closed

vsubhuman requested a review from pedromtcosta November 1, 2021 15:39

pedromtcosta approved these changes Nov 1, 2021

View reviewed changes

vsubhuman modified the milestones: 10.0.0, 9.2.0 Nov 2, 2021

vsubhuman approved these changes Nov 4, 2021

View reviewed changes

vsubhuman changed the base branch from master to alonzo-min-utxo-size November 4, 2021 08:40

vsubhuman changed the base branch from alonzo-min-utxo-size to master November 4, 2021 08:41

vsubhuman reviewed Nov 4, 2021

View reviewed changes

vsubhuman changed the base branch from master to alonzo-min-utxo-size November 4, 2021 09:44

vsubhuman merged commit 308800a into Emurgo:alonzo-min-utxo-size Nov 4, 2021

vsubhuman mentioned this pull request Nov 4, 2021

[pull] Update min_ada_required() to Alonzo rules #248

Merged

vsubhuman mentioned this pull request Dec 7, 2021

BETA Release / 10.0.0-beta.8 #283

Merged

vsubhuman mentioned this pull request Feb 6, 2022

Release / 10.0.0 #358

Merged

40 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update min_ada_required() to Alonzo rules #219

Update min_ada_required() to Alonzo rules #219

rooooooooob commented Oct 8, 2021

rooooooooob Oct 8, 2021

rooooooooob Oct 8, 2021

rooooooooob Oct 13, 2021

rooooooooob Oct 8, 2021

Scitz0 Oct 8, 2021

rooooooooob Oct 14, 2021

rooooooooob Oct 14, 2021

rooooooooob Oct 14, 2021

rooooooooob left a comment

Scitz0 commented Oct 14, 2021 •

edited

Loading

vsubhuman commented Nov 1, 2021

pedromtcosta left a comment

vsubhuman left a comment

vsubhuman commented Nov 4, 2021

Update min_ada_required() to Alonzo rules #219

Update min_ada_required() to Alonzo rules #219

Conversation

rooooooooob commented Oct 8, 2021

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

rooooooooob left a comment

Choose a reason for hiding this comment

Scitz0 commented Oct 14, 2021 • edited Loading

vsubhuman commented Nov 1, 2021

pedromtcosta left a comment

Choose a reason for hiding this comment

vsubhuman left a comment

Choose a reason for hiding this comment

vsubhuman commented Nov 4, 2021

Scitz0 commented Oct 14, 2021 •

edited

Loading