Allow coin-selection to carry an extra "reserve input" #1843
Conversation
KtorZ
added
the
ADDING FEATURE
| it "1561 - The fee balancing algorithm converges for any coin selection." | ||
| $ property | ||
| $ withMaxSuccess 10000 | ||
| $ forAllBlind (genSelection @'Mainnet @ByronKey) prop_rebalanceChangeOutputs |
| newTransactionLayer @'Mainnet @ByronKey Proxy mainnetMagic | ||
| feePolicy = | ||
| LinearFee (Quantity 155381) (Quantity 43) (Quantity 0) | ||
|
|
| @@ -48,6 +48,8 @@ data CoinSelection = CoinSelection | |||
| -- ^ Picked outputs | |||
| , change :: [Coin] | |||
| -- ^ Resulting changes | |||
| , reserve :: Maybe Coin | |||
There was a problem hiding this comment.
it can be only "one" ? not list?
There was a problem hiding this comment.
See comments above. In theory, we could also have deposits here. So I am thinking about having a list of Reserve where Reserve = Withdrawal | Deposit
There was a problem hiding this comment.
Will Withdraw and Deposit, if introduced will change on this level? Will there be different conditions in rebalanceSelection for them? for example Withdraw will deplete and transform into change at first, and only then Deposit? Or other rules?
There was a problem hiding this comment.
No, they'll have the exact same effect hence why I initially went for a simple Maybe Coin. Perhaps it's not worth abstracting further here, in the end, both the deposit and withdrawal can be represent as a sum for this reserve.
The difference lies in how the transaction is then constructed / signed / serialized. Withdrawals require a special entry as .. "withdrawals" in the transaction, as well as a special witness.
Whereas deposit are implicit, when de-registering a stake key.
| -- selection is now balanced, nothing to do. | ||
| | φ_original == δ_original = | ||
| (s, Fee 0) | ||
|
|
||
| -- some fee left to pay, and the reserve is non-empty, use it first. | ||
| | φ_original > δ_original && reserve s > Just (Coin 0) = |
| -- Safe because of the above guard. | ||
| Just (Coin r) = reserve s | ||
| r' = if r > φ_original | ||
| then Coin (r - φ_original) |
There was a problem hiding this comment.
it was supposed to turn all reserve (if there is left) into change. So rebalanceSelection will be called in with decreased reserve and some branch will decrease it to zero and after that it will return in senderPaysFee with reserve (Just (Coin 0)). Is that right?
There was a problem hiding this comment.
Hence the added check in the corresponding property checking that the reserve is always 0 at the end of the balancing.
| & classify reserveLargerThanFee | ||
| "reserve > fee" | ||
| & classify feeLargerThanDelta | ||
| "fee > delta" |
There was a problem hiding this comment.
any chance to test ErrValidateSelection here or maybe at integration testing/backend TransactionSpecs ? 🤔
There was a problem hiding this comment.
In the fee spec, no chance, and in integration also quite hard. This is actually a server error (500), so something that we shouldn't be able to trigger under any circumstances, in theory.
KtorZ
force-pushed
the
KtorZ/1821/reserve-coin-selection
branch
from
6d367c0 to
6d33031
Compare
| , withdrawal :: Word64 | ||
| -- ^ An available withdrawal amount, counting as an extra input | ||
| , reclaim :: Word64 | ||
| -- ^ Claim back a deposit, counting as a an extra input |
| -> Either ErrMkTx (Tx, SealedTx) | ||
| -- ^ Construct a transaction containing a certificate for quiting from | ||
| -- a stake pool. | ||
| -- | ||
| -- The certificate is the public key of the reward account. | ||
|
|
||
| , minimumFee :: FeePolicy -> [Certificate] -> CoinSelection -> Fee | ||
| , initDelegationSelection |
| genWithdrawal :: Gen Word64 | ||
| genWithdrawal = frequency | ||
| [ (3, pure 0) | ||
| , (1, oneof |
There was a problem hiding this comment.
To favor some edge-cases a bit more in the generator. The type is a Word64, so something like arbitrary will span over the entire domain. Yet, we know that bugs are usually found at edges, so I've favored small values, close to the fee amount so see how it behaves when a bit smaller than the fee, or a bit bigger, and still, allowed for big values far from this edge.
There was a problem hiding this comment.
A lot of changes in the code due to two ideas implemented (not all was able to follow, but I bet the author mainly followed compiler here). When reviewing focused on the ideas, ie. 1. extension of CoinSelection to account for deposit, withdrawal and reclaim, 2. changing interface in Cardano.Wallet.Transaction - and here using CoinSelection in number of methods instead of inputs/outputs and introducing initDelegationSelection to handle initial selection for different backends, 3. testing rebalancingSelection in unit tests. All three point looks solid to me!
… be used as input For now, the reserve is always 'Nothing', which should not alter any of the existing behavior. Then, it can be set to a specific value (e.g. the reward balance) to be used on the left side of the balance (i.e. the input side).
…ing from the reserve The approach is still very 'simple', before trying to deplete any change output, we first try to remove fees from the reserve. Once the reserve is empty, we'll start depleting change outputs.
It was originally in `-byron` because it was using the fee policy and fee estimation from Byron, so, while moving it, I also changed the way the transaction size is estimated to make it mimics the way it's done on shelley and byron, with rather realistic fee values.
…d assertions to properties
Everything in the past coin selection (without reserve) was easy to reason about. One could always look at the sum of inputs and compare it with the sum of outputs and changes, and there were some nice properties about this. Having mutable reserve makes it somewhat very confusing because, there's no trace of where the money comes from (turning reserve into changes shouldn't make it disappear).
So that types are clear from the coin selection and a bit less confusing.
This in order to defer to the relevant backend the necessary bits of logic regarding withdrawal, deposits and reclaims. I've also removed the 'WalletDelegation' from 'mkDelegationJoinTx' as this can now be inferred from the presence of a deposit in the coin selection. We have the following situation: object | cardano-node | jormungandr ---- | ---- | ---- inputs | explicit | explicit reclaim | implicit | N/A withdrawal | explicit | N/A --- | --- | --- outputs | explicit | explicit deposit | implicit | N/A Regardless of the situation, we must always have at least one input (replay protection) and output (although outputs may be entirely change outputs). Then, depending on the situation, we may have 'withdrawal', 'reclaim' and or 'deposit' present. `reclaim` and `deposit` are "opposed" and can't be present together. `withdrawal` can in theory be present with both, but in practice, will only be present alone: we'll only allow users to withdraw their reward as part of standard transaction. `deposit` are only present when registering a stake-key for the first time. In summary, this gives us: type of transaction | objects ------------------- | -------- standard transaction | inputs, outputs, withdrawal? (if users said so) initial delegation | inputs, outputs, deposit re-delegation | inputs, outputs de-registration | inputs, outputs, reclaim
…re's already a UTxO input
The generator was good for 'payment' kind of selection. Yet, we also sort of abuse the fee balancing when creating selection for delegation with special delegation with no inputs and no outputs. This was therefore completely untested by the current generator! Now it does, sometimes, create typical selections we would create when delegating or undelegating. It nicely caught an error in the fee balancing algorithm which this commit also fixes
In the end, it is much simpler and cleaner, no more hacky-hack to get things working. The newly introduced 'initDelegationSelection' makes it easier to construct a correct coin selection from the start and simply go with it.
We have two different ways of computing fees, and this is because in Byron, it used to be complicated to evaluate fees for a transaction. Now, this fee estimator feels sort of redundant and we should solely rely on the result from the fee estimation endpoint. In the meantime, I've adjusted it to make a bit more consistent with how fees are calculated elsewhere. I've also adjusted the genesis file to have a much bigger deposit key. The previous code was actually handling things in a very wrong way, but it went unnoticed because two errors were cancelling each others: fee were slightly over-evaluated, and the deposit for key was small enough to compensate.
We could make it slightly smarter here and requires that the withdrawal is only requested when the balance is somewhat sufficient. Sufficient would mean, bigger than the cost of adding it. Indeed, adding a withdrawal would only make sense if the cost of adding it isn't greater to its value.
Otherwise, we end-up paying for more than the value they offer.
…action I've once again changed a bit the transaction layer to be more aligned with the recent changes (now takes a full coin selection). Also, it now requires a reward account to be given; this is relatively 'unsound' for Byron and Icarus wallet since they have no reward account, yet, the account is only used when a non-null withdrawal is provided which should never happen for these wallet types since they aren't receiving rewards at all.
KtorZ
force-pushed
the
KtorZ/1821/reserve-coin-selection
branch
from
f738fe0 to
10e04f4
Compare
|
bors r+ |
Build succeeded |
Issue Number
#1821
Overview
859f53b
📍 extend 'CoinSelection' type to carry an extra reserve amount that can be used as input
For now, the reserve is always 'Nothing', which should not alter any of the existing behavior. Then, it can be
set to a specific value (e.g. the reward balance) to be used on the left side of the balance (i.e. the input side).
eebc981
📍 rename 'rebalanceChangeOutput' in 'rebalanceSelection' and allow picking from the reserve
The approach is still very 'simple', before trying to deplete any change output, we first try to remove fees
from the reserve. Once the reserve is empty, we'll start depleting change outputs.
08a6cbe
📍 move prop_rebalanceSelection from byron package to core package
It was originally in
-byronbecause it was using the fee policy andfee estimation from Byron, so, while moving it, I also changed the way
the transaction size is estimated to make it mimics the way it's done on
shelley and byron, with rather realistic fee values.
338dde7
📍 generate arbitrary selection with reserve, add more classification and assertions to properties
20c161b
📍 do not mutate the reserve as part of the fee balancing process
Everything in the past coin selection (without reserve) was easy to reason about. One could always look at the sum
of inputs and compare it with the sum of outputs and changes, and there were some nice properties about this. Having
mutable reserve makes it somewhat very confusing because, there's no trace of where the money comes from (turning
reserve into changes shouldn't make it disappear).
22e1714
📍 expand 'reserve' to also cover deposit and reclaim
So that types are clear from the coin selection and a bit less confusing.
43cace2
📍 extend transaction layer with an 'initDelegationSelection'
This in order to defer to the relevant backend the necessary bits of logic regarding withdrawal, deposits and reclaims.
I've also removed the 'WalletDelegation' from 'mkDelegationJoinTx' as this can now be inferred from the presence of a deposit in the coin selection.
We have the following situation:
Regardless of the situation, we must always have at least one input (replay protection) and output (although outputs may be entirely change outputs).
Then, depending on the situation, we may have 'withdrawal', 'reclaim' and or 'deposit' present.
reclaimanddepositare "opposed" and can't be present together.withdrawalcan in theory be present with both, but in practice, will only be present alone: we'll onlyallow users to withdraw their reward as part of standard transaction.
depositare only present when registering a stake-key for the first time. In summary, this gives us:227dcd9
📍 only count reclaim and withdrawal as part of the input balance if there's already a UTxO input
dd00306
📍 tweak arbitrary coin selection generator to produce more realistic cases
The generator was good for 'payment' kind of selection. Yet, we also sort of abuse the fee balancing when creating selection for delegation with special delegation
with no inputs and no outputs. This was therefore completely untested by the current generator! Now it does, sometimes, create typical selections we would create when
delegating or undelegating. It nicely caught an error in the fee balancing algorithm which this commit also fixes
523ca66
📍 re-implement shelley's transaction layer accordingly
In the end, it is much simpler and cleaner, no more hacky-hack to get things working. The newly introduced 'initDelegationSelection' makes it easier to construct a correct coin selection from the start and simply go with it.
7fa1162
📍 review integration 'feeEstimator' to use 'DelegationAction'
We have two different ways of computing fees, and this is because in Byron, it used to be complicated to evaluate fees for a transaction. Now, this fee estimator feels sort of redundant and we should solely rely on the result from the fee estimation endpoint. In the meantime, I've adjusted it to make a bit more consistent with how fees are calculated elsewhere.
I've also adjusted the genesis file to have a much bigger deposit key. The previous code was actually handling things in a very wrong way, but it went unnoticed because two errors were cancelling each others: fee were slightly over-evaluated, and the deposit for key was small enough to compensate.
f8faf55
📍 re-implement byron & jormungandr transaction layers accordingly
ff7ee22
📍 add withdrawal on each payment transaction
We could make it slightly smarter here and requires that the withdrawal is only requested when the balance is somewhat sufficient.
Sufficient would mean, bigger than the cost of adding it. Indeed, adding a withdrawal would only make sense if the cost of adding it
isn't greater to its value.
98ace73
📍 only set withdrawal when they are sufficiently big
Otherwise, we end-up paying for more than the value they offer.
d6b1db1
📍 remove now obsolete 'ErrChangeIsEmptyForRetirement'
ba174de
📍 prepare foundation to support withdrawals witnesses in standard transaction
I've once again changed a bit the transaction layer to be more aligned with the recent changes (now takes a full coin selection).
Also, it now requires a reward account to be given; this is relatively 'unsound' for Byron and Icarus wallet since they have no
reward account, yet, the account is only used when a non-null withdrawal is provided which should never happen for these wallet types
since they aren't receiving rewards at all.
e7ab562
📍 add property showing that withdrawals are correctly estimated by Shelley tx layer
3035ec7
📍 add integration test showing that rewards are spent with a transaction
10e04f4
📍 count rewards as part of the total and available balance
Comments
TODO: need to create the a corresponding withdrawal witness inside payment transactionsTODO: need to NOT add a withdrawal when it's below certain thresholdTODO: need to add an integration test showing that reward are withdrawn successfully.