-
Notifications
You must be signed in to change notification settings - Fork 217
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
Reduce overestimation of minimum UTxO values for Shelley-era addresses. #3416
Conversation
88815d0
to
0224cd8
Compare
0224cd8
to
e9b2d29
Compare
6fa492d
to
cc8bf2e
Compare
bors try |
lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs
Outdated
Show resolved
Hide resolved
tryℹ️ Note that:
Build that originally failed: Build constituent: Log: Log extract:
Related issues: |
This is necessary to avoid a circular import.
This allows `dummyAddress` to be specified by the caller of coin selection instead of it being a hard-coded constant.
This renaming is intended to more accurately reflect the intended usage. A future commit will ensure that this field is initialized to an appropriate value.
4b78077
to
4de051d
Compare
This class provides a way to specify that addresses are bounded in length for a particular key type. In principle, for a given key type, the set of valid address sizes is bounded in both directions: there will exist both a minimum and maximum length. In practice, for now, we only define the `maxLengthAddressFor` function, which returns an address of maximum length for the given key. Later on, if needed, we could add a `minLengthAddressFor` function to specify minimum length addresses. Co-Authored-By: Johannes Lund <[email protected]>
This commit adds the `BoundedAddressLength` constraint to all functions that will need access to address length bounds within `Cardano.Wallet` and `Cardano.Wallet.Api.Server`.
This commit reduces the overestimation of minimum UTxO values for outputs with Shelley-era addresses. It makes the following changes: - Adds an `Address` parameter to the following functions: - `Shelley.MinimumUTxO.computeMinimumCoinForUTxO` - `CoinSelection.SelectionConstraints.computeMinimumAdaQuantity` - `CoinSelection.Internal.SelectionConstraints.computeMinimumAdaQuantity` - `CoinSelection.Internal.Balance.SelectionConstraints.computeMinimumAdaQuantity` - `Primitive.Types.Tx.TxConstraints.txOutputMinimumAdaQuantity` - Adds a `maximumLengthChangeAddress` parameter to the following types: - `CoinSelection.SelectionConstraints` - `CoinSelection.Internal.SelectionConstraints` - `CoinSelection.Internal.Balance.SelectionConstraints` - Updates `Cardano.Wallet` to initialize the `maximumLengthChangeAddress` field with a value that is suitable for the current wallet key type (made available by the `BoundedLengthAddress` class constraint). - Updates `CoinSelection` modules to use the given `maximumLengthChangeAddress` parameter whenever calculating minimum ada quantities for auto-generated change outputs. - Updates `CoinSelection` modules to use real user-specified addresses whenever calculating minimum ada quantities for user-specified outputs. - Updates the Swagger API description of the `minimum_utxo_value` field with a more comprehensive description of this field's meaning, as well as a short discussion of its history. (This field should ideally be deprecated, as the protocol parameter it used to represent no longer exists.) - Updates the `Wallet.Api.Types.toApiNetworkParameters` function to return the most appropriate value for `minimum_utxo_value` that we can, given that there is no single ideal value that we can return here. (This field should ideally be deprecated, as the protocol parameter it used to represent no longer exists.) - Adds an `Address.Constants` module with a global `maxLengthAddress` constant, defined to be an address that is maximal in length when compared to the maximum length addresses for all wallet key types. - Updates the migration algorithm to use the global `maxLengthAddress` constant when computing minimum ada quantities for all generated outputs. - Extends the set of golden minimum ada quantities within `MinimumUTxOSpec` so that both Byron-era and Shelley-era addresses are covered.
At the time `SelectionSpec` was originally written, there was no reusable function available for choosing `Coin` values from a range, so we defined the `genCoinRange` function for this purpose. But now we have a standard `chooseCoin` function, we can use that to replace function `genCoinRange` within `SelectionSpec`.
We do need to use a valid address here, since the `performSelectionNonEmpty` function currently needs a valid address in order to compute and validate minimum ada quantities. However, since the dummy output is always eventually thrown away, we should use the shortest possible valid address in order to minimize any overestimation of the required cost.
…en tests. In the case of delegation, the wallet typically calls coin selection without any user-specified outputs. In such cases, coin selection will use a temporary dummy output to act as a target for change generation, defined within the `performSelectionEmpty` function. Although we have adjusted this function to use a valid address of minimal length, this address is still longer than the null-length dummy address that we were able to use when the `computeMinimumCoinForUTxO` function did not require an address. This increase in address length causes an increase in the estimated cost when generating change. Therefore, any `UnableToConstructChange` errors will report ada shortfalls that are larger in magnitude than before. A future commit will adjust `performSelectionEmpty` so that it can use a null-length address again, so we can minimize the cost overestimation.
In the case of delegation, the wallet typically calls coin selection without any user-specified outputs. In such cases, coin selection will use a temporary dummy output to act as a target for change generation, defined within the `performSelectionEmpty` function. Although we have adjusted this function to use a valid address of minimal length, this address is still longer than the null-length dummy address that we were able to use when the `computeMinimumCoinForUTxO` function did not require an address. This increase in address length causes an increase in the estimated cost when generating change. Therefore, this also raises the cost of joining or quitting a stake pool. A future commit will adjust `performSelectionEmpty` so that it can use a null-length address again, so we can minimize the cost overestimation.
… tests. This allows us to revise the error message without causing multiple integration tests to fail unnecessarily.
…Error`. It's advantageous to perform all validation of user-specified outputs within the top-level `CoinSelection.Internal` module, rather than deferring some parts of the validation process to `CoinSelection.Internal.Balance`. Justification: Performing validation of user-specified outputs within the top-level coin selection module means that we don't have to do it again within lower-level internal modules. Goal: We can eventually relax the restriction requiring us to always provide valid addresses when calling `performSelectionNonEmpty`, which also means we can eventually use a null-length address for the dummy output within `performSelectionEmpty`.
…ernal`. As a result of this commit, verification is now performed in two places: - `CoinSelection.Internal` - `CoinSelection.Internal.Balance` The second verification step, within the `Balance` module, is now redundant, and should never fail in production. We will remove this redundant step from the `Balance` module in a later commit.
…alues`. This commit removes the now-failing coverage check for `CoinSelection.Internal.Balance.InsufficientMinCoinValues` from `CoinSelection.InternalSpec`. This coverage check can no longer fire, as insufficient ada quantities are now detected at the top level within the `prepareOutputsInternal` function of module `CoinSelection.Internal`. Therefore, if we call coin selection through `CoinSelection.Internal`, it should no longer possible to trigger `Balance.InsufficientMinCoinValues`. A later commit will remove `Balance.InsufficientMinCoinValues` entirely, in favour of using `SelectionOutputErrorInsufficientCoinError` from `CoinSelection.Internal`.
We repurpose the existing verification function, and make it target values of type `SelectionOutputCoinInsufficientError` instead of the soon-to-be-removed `InsufficientMinCoinValues` type from `Balance`.
…ror`. Now that coin selection returns `SelectionOutputCoinInsufficientError` in preference to the soon-to-be-deleted `InsufficientMinCoinValues` error, we must adjust the coverage checks within `TransactionSpec` to expect the new error type instead of the old.
This error has now been completely superseded by `SelectionOutputCoinInsufficientError`.
…ty`. Now that `performSelectionNonEmpty` no longer needs to validate the minimum ada quantities of user-specified outputs (a responsibility that has been moved to the top-level coin selection module), we are free to choose any `Address` (even if invalid) and any non-zero `Coin` value for the dummy output used by `performSelectionEmpty`. In order to minimize any overestimation in cost resulting from the use of a dummy output, we choose the shortest possible `Address` (which is a null-length address) and the smallest possible non-zero `Coin` value.
4de051d
to
3bfe1bb
Compare
…en tests. The previous commit adjusted the dummy output used by function `performSelectionEmpty` so that it uses a null-length `Address` value. This null-length `Address` value is identical to the one we used before we adjusted `computeMinimumCoinForUTxO` to require a valid `Address`. However, we were also able to adjust `performSelectionNonEmpty` so that it no longer needs to validate minimum ada quantities of user-specified outputs, and consequently we were able to reduce the dummy output's `Coin` value to just `Coin 1`, which has the smallest space cost of any non-zero `Coin` value. This results in a further reduction in cost overestimation while constructing change, which results in smaller `shortfall` values within `UnableToConstructChange` errors.
A previous commit adjusted the dummy output used by function `performSelectionEmpty` so that it uses a null-length `Address` value. This null-length `Address` value is identical to the one we used before we adjusted `computeMinimumCoinForUTxO` to require a valid `Address`. However, we were also able to adjust `performSelectionNonEmpty` so that it no longer needs to validate minimum ada quantities of user-specified outputs, and consequently we were able to reduce the dummy output's `Coin` value to just `Coin 1`, which has the smallest space cost of any non-zero `Coin` value. This results in a further reduction in cost overestimation while constructing change, which results in a lower cost when joining and quitting stake pools.
This refactoring adjusts type `SelectionOutputCoinInsufficientError` so that it takes a `ctx` parameter, rather than an `Address ctx` parameter. As a result, all `SelectionOutputError` constructors now use the same style of parameterisation: ``` data SelectionOutputError ctx = SelectionOutputCoinInsufficient (SelectionOutputCoinInsufficientError ctx) | SelectionOutputSizeExceedsLimit (SelectionOutputSizeExceedsLimitError ctx) | SelectionOutputTokenQuantityExceedsLimit (SelectionOutputTokenQuantityExceedsLimitError ctx) ```
3bfe1bb
to
cbc2c20
Compare
Therefore, the value reported by this field should only be viewed | ||
as an absolute minimum, and only applies to outputs that send ada | ||
(and no other assets) to Shelley-era addresses. If an output | ||
contains other assets, specifies a datum hash, or sends funds to a | ||
Byron-era address, then the minimum value required by the ledger | ||
(and the wallet) will be higher than the value reported by this | ||
field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
txOutputMinimumAdaQuantity txConstraints
(AD.maxLengthAddressFor $ Proxy @ShelleyKey)
TokenMap.empty
this seems either incorrect or misleading. It's not the minimum value: minLengthAddress
would yield a smaller result. I think we should either
- Replace
AD.maxLengthAddressFor
withminLengthAddress
- Reword this paragraph.
(And arguably the 4*44 lovelace overestimation of the requirement for coin values (below 4295 ada) is also at odds with this value being an "absolute minimum", though only when considering the broader context outside of the wallet API.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Anviking, I think you've raised a good point.
I'll have a think about what we should do here, and make a PR to either adjust the comment, or adjust the definition in the way that you suggest.
I think the intention of the comment is that "If you're sending ada (and no other assets) to a Shelley address, then sending this amount of ada or more should always work when creating transactions with the wallet". So in this sense, it's the minimum value that we can safely state will always be fine for all Shelley addresses, when using the wallet.
If we were to alter the definition to use minLengthAddress
, but then the user specifies a Shelley address that is longer, then their payment would be rejected (if they had chosen to pay the value returned by this field). I would argue that this would make the min_utxo_value
field even less useful that it already is.
As you know, I think we should mark this field as "deprecated" as soon as possible, since it's quite hard for the user to interpret the meaning (and of course, this protocol parameter no longer exists in the Alonzo and Babbage eras).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Anviking I've added a reminder to address this comment here: https://input-output.atlassian.net/browse/ADP-2039.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're sending ada (and no other assets) to a Shelley address, then sending this amount of ada or more should always work when creating transactions with the wallet
I think that's fair, but this is what should be said in the docs then. So e.g. something more like:
In Babbage the minimum, even for ada-only outputs, is not constant. It depends upon the serialised length of the output, including address and ada quantity. What is returned by this field is a value which works for sending ada to any Shelley address. The true minimum may be marginally smaller with short addresses, or bigger with longer Byron addresses or included tokens. The field may be deprecated or removed in future versions of the wallet.
I think we should mark this field as "deprecated" as soon as possible, since it's quite hard for the user to interpret the meaning
I do agree 💯
the minimum ada quantity for an output was determined by | ||
multiplying the `coinsPerUTxOWord` parameter by the length (in | ||
8-byte words) of the in-memory representation of that output, which | ||
was not dependent on the length of the address. Therefore, in this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of the in-memory representation of that output, which was not dependent on the length of the address.
Are you sure this is entirely correct? I thought it was an upper bound of the in-memory representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of the in-memory representation of that output, which was not dependent on the length of the address.
Are you sure this is entirely correct?
This was my understanding, after previous discussions with the ledger team.
I thought it was an upper bound of the in-memory representation.
This would indeed explain why the address length does not affect the Alonzo-era minimum UTxO value computation.
I'll double-check with the ledger team and/or verify this statement by looking at the source code.
If it turns out that this description is inaccurate, I'll make another PR to fix it. 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Anviking I've added a reminder to address this comment here: https://input-output.atlassian.net/browse/ADP-2039.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🎉 modulo in particular #3416 (comment)
bors r+ |
3416: Reduce overestimation of minimum UTxO values for Shelley-era addresses. r=jonathanknowles a=jonathanknowles ### Issue Number ADP-2039 ### Summary This PR reduces our overestimation of minimum UTxO values for outputs with Shelley-era addresses. ### Details - When computing minimum ada quantities for _**user-specified outputs**_, coin selection now uses the _actual addresses specified by the user_. - When computing minimum ada quantities for _**auto-generated change outputs**_, coin selection now uses a _maximum length change address_ that is specific to the current wallet key type. - This allows us to use a smaller overestimation in the case of Shelley-era addresses, as the longest Shelley-era address is shorter than the longest Byron-era address. Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Johannes Lund <[email protected]>
Build failed:
|
bors r+ |
Build succeeded: |
Issue Number
ADP-2039
Summary
This PR reduces our overestimation of minimum UTxO values for outputs with Shelley-era addresses.
Details