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

Interpret fee rates in reference fee unit #21

Merged
merged 54 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
e1d24ef
Introduce CValue for distinguishing amounts from values
JBetz Apr 24, 2024
f31e9b0
Remove random character
JBetz Apr 24, 2024
c757bc3
Define insertion operator for CValue
JBetz Apr 24, 2024
6820fe5
Move CValue to separate module
JBetz Apr 24, 2024
4ed94c3
Remove consensus changes
JBetz Apr 24, 2024
5fcbcdc
Remove CValue usage for fee rates, restrict to mempool
JBetz Apr 24, 2024
6f59bad
More cleanup
JBetz Apr 24, 2024
4409eab
Use CValue for descendant and ancestor fee updates
JBetz Apr 24, 2024
182dfb5
More mempool fixes
JBetz Apr 24, 2024
61df423
Remove unused include
JBetz Apr 24, 2024
e5221f2
Fix variable shadowing
JBetz Apr 24, 2024
e14ef73
More conversions
JBetz Apr 24, 2024
02f827f
Factor in fee asset when validating package fees
JBetz Apr 24, 2024
70579cc
More conversions
JBetz Apr 24, 2024
0ff30fa
Convert dust relay fee from RFU
JBetz Apr 24, 2024
ecd598b
More cleanup
JBetz Apr 24, 2024
66fc881
Cleanup
JBetz Apr 24, 2024
9b10788
Don't factor in asset in when calculating dust threshold
JBetz Apr 25, 2024
eda394a
Convert package fees
JBetz Apr 25, 2024
61f3a20
Return CValue from CalculateExchangeValue
JBetz Apr 25, 2024
3e354d5
Use CValue for reverse exchange rate conversion
JBetz Apr 25, 2024
c6f7eac
Update method documentation
JBetz Apr 25, 2024
6829d00
Rename currency conversion methods
JBetz Apr 25, 2024
e66f4bf
Fix typo
JBetz Apr 25, 2024
2e12e9a
Add justification for RFU to comments
JBetz Apr 25, 2024
c2a58c2
Use accessor instead of public instance variable unpacking CValue
JBetz Apr 25, 2024
7177e3c
Use GetValue() in fee rate conversion
JBetz Apr 25, 2024
8843a32
Only apply dust check to outputs denominated in same asset as fee
JBetz Apr 30, 2024
3892304
Interpret coin selection effective feerate in RFU
JBetz Apr 30, 2024
30a1539
Change GetModifiedFee() method to use RFU for summing with ancestor a…
JBetz Apr 30, 2024
d50341f
Add functional test for "fee_rate" parameter in fundrawtransaction RP…
JBetz Apr 30, 2024
6665de7
Normalize fees to RFU during fee estimation
JBetz May 1, 2024
9e18f3a
Lint fixes
JBetz May 1, 2024
c12707c
More linting fixes
JBetz May 1, 2024
734d6bc
More linting
JBetz May 1, 2024
b2b32d0
Apply method renams to fee amount conversions
JBetz May 1, 2024
abbb3a0
Add any asset fee rates test to test runner
JBetz May 1, 2024
66879ed
More amount conversion fixes
JBetz May 1, 2024
3174b34
Add compiler switch for currency constants
JBetz May 2, 2024
29a4841
Add new configuration flag to documentation
JBetz May 2, 2024
bcf9e69
Apply fixes to fee estimation functional test from https://github.com…
JBetz May 2, 2024
de1eed5
Add tests for paytxfee parameter
JBetz May 2, 2024
5a747d3
Cleanup
JBetz May 2, 2024
d9d8114
Fix compiler flag
JBetz May 2, 2024
681fd41
Add tests for blockmintxfee
JBetz May 2, 2024
947aa98
Remove whitespace
JBetz May 2, 2024
b9d84a8
Add comments clarifying what's being demonstrated
JBetz May 2, 2024
ab3a67b
Remove nFeeValue, recompute as needed
JBetz May 2, 2024
543522e
Revert "Remove nFeeValue, recompute as needed"
JBetz May 3, 2024
0c95a59
Move No Coin configuration documentation to separate line
JBetz May 3, 2024
9c67d2e
Merge branch 'dev' into value-type
JBetz May 3, 2024
69b09d5
Add constant for full name of currency atom
JBetz May 3, 2024
a51d080
Fix constant reference
JBetz May 3, 2024
be4d954
Fix typo
JBetz May 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ To speed up the build if not necessary, disable bench and tests in configure:
```bash
./configure --without-gui --without-natpmp --without-miniupnpc --disable-bench --disable-tests
```
To configure RPC documentation to denominate fee rates using RFU and rfa instead of BTC and sat:
```bash
./configure --enable-any-asset-fees
```

