Interpret fee rates in reference fee unit #21
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JBetz
force-pushed
the
value-type
branch
2 times, most recently
from
0deda5f
to
4771787
Compare
JBetz
commented
Apr 24, 2024
| CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinControl& coin_control, FeeCalculation* feeCalc) | ||
| { | ||
| return GetMinimumFeeRate(wallet, coin_control, feeCalc).GetFee(nTxBytes); | ||
| return GetMinimumFeeRate(wallet, coin_control, feeCalc).GetFee(nTxBytes, coin_control.m_fee_asset.value_or(::policyAsset)); |
There was a problem hiding this comment.
GetMinimumFeeRate is where mintxfee, paytxfee, and
blockmintxfee are used to negotiate a suitable fee rate for the transaction, and passing the asset parameter here ensures that the result is denominated in the fee asset and not RFU.
JBetz
commented
Apr 24, 2024
| @@ -926,7 +928,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) | |||
|
|
|||
| // No transactions are allowed below minRelayTxFee except from disconnected | |||
| // blocks | |||
| if (!bypass_limits && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; | |||
| if (!bypass_limits && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, feeAsset, state)) return false; | |||
There was a problem hiding this comment.
CheckFeeRate is where incrementalrelayfee is applied.
JBetz
commented
Apr 24, 2024
| @@ -53,7 +53,7 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFeeIn) | |||
| nSize += (32 + 4 + 1 + 107 + 4); // the 148 mentioned above | |||
| } | |||
|
|
|||
| return dustRelayFeeIn.GetFee(nSize); | |||
| return dustRelayFeeIn.GetFee(nSize, txout.nAsset.GetAsset()); | |||
There was a problem hiding this comment.
And finally, dustrelayfee. Here it's used exclusively for non-fee outputs, but still needs to be converted using exchange rates to check that the output doesn't cost more in fees than it's worth.
There was a problem hiding this comment.
Actually, this shouldn't be using the exchange rates at all.
If there's a transaction with an output of some asset that the node doesn't have an exchange rate for, then there's no meaningful conversion from the fee rate to an amount using exchange rates. So we should probably just treat dustrelayfee as a lower bound on the amount that doesn't have anything to do with the RFU.
Otherwise, the node will reject non-fee outputs as dust just because it doesn't care about the asset, which is obviously wrong.
There was a problem hiding this comment.
Did some more investigating and realized that dust checks are currently limited to outputs that have the same asset as the fee, so there's no risk of different assets being compared.
This applies to in policy: https://github.com/ElementsProject/elements/blob/master/src/policy/policy.cpp#L149
And to wallet: https://github.com/ElementsProject/elements/blob/master/src/wallet/spend.cpp#L1143 and https://github.com/ElementsProject/elements/blob/master/src/wallet/spend.cpp#L1559
JBetz
changed the title
This reverts commit ab3a67b.
Mixa84
previously approved these changes
May 3, 2024
Mixa84
approved these changes
May 3, 2024
JBetz
added a commit
that referenced
this pull request
May 3, 2024
* Introduce CValue for distinguishing amounts from values * Remove random character * Define insertion operator for CValue * Move CValue to separate module * Remove consensus changes * Remove CValue usage for fee rates, restrict to mempool * More cleanup * Use CValue for descendant and ancestor fee updates * More mempool fixes * Remove unused include * Fix variable shadowing * More conversions * Factor in fee asset when validating package fees * More conversions * Convert dust relay fee from RFU * More cleanup * Cleanup * Don't factor in asset in when calculating dust threshold * Convert package fees * Return CValue from CalculateExchangeValue * Use CValue for reverse exchange rate conversion * Update method documentation * Rename currency conversion methods * Fix typo * Add justification for RFU to comments * Use accessor instead of public instance variable unpacking CValue * Use GetValue() in fee rate conversion * Only apply dust check to outputs denominated in same asset as fee * Interpret coin selection effective feerate in RFU * Change GetModifiedFee() method to use RFU for summing with ancestor and descendant transactions * Add functional test for "fee_rate" parameter in fundrawtransaction RPC and -mintxfee node configuration parameter * Normalize fees to RFU during fee estimation * Lint fixes * More linting fixes * More linting * Apply method renams to fee amount conversions * Add any asset fee rates test to test runner * More amount conversion fixes * Add compiler switch for currency constants * Add new configuration flag to documentation * Apply fixes to fee estimation functional test from ElementsProject/elements#1298 * Add tests for paytxfee parameter * Cleanup * Fix compiler flag * Add tests for blockmintxfee * Remove whitespace * Add comments clarifying what's being demonstrated * Remove nFeeValue, recompute as needed * Revert "Remove nFeeValue, recompute as needed" This reverts commit ab3a67b. * Move No Coin configuration documentation to separate line * Add constant for full name of currency atom * Fix constant reference * Fix typo
JBetz
added a commit
that referenced
this pull request
May 3, 2024
* Introduce CValue for distinguishing amounts from values * Remove random character * Define insertion operator for CValue * Move CValue to separate module * Remove consensus changes * Remove CValue usage for fee rates, restrict to mempool * More cleanup * Use CValue for descendant and ancestor fee updates * More mempool fixes * Remove unused include * Fix variable shadowing * More conversions * Factor in fee asset when validating package fees * More conversions * Convert dust relay fee from RFU * More cleanup * Cleanup * Don't factor in asset in when calculating dust threshold * Convert package fees * Return CValue from CalculateExchangeValue * Use CValue for reverse exchange rate conversion * Update method documentation * Rename currency conversion methods * Fix typo * Add justification for RFU to comments * Use accessor instead of public instance variable unpacking CValue * Use GetValue() in fee rate conversion * Only apply dust check to outputs denominated in same asset as fee * Interpret coin selection effective feerate in RFU * Change GetModifiedFee() method to use RFU for summing with ancestor and descendant transactions * Add functional test for "fee_rate" parameter in fundrawtransaction RPC and -mintxfee node configuration parameter * Normalize fees to RFU during fee estimation * Lint fixes * More linting fixes * More linting * Apply method renams to fee amount conversions * Add any asset fee rates test to test runner * More amount conversion fixes * Add compiler switch for currency constants * Add new configuration flag to documentation * Apply fixes to fee estimation functional test from ElementsProject/elements#1298 * Add tests for paytxfee parameter * Cleanup * Fix compiler flag * Add tests for blockmintxfee * Remove whitespace * Add comments clarifying what's being demonstrated * Remove nFeeValue, recompute as needed * Revert "Remove nFeeValue, recompute as needed" This reverts commit ab3a67b. * Move No Coin configuration documentation to separate line * Add constant for full name of currency atom * Fix constant reference * Fix typo
Mixa84
pushed a commit
that referenced
this pull request
May 6, 2024
* Introduce CValue for distinguishing amounts from values * Remove random character * Define insertion operator for CValue * Move CValue to separate module * Remove consensus changes * Remove CValue usage for fee rates, restrict to mempool * More cleanup * Use CValue for descendant and ancestor fee updates * More mempool fixes * Remove unused include * Fix variable shadowing * More conversions * Factor in fee asset when validating package fees * More conversions * Convert dust relay fee from RFU * More cleanup * Cleanup * Don't factor in asset in when calculating dust threshold * Convert package fees * Return CValue from CalculateExchangeValue * Use CValue for reverse exchange rate conversion * Update method documentation * Rename currency conversion methods * Fix typo * Add justification for RFU to comments * Use accessor instead of public instance variable unpacking CValue * Use GetValue() in fee rate conversion * Only apply dust check to outputs denominated in same asset as fee * Interpret coin selection effective feerate in RFU * Change GetModifiedFee() method to use RFU for summing with ancestor and descendant transactions * Add functional test for "fee_rate" parameter in fundrawtransaction RPC and -mintxfee node configuration parameter * Normalize fees to RFU during fee estimation * Lint fixes * More linting fixes * More linting * Apply method renams to fee amount conversions * Add any asset fee rates test to test runner * More amount conversion fixes * Add compiler switch for currency constants * Add new configuration flag to documentation * Apply fixes to fee estimation functional test from ElementsProject/elements#1298 * Add tests for paytxfee parameter * Cleanup * Fix compiler flag * Add tests for blockmintxfee * Remove whitespace * Add comments clarifying what's being demonstrated * Remove nFeeValue, recompute as needed * Revert "Remove nFeeValue, recompute as needed" This reverts commit ab3a67b. * Move No Coin configuration documentation to separate line * Add constant for full name of currency atom * Fix constant reference * Fix typo
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additionally, this introduces a new type
CValueto distinguish amounts denominated in reference fee unit from actual assets. It's a rather fundamental change that may not be upstreamable, but already helped reveal some places where an amount was being treated as a value and vice-versa. If needed, we can always remove it later on once the any asset fee feature is more battle-tested. [Update: The use ofCValuehas been restricted to places that already needed to be updated for any asset fees, so it is no longer as an invasive of a change.]I've also added a compiler time variable named
ANY_ASSET_FEESfor configuring theCURRENCY_UNITandCURRENCY_ATOMconstants to beeRFUandrfainstead ofBTCandsat, respectively. Those two constants are used by RPC documentation for fee rates and represent the bulk of documentation changes we need for this feature.Node parameters:
RPC arguments:
fee_rateinfundrawtransactionInternal: