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

Add integration test for Babbage minUTxOValue validation precision #3426

Merged
merged 3 commits into from
Aug 10, 2022

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Aug 5, 2022

Comments

Issue Number

https://input-output.atlassian.net/browse/ADP-1978

@Anviking Anviking self-assigned this Aug 5, 2022
@Anviking Anviking force-pushed the anviking/min-utxo-integration-test branch from dc64baa to 21e8bcf Compare August 5, 2022 15:31
@Anviking
Copy link
Member Author

Anviking commented Aug 5, 2022

bors try

Base automatically changed from jonathanknowles/minimum-utxo-improvements to master August 5, 2022 20:21
@jonathanknowles jonathanknowles force-pushed the anviking/min-utxo-integration-test branch from 21e8bcf to f1ca235 Compare August 9, 2022 01:01
Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @Anviking

Thanks for creating this PR!

Just a couple of comments:

  1. I think it's worth explaining what we mean by "caution" and "overestimation" w.r.t. to the coin selection and migration algorithms' minimum ada computations. (See comment.)
  2. The indentation seems to have been mangled. I pushed an extra commit to fix this (and left your original commit alone). Feel free to squash into your commit if you'd prefer.

Comment on lines 229 to 230
-- Coin selection will out of caution overestimate the size of the ada
-- quantity in its calculation when assigning a new minUTxOValue. In
Copy link
Member

@jonathanknowles jonathanknowles Aug 9, 2022

Choose a reason for hiding this comment

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

I think it might be worth explaining what we mean by "caution" and "overestimation" here.

Suggestion:

When producing auto-generated outputs, the coin selection and migration algorithms both use a minimum ada quantity function whose result can always be safely increased. This allows these algorithms the flexibility to freely reassign excess ada to auto-generated outputs without the risk of producing invalid ada quantities.

This means that auto-generated outputs are assigned an initial ada quantity a1 that is strictly in excess of the minimum required quantity a0, where the excess a1 - a0 is exactly equal to 17,240 lovelace.

However, when validating user-specified outputs with pre-defined ada quantities, the coin selection algorithm merely verifies that the pre-defined ada quantities are valid without adjusting them in any way.

This test verifies that the wallet doesn't require the aforementioned excess for user-specified values with pre-defined ada quantities.

Copy link
Member Author

@Anviking Anviking Aug 9, 2022

Choose a reason for hiding this comment

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

071cce4 - what do you think?

When producing auto-generated outputs, the coin selection and migration algorithms both use a minimum ada quantity function whose result can always be safely increased. This allows these algorithms the flexibility to freely reassign excess ada to auto-generated outputs without the risk of producing invalid ada quantities.

I'm not sure we need/want to give rational for this here. But I changed "will out of caution" to a softer/more future-proof "may", as I believe even some "ideal", fix-point-aware solution would be allowed to overestimate.

@Anviking Anviking force-pushed the anviking/min-utxo-integration-test branch from 9eb41c1 to 071cce4 Compare August 9, 2022 11:22
Anviking and others added 2 commits August 10, 2022 10:26
Fails on master with:

  src/Test/Integration/Scenario/API/Shelley/Transactions.hs:276:11:
  1) API Specifications, SHELLEY_TRANSACTIONS, TRANS_MIN_UTXO_01b - I can spend exactly minUTxOValue
       From the following response: Left
           ( ClientError
               ( Object
                   ( fromList
                       [
                           ( "code"
                           , String "utxo_too_small"
                           )
                       ,
                           ( "message"
                           , String "One of the outputs you've specified has an ada quantity that is below the minimum required. Either increase the ada quantity to at least the minimum, or specify an ada quantity of zero, in which case the wallet will automatically assign the correct minimum ada quantity to the output. Destination address: 0179aff1...8e21f45f Required minimum ada quantity: 0.995610 Specified ada quantity: 0.978370  "
                           )
                       ]
                   )
               )
           )
       2. we should be able to send minUTxOValue
       expected: Status {
                   statusCode = 202,
                   statusMessage = "Accepted"
                 }
        but got: Status {
                   statusCode = 403,
                   statusMessage = "Forbidden"
                 }

  To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/TRANS_MIN_UTXO_01b - I can spend exactly minUTxOValue/"