Modes
-----
Expand Down Expand Up @@ -106,4 +110,3 @@ https://github.com/ElementsProject/elementsproject.github.io
Secure Reporting
------------------
See [our vulnerability reporting guide](SECURITY.md)

13 changes: 13 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,24 @@ AC_ARG_ENABLE([liquid],
[liquid_build=yes],
[liquid_build=no])

AC_ARG_ENABLE([any_asset_fees],
[AS_HELP_STRING([--enable-any-asset-fees],
[Enable build that uses constants for any asset fees feature])],
[any_asset_fees=yes],
[any_asset_fees=no])

if test "$use_asm" = "yes"; then
AC_DEFINE([USE_ASM], [1], [Define this symbol to build in assembly routines])
fi

if test "$liquid_build" = "yes"; then
AC_DEFINE(LIQUID, 1, [Define this symbol for Liquid builds])
fi

if test "$any_asset_fees" = "yes"; then
AC_DEFINE(ANY_ASSET_FEES, 1, [Define this symbol to modify constants for any asset fees feature])
fi

AC_ARG_ENABLE([zmq],
[AS_HELP_STRING([--disable-zmq],
[disable ZMQ notifications])],
Expand Down Expand Up @@ -1841,6 +1852,7 @@ AM_CONDITIONAL([WORDS_BIGENDIAN], [test "$ac_cv_c_bigendian" = "yes"])
AM_CONDITIONAL([USE_NATPMP], [test "$use_natpmp" = "yes"])
AM_CONDITIONAL([USE_UPNP], [test "$use_upnp" = "yes"])
AM_CONDITIONAL([LIQUID], [test "$liquid_build" = "yes"])
AM_CONDITIONAL([ANY_ASSET_FEES], [test "$any_asset_fees" = "yes"])

dnl for minisketch
AM_CONDITIONAL([ENABLE_CLMUL], [test "$enable_clmul" = "yes"])
Expand Down Expand Up @@ -1995,6 +2007,7 @@ echo " with upnp = $use_upnp"
echo " with natpmp = $use_natpmp"
echo " use asm = $use_asm"
echo " liquid_build = $liquid_build"
echo " any_asset_fees = $any_asset_fees"
echo " USDT tracing = $use_usdt"
echo " sanitizers = $use_sanitizers"
echo " debug enabled = $enable_debug"
Expand Down
1 change: 1 addition & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ BITCOIN_CORE_H = \
policy/policy.h \
policy/rbf.h \
policy/settings.h \
policy/value.h \
pow.h \
primitives/pak.h \
protocol.h \
Expand Down
26 changes: 21 additions & 5 deletions src/exchangerates.cpp
Original file line number Diff line number Diff line change
@@ -1,24 +1,40 @@
#include <assetsdir.h>
#include <exchangerates.h>
#include <policy/policy.h>
#include <policy/value.h>
#include <util/settings.h>
#include <util/system.h>
#include <univalue.h>

#include <fstream>

CAmount ExchangeRateMap::CalculateExchangeValue(const CAmount& amount, const CAsset& asset) {
CValue ExchangeRateMap::ConvertAmountToValue(const CAmount& amount, const CAsset& asset) {
int64_t int64_max = std::numeric_limits<int64_t>::max();
auto it = this->find(asset);
if (it == this->end()) {
return 0;
return CValue(0);
}
auto scaled_value = it->second.m_scaled_value;
__uint128_t value = ((__uint128_t)amount * (__uint128_t)scaled_value) / (__uint128_t)exchange_rate_scale;
__uint128_t result = ((__uint128_t)amount * (__uint128_t)scaled_value) / (__uint128_t)exchange_rate_scale;
if (result > int64_max) {
return CValue(int64_max);
} else {
return CValue((int64_t) result);
}
}

CAmount ExchangeRateMap::ConvertValueToAmount(const CValue& value, const CAsset& asset) {
int64_t int64_max = std::numeric_limits<int64_t>::max();
if (value > int64_max) {
auto it = this->find(asset);
if (it == this->end()) {
return int64_max;
}
auto scaled_value = it->second.m_scaled_value;
__uint128_t result = ((__uint128_t)value.GetValue() * (__uint128_t)exchange_rate_scale) / (__uint128_t)scaled_value;
if (result > int64_max) {
return int64_max;
} else {
return (int64_t) value;
return (int64_t) result;
}
}

Expand Down
22 changes: 16 additions & 6 deletions src/exchangerates.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@

#include <fs.h>
#include <policy/policy.h>
#include <policy/value.h>
#include <univalue.h>

constexpr const CAmount exchange_rate_scale = COIN;
constexpr const CAmount exchange_rate_scale = COIN; // 100,000,000
const std::string exchange_rates_config_file = "exchangerates.json";

class CAssetExchangeRate
Expand Down Expand Up @@ -33,15 +34,24 @@ class ExchangeRateMap : public std::map<CAsset, CAssetExchangeRate>
}

/**
* Calculate the exchange value
* Convert an amount denominated in some asset to the node's RFU (reference fee unit)
*
* @param[in] amount Corresponds to CTxMemPoolEntry.nFeeAmount
* @param[in] amount Corresponds to CTxMemPoolEntry.nFee
* @param[in] asset Corresponds to CTxMemPoolEntry.nFeeAsset
* @return the value at current exchange rate. Corresponds to CTxMemPoolEntry.nFee
* @return the value at current exchange rate. Corresponds to CTxMemPoolEntry.nFeeValue
*/
CAmount CalculateExchangeValue(const CAmount& amount, const CAsset& asset);
CValue ConvertAmountToValue(const CAmount& amount, const CAsset& asset);

/**
/**
* Convert an amount denominated in the node's RFU (reference fee unit) into some asset
*
* @param[in] value Corresponds to CTxMemPoolEntry.nFeeValue
* @param[in] asset Corresponds to CTxMemPoolEntry.nFeeAsset
* @return the amount at current exchange rate. Corresponds to CTxMemPoolEntry.nFee
*/
CAmount ConvertValueToAmount(const CValue& value, const CAsset& asset);

/**
* Load the exchange rate map from the default JSON config file in <datadir>/exchangerates.json.
*
* @param[in] errors Vector for storing error messages, if there are any.
Expand Down
4 changes: 2 additions & 2 deletions src/node/miner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -452,15 +452,15 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
assert(!inBlock.count(iter));

uint64_t packageSize = iter->GetSizeWithAncestors();
CAmount packageFees = iter->GetModFeesWithAncestors();
CValue packageFees = iter->GetModFeesWithAncestors();
int64_t packageSigOpsCost = iter->GetSigOpCostWithAncestors();
if (fUsingModified) {
packageSize = modit->nSizeWithAncestors;
packageFees = modit->nModFeesWithAncestors;
packageSigOpsCost = modit->nSigOpCostWithAncestors;
}

if (packageFees < blockMinFeeRate.GetFee(packageSize)) {
if (packageFees.GetValue() < blockMinFeeRate.GetFee(packageSize)) {
// Everything else we might consider has a lower fee rate
return;
}
Expand Down
6 changes: 3 additions & 3 deletions src/node/miner.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ struct CTxMemPoolModifiedEntry {

int64_t GetModifiedFee() const { return iter->GetModifiedFee(); }
uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; }
CValue GetModFeesWithAncestors() const { return nModFeesWithAncestors; }
size_t GetTxSize() const { return iter->GetTxSize(); }
const CTransaction& GetTx() const { return iter->GetTx(); }

CTxMemPool::txiter iter;
uint64_t nSizeWithAncestors;
CAmount nModFeesWithAncestors;
CValue nModFeesWithAncestors;
int64_t nSigOpCostWithAncestors;
};

Expand Down Expand Up @@ -116,7 +116,7 @@ struct update_for_parent_inclusion

void operator() (CTxMemPoolModifiedEntry &e)
{
e.nModFeesWithAncestors -= iter->GetFee();
e.nModFeesWithAncestors -= iter->GetFeeValue();
e.nSizeWithAncestors -= iter->GetTxSize();
e.nSigOpCostWithAncestors -= iter->GetSigOpCost();
}
Expand Down
4 changes: 2 additions & 2 deletions src/node/transaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
return HandleATMPError(result.m_state, err_string);
} else if (g_con_any_asset_fees) {
CAmount mBaseFeesValue = ExchangeRateMap::GetInstance().CalculateExchangeValue(result.m_base_fees.value(), tx->GetFeeAsset(::policyAsset));
if (mBaseFeesValue > max_tx_fee) {
CValue mBaseFeesValue = ExchangeRateMap::GetInstance().ConvertAmountToValue(result.m_base_fees.value(), tx->GetFeeAsset(::policyAsset));
if (mBaseFeesValue.GetValue() > max_tx_fee) {
return TransactionError::MAX_FEE_EXCEEDED;
}
} else if (result.m_base_fees.value() > max_tx_fee) {
Expand Down
13 changes: 13 additions & 0 deletions src/policy/feerate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <asset.h>
#include <exchangerates.h>
#include <policy/feerate.h>
#include <policy/value.h>
#include <primitives/transaction.h>

#include <tinyformat.h>

Expand Down Expand Up @@ -36,6 +40,15 @@ CAmount CFeeRate::GetFee(uint32_t num_bytes) const
return nFee;
}

CAmount CFeeRate::GetFee(uint32_t num_bytes, const CAsset& asset) const
{
CValue nFee = CValue(this->GetFee(num_bytes));
if (g_con_any_asset_fees) {
nFee = ExchangeRateMap::GetInstance().ConvertValueToAmount(nFee, asset);
}
return nFee.GetValue();
}

std::string CFeeRate::ToString(const FeeEstimateMode& fee_estimate_mode) const
{
switch (fee_estimate_mode) {
Expand Down
14 changes: 14 additions & 0 deletions src/policy/feerate.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,21 @@
#ifndef BITCOIN_POLICY_FEERATE_H
#define BITCOIN_POLICY_FEERATE_H

#include <asset.h>
#include <consensus/amount.h>
#include <serialize.h>

#include <string>

#ifdef ANY_ASSET_FEES
const std::string CURRENCY_UNIT = "RFU"; // One formatted unit (reference fee unit)
const std::string CURRENCY_ATOM = "rfa"; // One indivisible minimum value unit (reference fee atom)
const std::string CURRENCY_ATOM_FULL = "reference fee atom";
#else
const std::string CURRENCY_UNIT = "BTC"; // One formatted unit
const std::string CURRENCY_ATOM = "sat"; // One indivisible minimum value unit
const std::string CURRENCY_ATOM_FULL = "satoshi";
#endif

/* Used to determine type of fee estimation requested */
enum class FeeEstimateMode {
Expand Down Expand Up @@ -56,6 +64,12 @@ class CFeeRate
*/
CAmount GetFee(uint32_t num_bytes) const;

/**
* Return the fee in denominations of the fee asset for the given
* vsize in vbytes.
*/
CAmount GetFee(uint32_t num_bytes, const CAsset& asset) const;

/**
* Return the fee in satoshis for a vsize of 1000 vbytes
*/
Expand Down
2 changes: 1 addition & 1 deletion src/policy/fees.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo
trackedTxs++;

// Feerates are stored and reported as BTC-per-kb:
CFeeRate feeRate(entry.GetFee(), entry.GetTxSize());
CFeeRate feeRate(entry.GetFeeValue().GetValue(), entry.GetTxSize());

mapMemPoolTxs[hash].blockHeight = txHeight;
unsigned int bucketIndex = feeStats->NewTx(txHeight, (double)feeRate.GetFeePerK());
Expand Down
4 changes: 2 additions & 2 deletions src/policy/policy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@JBetz JBetz Apr 24, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
Expand Down Expand Up @@ -146,7 +146,7 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR
} else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) {
reason = "bare-multisig";
return false;
} else if ((txout.nAsset.IsExplicit() && txout.nAsset.GetAsset() == policyAsset) && IsDust(txout, dust_relay_fee)) {
} else if ((txout.nAsset.IsExplicit() && txout.nAsset.GetAsset() == tx.GetFeeAsset(::policyAsset)) && IsDust(txout, dust_relay_fee)) {
reason = "dust";
return false;
}
Expand Down
65 changes: 65 additions & 0 deletions src/policy/value.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_POLICY_VALUE_H
#define BITCOIN_POLICY_VALUE_H

#include <cstdint>

/** ELEMENTS: Amount denominated in the node's RFU (reference fee unit). Used only
* when con_any_asset_fees is enabled in order to distinguish from amounts in an
* actual asset. RFU is needed to make amounts comparable when sorting transactions
* in the mempool, as well as for fee estimation and subsequent validation of those
* fees according to various limits (e.g., mintxfee, paytsxfee, blockmintxfee,
* incrementalrelaytxfee, etc.).
*/
struct CValue
{
private:
int64_t value;

public:
CValue(): value(0) {}
CValue(const int64_t value): value(value) {}

int64_t GetValue() const
{
return value;
}

CValue operator -(const CValue& operand)
{
return CValue(value - operand.value);
}

CValue operator -=(const CValue& operand)
{
value -= operand.value;
return *this;
}

CValue operator +(const CValue& operand)
{
return CValue(value + operand.value);
}

CValue operator +=(const CValue& operand)
{
value += operand.value;
return *this;
}

bool operator ==(const CValue& operand)
{
return value == operand.value;
}

bool operator !=(const CValue& operand)
{
return value != operand.value;
}
};

#endif // BITCOIN_POLICY_VALUE_H
Loading