From ce6e3d8137128e99b2f7f99f79327b8b440df0a2 Mon Sep 17 00:00:00 2001 From: Ross Nicoll Date: Mon, 9 Aug 2021 23:27:38 +0100 Subject: [PATCH 1/2] Remove relay-only rounding Remove rounding of transaction sizes when calculating fee minimums for relaying, to simplify fee logic. --- qa/rpc-tests/test_framework/util.py | 2 +- src/amount.cpp | 10 ++++++++++ src/amount.h | 6 +++++- src/dogecoin-fees.cpp | 2 +- src/miner.cpp | 2 +- src/test/amount_tests.cpp | 10 +++++++++- src/test/miner_tests.cpp | 4 ++-- src/validation.cpp | 8 ++++---- 8 files changed, 33 insertions(+), 11 deletions(-) diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index 4e4011b3a..a9723df07 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -509,7 +509,7 @@ def random_transaction(nodes, amount, min_fee, fee_increment, fee_variants): def assert_fee_amount(fee, tx_size, fee_per_kB): """Assert the fee was in range""" - target_fee = round_tx_size(tx_size) * fee_per_kB / 1000 + target_fee = fee_per_kB / 1000 if fee < target_fee: raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)"%(str(fee), str(target_fee))) # allow the wallet's estimation to be at most 2 bytes off diff --git a/src/amount.cpp b/src/amount.cpp index 176c0be85..aa6bf943e 100644 --- a/src/amount.cpp +++ b/src/amount.cpp @@ -30,6 +30,16 @@ CAmount CFeeRate::GetFee(size_t nBytes_) const nSize = nSize + 1000 - (nSize % 1000); } + return GetRelayFee(nSize); +} + +// Dogecoin: Specifically for 1.14.4 we lower accepted relay fees by removing rounding, +// in 1.14.5 we should unify the GetFee() functions again. +CAmount CFeeRate::GetRelayFee(size_t nBytes_) const +{ + assert(nBytes_ <= uint64_t(std::numeric_limits::max())); + int64_t nSize = int64_t(nBytes_); + CAmount nFee = nSatoshisPerK * nSize / 1000; if (nFee == 0 && nSize != 0) { diff --git a/src/amount.h b/src/amount.h index e6e503905..02d61fc9b 100644 --- a/src/amount.h +++ b/src/amount.h @@ -46,9 +46,13 @@ public: CFeeRate(const CAmount& nFeePaid, size_t nBytes); CFeeRate(const CFeeRate& other) { nSatoshisPerK = other.nSatoshisPerK; } /** - * Return the fee in satoshis for the given size in bytes. + * Return the wallet fee in koinus for the given size in bytes. */ CAmount GetFee(size_t nBytes) const; + /** + * Return the relay fee in koinus for the given size in bytes. + */ + CAmount GetRelayFee(size_t nBytes) const; /** * Return the fee in satoshis for a size of 1000 bytes */ diff --git a/src/dogecoin-fees.cpp b/src/dogecoin-fees.cpp index c0542eb3c..890908ad6 100644 --- a/src/dogecoin-fees.cpp +++ b/src/dogecoin-fees.cpp @@ -46,7 +46,7 @@ CAmount GetDogecoinMinRelayFee(const CTransaction& tx, unsigned int nBytes, bool return 0; } - CAmount nMinFee = ::minRelayTxFeeRate.GetFee(nBytes); + CAmount nMinFee = ::minRelayTxFeeRate.GetRelayFee(nBytes); nMinFee += GetDogecoinDustFee(tx.vout, ::minRelayTxFeeRate); if (fAllowFree) diff --git a/src/miner.cpp b/src/miner.cpp index 55a90b86f..c4ec82d6a 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -490,7 +490,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda packageSigOpsCost = modit->nSigOpCostWithAncestors; } - if (packageFees < blockMinFeeRate.GetFee(packageSize)) { + if (packageFees < blockMinFeeRate.GetRelayFee(packageSize)) { // Everything else we might consider has a lower fee rate return; } diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp index 47e73ed38..80a6c3a6f 100644 --- a/src/test/amount_tests.cpp +++ b/src/test/amount_tests.cpp @@ -19,7 +19,7 @@ BOOST_AUTO_TEST_CASE(GetFeeTest) BOOST_CHECK_EQUAL(feeRate.GetFee(1e5), 0); feeRate = CFeeRate(1000); - // Must always just return the arg + // Wallet fees are rounded up BOOST_CHECK_EQUAL(feeRate.GetFee(0), 0); BOOST_CHECK_EQUAL(feeRate.GetFee(1), 1000); BOOST_CHECK_EQUAL(feeRate.GetFee(121), 1000); @@ -27,6 +27,14 @@ BOOST_AUTO_TEST_CASE(GetFeeTest) BOOST_CHECK_EQUAL(feeRate.GetFee(1e3), 1000); BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), 9000); + // Relay fees must always just return the arg + BOOST_CHECK_EQUAL(feeRate.GetRelayFee(0), 0); + BOOST_CHECK_EQUAL(feeRate.GetRelayFee(1), 1); + BOOST_CHECK_EQUAL(feeRate.GetRelayFee(121), 121); + BOOST_CHECK_EQUAL(feeRate.GetRelayFee(999), 999); + BOOST_CHECK_EQUAL(feeRate.GetRelayFee(1e3), 1e3); + BOOST_CHECK_EQUAL(feeRate.GetRelayFee(9e3), 9e3); + feeRate = CFeeRate(-1000); // Must always just return -1 * arg BOOST_CHECK_EQUAL(feeRate.GetFee(0), 0); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index ba06a2ab7..3995d3096 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -121,7 +121,7 @@ void TestPackageSelection(const CChainParams& chainparams, CScript scriptPubKey, // Calculate a fee on child transaction that will put the package just // below the block min tx fee (assuming 1 child tx of the same size). - CAmount feeToUse = blockMinFeeRate.GetFee(2*freeTxSize) - 1; + CAmount feeToUse = blockMinFeeRate.GetRelayFee(2*freeTxSize) - 1; tx.vin[0].prevout.hash = hashFreeTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse; @@ -158,7 +158,7 @@ void TestPackageSelection(const CChainParams& chainparams, CScript scriptPubKey, // This tx can't be mined by itself tx.vin[0].prevout.hash = hashFreeTx2; tx.vout.resize(1); - feeToUse = blockMinFeeRate.GetFee(freeTxSize); + feeToUse = blockMinFeeRate.GetRelayFee(freeTxSize); tx.vout[0].nValue = 5000000000LL - 100000000 - feeToUse; uint256 hashLowFeeTx2 = tx.GetHash(); mempool.addUnchecked(hashLowFeeTx2, entry.Fee(feeToUse).SpendsCoinbase(false).FromTx(tx)); diff --git a/src/validation.cpp b/src/validation.cpp index 8fc4ef6c7..9a307c9d0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -786,10 +786,10 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C return state.DoS(0, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops", false, strprintf("%d", nSigOpsCost)); - CAmount mempoolRejectFee = pool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(nSize); + CAmount mempoolRejectFee = pool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetRelayFee(nSize); if (mempoolRejectFee > 0 && nModifiedFees < mempoolRejectFee) { return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "mempool min fee not met", false, strprintf("%d < %d", nFees, mempoolRejectFee)); - } else if (GetBoolArg("-relaypriority", DEFAULT_RELAYPRIORITY) && nModifiedFees < ::minRelayTxFeeRate.GetFee(nSize) && !AllowFree(entry.GetPriority(chainActive.Height() + 1))) { + } else if (GetBoolArg("-relaypriority", DEFAULT_RELAYPRIORITY) && nModifiedFees < ::minRelayTxFeeRate.GetRelayFee(nSize) && !AllowFree(entry.GetPriority(chainActive.Height() + 1))) { // Require that free transactions have sufficient priority to be mined in the next block. return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "insufficient priority"); } @@ -966,14 +966,14 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C // Finally in addition to paying more fees than the conflicts the // new transaction must pay for its own bandwidth. CAmount nDeltaFees = nModifiedFees - nConflictingFees; - if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) + if (nDeltaFees < ::incrementalRelayFee.GetRelayFee(nSize)) { return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", hash.ToString(), FormatMoney(nDeltaFees), - FormatMoney(::incrementalRelayFee.GetFee(nSize)))); + FormatMoney(::incrementalRelayFee.GetRelayFee(nSize)))); } } From e90e8e5cba03639292b68d055fab08c57ef47d81 Mon Sep 17 00:00:00 2001 From: Ed <84785904+edtubbs@users.noreply.github.com> Date: Wed, 11 Aug 2021 14:01:42 -0500 Subject: [PATCH 2/2] Create feelimit.py test Create feelimit.py test to verify the updated fee values now rounding has been eliminated. --- qa/pull-tester/rpc-tests.py | 1 + qa/rpc-tests/feelimit.py | 77 +++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 qa/rpc-tests/feelimit.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 71c7b5f20..c5528118e 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -156,6 +156,7 @@ testScripts = [ 'import-rescan.py', 'harddustlimit.py', 'paytxfee.py', + 'feelimit.py', # While fee bumping should work in Doge, these tests depend on free transactions, which we don't support. # Disable until we can do a full rewrite of the tests (possibly upstream), or revise fee schedule, or something 'bumpfee.py', diff --git a/qa/rpc-tests/feelimit.py b/qa/rpc-tests/feelimit.py new file mode 100644 index 000000000..aef63564c --- /dev/null +++ b/qa/rpc-tests/feelimit.py @@ -0,0 +1,77 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 The Dogecoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Fee limit QA test. +# Tests nodes under fee limit, verifies fee rounding +""" + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import * +from decimal import Decimal + +class FeeLimitTest(BitcoinTestFramework): + + def __init__(self): + super().__init__() + self.setup_clean_chain = True + self.num_nodes = 2 + + def setup_network(self, split=False): + self.nodes = [] + self.nodes.append(start_node(0, self.options.tmpdir, [])) + self.nodes.append(start_node(1, self.options.tmpdir, [])) + + connect_nodes_bi(self.nodes,0,1) + + self.is_network_split=False + self.sync_all() + + def run_test(self): + # mine some blocks + self.nodes[0].generate(101) + self.sync_all() + + # Output address + addr_to = self.nodes[1].getnewaddress() + lower_fee_per_kb = Decimal("0.001") + + # Force generating a transaction with 1.14.5-like fees by manually building the tx + utx = self.nodes[0].listunspent()[0] + inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']}] + outputs = { addr_to : utx['amount'] - Decimal("1.0") } + rawtx = self.nodes[0].createrawtransaction(inputs, outputs) + + # Work out how big this transaction would be, then change the output to match + tx_size = count_bytes(self.nodes[0].signrawtransaction(rawtx)['hex']) + fee = lower_fee_per_kb * (tx_size + 1) / Decimal("1000") + outputs = { addr_to : utx['amount'] - fee } + rawtx = self.nodes[0].createrawtransaction(inputs, outputs) + signedtx = self.nodes[0].signrawtransaction(rawtx) + self.nodes[0].sendrawtransaction(signedtx["hex"]) + + # Sync all of the nodes + self.sync_all() + + # Check if the TX made it to node 2 + assert_equal(self.nodes[1].getmempoolinfo()['size'], 1) + assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) + + # Force generating a transaction with fees which are too low to relay + too_low_fee = lower_fee_per_kb * (tx_size - 4) / Decimal("1000") + utx = self.nodes[0].listunspent()[0] + inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']}] + outputs = { self.nodes[1].getnewaddress() : utx['amount'] - too_low_fee } + rawtx = self.nodes[0].createrawtransaction(inputs, outputs) + signedtx = self.nodes[0].signrawtransaction(rawtx) + self.nodes[0].sendrawtransaction(signedtx["hex"]) + + # wait 10 seconds to sync mempools + time.sleep(10) + + # Check the TX did not relay, so is only on node 0 + assert_equal(self.nodes[1].getmempoolinfo()['size'], 1) + assert_equal(self.nodes[0].getmempoolinfo()['size'], 2) + +if __name__ == '__main__': + FeeLimitTest().main()