but succeeds with #3423.
Change 2-space indentation to 4-space indentation.
@Anviking Anviking force-pushed the anviking/min-utxo-integration-test branch from 1d570b4 to 72dea5e Compare August 10, 2022 08:26
@Anviking
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 10, 2022
3426: Add integration test for Babbage minUTxOValue validation precision r=Anviking a=Anviking

- [x] Verify that #3423 successfully alleviates problem where the wallet used to be 0.01724 ada too strict compared to the ledger when validating the minUTxOValue requirement of user-specified outputs.
    - Test fails without the fix.

### Comments


### Issue Number

https://input-output.atlassian.net/browse/ADP-1978

Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 10, 2022

Build failed:

  src/Test/Integration/Scenario/API/Shelley/StakePools.hs:1042:17: 
  1) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_LIST_01 - List stake pools, pools have the correct retirement information
       Waited longer than 90s to resolve action: "pools have the correct retirement information".
       expected: fromList [Nothing, Just EpochNo {
                   unEpochNo = 100000
                 }, Just EpochNo {
                   unEpochNo = 1000000
                 }]
        but got: fromList [Nothing, Just EpochNo {
                   unEpochNo = 1000000
                 }]

  To rerun use: --match "/API Specifications/SHELLEY_STAKE_POOLS/STAKE_POOLS_LIST_01 - List stake pools/pools have the correct retirement information/"

Randomized with seed 594490807

Seems like a variant of:
#3440
but with smaller consequences when one of the "retire far in the future" pools ended up having its retirement cert lost.

@Anviking
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 10, 2022
3426: Add integration test for Babbage minUTxOValue validation precision r=Anviking a=Anviking

- [x] Verify that #3423 successfully alleviates problem where the wallet used to be 0.01724 ada too strict compared to the ledger when validating the minUTxOValue requirement of user-specified outputs.
    - Test fails without the fix.

### Comments


### Issue Number

https://input-output.atlassian.net/browse/ADP-1978

3428: Bump cardano-node to 1.35.3-rc1 r=Anviking a=sevanspowell

- Bump dependencies:
  - cardano-node to 1.35.3-rc1
  - hw-aeson to match cardano-node
  - hedgehog-extras to match cardano-node
- Update compatibility matrix.

### Comments

Changelogs:

- [API](https://github.com/input-output-hk/cardano-node/blob/release/1.35/cardano-api/ChangeLog.md)
- [CLI](https://github.com/input-output-hk/cardano-node/blob/release/1.35/cardano-cli/ChangeLog.md)
- [Node](https://github.com/input-output-hk/cardano-node/blob/release/1.35/cardano-node/ChangeLog.md)

- [Compare v1.35.3-testnetonly and v1.35.3-rc1](IntersectMBO/cardano-node@1.35.3-testnetonly...1.35.3-rc1)

### Issue Number

ADP-2123


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
Co-authored-by: Samuel Evans-Powell <mail@sevanspowell.net>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 10, 2022

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"Required status check \"docs\" is expected.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@Anviking Anviking force-pushed the anviking/min-utxo-integration-test branch from 72dea5e to b080972 Compare August 10, 2022 13:10
@Anviking
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 10, 2022
3426: Add integration test for Babbage minUTxOValue validation precision r=Anviking a=Anviking

- [x] Verify that #3423 successfully alleviates problem where the wallet used to be 0.01724 ada too strict compared to the ledger when validating the minUTxOValue requirement of user-specified outputs.
    - Test fails without the fix.

### Comments


### Issue Number

https://input-output.atlassian.net/browse/ADP-1978

Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 10, 2022

Build failed:

🚨 Error: The command was interrupted by a signal

#3442

@Anviking
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 10, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 18a9316 into master Aug 10, 2022
@iohk-bors iohk-bors bot deleted the anviking/min-utxo-integration-test branch August 10, 2022 14:56
WilliamKingNoel-Bot pushed a commit that referenced this pull request Aug 10, 2022
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.

2 participants