Merge #16507: feefilter: Compute the absolute fee rather than stored rate
eb7b781659
modify p2p_feefilter test to catch rounding error (Gregory Sanders)6a51f79517
Disallow implicit conversion for CFeeRate constructor (Gregory Sanders)8e59af55aa
feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders) Pull request description: This means we will use the rounding-down behavior in `GetFee` to match both mempool acceptance and wallet logic, with minimal changes. Fixes https://github.com/bitcoin/bitcoin/issues/16499 Replacement PR for https://github.com/bitcoin/bitcoin/pull/16500 ACKs for top commit: ajtowns: ACKeb7b781659
code review only naumenkogs: utACKeb7b781659
achow101: re ACKeb7b781659
promag: ACKeb7b781659
. Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e
This commit is contained in:
commit
94d6a18f23
6 changed files with 34 additions and 21 deletions
|
@ -3847,10 +3847,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
|
||||||
if (fSendTrickle && pto->m_tx_relay->fSendMempool) {
|
if (fSendTrickle && pto->m_tx_relay->fSendMempool) {
|
||||||
auto vtxinfo = mempool.infoAll();
|
auto vtxinfo = mempool.infoAll();
|
||||||
pto->m_tx_relay->fSendMempool = false;
|
pto->m_tx_relay->fSendMempool = false;
|
||||||
CAmount filterrate = 0;
|
CFeeRate filterrate;
|
||||||
{
|
{
|
||||||
LOCK(pto->m_tx_relay->cs_feeFilter);
|
LOCK(pto->m_tx_relay->cs_feeFilter);
|
||||||
filterrate = pto->m_tx_relay->minFeeFilter;
|
filterrate = CFeeRate(pto->m_tx_relay->minFeeFilter);
|
||||||
}
|
}
|
||||||
|
|
||||||
LOCK(pto->m_tx_relay->cs_filter);
|
LOCK(pto->m_tx_relay->cs_filter);
|
||||||
|
@ -3859,8 +3859,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
|
||||||
const uint256& hash = txinfo.tx->GetHash();
|
const uint256& hash = txinfo.tx->GetHash();
|
||||||
CInv inv(MSG_TX, hash);
|
CInv inv(MSG_TX, hash);
|
||||||
pto->m_tx_relay->setInventoryTxToSend.erase(hash);
|
pto->m_tx_relay->setInventoryTxToSend.erase(hash);
|
||||||
if (filterrate) {
|
// Don't send transactions that peers will not put into their mempool
|
||||||
if (txinfo.feeRate.GetFeePerK() < filterrate)
|
if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
if (pto->m_tx_relay->pfilter) {
|
if (pto->m_tx_relay->pfilter) {
|
||||||
|
@ -3884,10 +3884,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
|
||||||
for (std::set<uint256>::iterator it = pto->m_tx_relay->setInventoryTxToSend.begin(); it != pto->m_tx_relay->setInventoryTxToSend.end(); it++) {
|
for (std::set<uint256>::iterator it = pto->m_tx_relay->setInventoryTxToSend.begin(); it != pto->m_tx_relay->setInventoryTxToSend.end(); it++) {
|
||||||
vInvTx.push_back(it);
|
vInvTx.push_back(it);
|
||||||
}
|
}
|
||||||
CAmount filterrate = 0;
|
CFeeRate filterrate;
|
||||||
{
|
{
|
||||||
LOCK(pto->m_tx_relay->cs_feeFilter);
|
LOCK(pto->m_tx_relay->cs_feeFilter);
|
||||||
filterrate = pto->m_tx_relay->minFeeFilter;
|
filterrate = CFeeRate(pto->m_tx_relay->minFeeFilter);
|
||||||
}
|
}
|
||||||
// Topologically and fee-rate sort the inventory we send for privacy and priority reasons.
|
// Topologically and fee-rate sort the inventory we send for privacy and priority reasons.
|
||||||
// A heap is used so that not all items need sorting if only a few are being sent.
|
// A heap is used so that not all items need sorting if only a few are being sent.
|
||||||
|
@ -3914,7 +3914,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
|
||||||
if (!txinfo.tx) {
|
if (!txinfo.tx) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
if (filterrate && txinfo.feeRate.GetFeePerK() < filterrate) {
|
// Peer told you to not send transactions at that feerate? Don't bother sending it.
|
||||||
|
if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
if (pto->m_tx_relay->pfilter && !pto->m_tx_relay->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue;
|
if (pto->m_tx_relay->pfilter && !pto->m_tx_relay->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue;
|
||||||
|
|
|
@ -25,7 +25,7 @@ public:
|
||||||
/** Fee rate of 0 satoshis per kB */
|
/** Fee rate of 0 satoshis per kB */
|
||||||
CFeeRate() : nSatoshisPerK(0) { }
|
CFeeRate() : nSatoshisPerK(0) { }
|
||||||
template<typename I>
|
template<typename I>
|
||||||
CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {
|
explicit CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {
|
||||||
// We've previously had bugs creep in from silent double->int conversion...
|
// We've previously had bugs creep in from silent double->int conversion...
|
||||||
static_assert(std::is_integral<I>::value, "CFeeRate should be used without floats");
|
static_assert(std::is_integral<I>::value, "CFeeRate should be used without floats");
|
||||||
}
|
}
|
||||||
|
|
|
@ -773,7 +773,7 @@ void CTxMemPool::queryHashes(std::vector<uint256>& vtxid) const
|
||||||
}
|
}
|
||||||
|
|
||||||
static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator it) {
|
static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator it) {
|
||||||
return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), CFeeRate(it->GetFee(), it->GetTxSize()), it->GetModifiedFee() - it->GetFee()};
|
return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), it->GetFee(), it->GetTxSize(), it->GetModifiedFee() - it->GetFee()};
|
||||||
}
|
}
|
||||||
|
|
||||||
std::vector<TxMempoolInfo> CTxMemPool::infoAll() const
|
std::vector<TxMempoolInfo> CTxMemPool::infoAll() const
|
||||||
|
|
|
@ -334,8 +334,11 @@ struct TxMempoolInfo
|
||||||
/** Time the transaction entered the mempool. */
|
/** Time the transaction entered the mempool. */
|
||||||
std::chrono::seconds m_time;
|
std::chrono::seconds m_time;
|
||||||
|
|
||||||
/** Feerate of the transaction. */
|
/** Fee of the transaction. */
|
||||||
CFeeRate feeRate;
|
CAmount fee;
|
||||||
|
|
||||||
|
/** Virtual size of the transaction. */
|
||||||
|
size_t vsize;
|
||||||
|
|
||||||
/** The fee delta. */
|
/** The fee delta. */
|
||||||
int64_t nFeeDelta;
|
int64_t nFeeDelta;
|
||||||
|
|
|
@ -2327,7 +2327,7 @@ static UniValue settxfee(const JSONRPCRequest& request)
|
||||||
|
|
||||||
CAmount nAmount = AmountFromValue(request.params[0]);
|
CAmount nAmount = AmountFromValue(request.params[0]);
|
||||||
CFeeRate tx_fee_rate(nAmount, 1000);
|
CFeeRate tx_fee_rate(nAmount, 1000);
|
||||||
if (tx_fee_rate == 0) {
|
if (tx_fee_rate == CFeeRate(0)) {
|
||||||
// automatic selection
|
// automatic selection
|
||||||
} else if (tx_fee_rate < pwallet->chain().relayMinFee()) {
|
} else if (tx_fee_rate < pwallet->chain().relayMinFee()) {
|
||||||
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", pwallet->chain().relayMinFee().ToString()));
|
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", pwallet->chain().relayMinFee().ToString()));
|
||||||
|
@ -3386,7 +3386,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
|
||||||
}
|
}
|
||||||
} else if (options.exists("fee_rate")) {
|
} else if (options.exists("fee_rate")) {
|
||||||
CFeeRate fee_rate(AmountFromValue(options["fee_rate"]));
|
CFeeRate fee_rate(AmountFromValue(options["fee_rate"]));
|
||||||
if (fee_rate <= 0) {
|
if (fee_rate <= CFeeRate(0)) {
|
||||||
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", fee_rate.ToString()));
|
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", fee_rate.ToString()));
|
||||||
}
|
}
|
||||||
coin_control.m_feerate = fee_rate;
|
coin_control.m_feerate = fee_rate;
|
||||||
|
|
|
@ -41,6 +41,12 @@ class TestP2PConn(P2PInterface):
|
||||||
class FeeFilterTest(BitcoinTestFramework):
|
class FeeFilterTest(BitcoinTestFramework):
|
||||||
def set_test_params(self):
|
def set_test_params(self):
|
||||||
self.num_nodes = 2
|
self.num_nodes = 2
|
||||||
|
# We lower the various required feerates for this test
|
||||||
|
# to catch a corner-case where feefilter used to slightly undercut
|
||||||
|
# mempool and wallet feerate calculation based on GetFee
|
||||||
|
# rounding down 3 places, leading to stranded transactions.
|
||||||
|
# See issue #16499
|
||||||
|
self.extra_args = [["-minrelaytxfee=0.00000100", "-mintxfee=0.00000100"]]*self.num_nodes
|
||||||
|
|
||||||
def skip_test_if_missing_module(self):
|
def skip_test_if_missing_module(self):
|
||||||
self.skip_if_no_wallet()
|
self.skip_if_no_wallet()
|
||||||
|
@ -54,22 +60,25 @@ class FeeFilterTest(BitcoinTestFramework):
|
||||||
|
|
||||||
self.nodes[0].add_p2p_connection(TestP2PConn())
|
self.nodes[0].add_p2p_connection(TestP2PConn())
|
||||||
|
|
||||||
# Test that invs are received for all txs at feerate of 20 sat/byte
|
# Test that invs are received by test connection for all txs at
|
||||||
node1.settxfee(Decimal("0.00020000"))
|
# feerate of .2 sat/byte
|
||||||
|
node1.settxfee(Decimal("0.00000200"))
|
||||||
txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
|
txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
|
||||||
assert allInvsMatch(txids, self.nodes[0].p2p)
|
assert allInvsMatch(txids, self.nodes[0].p2p)
|
||||||
self.nodes[0].p2p.clear_invs()
|
self.nodes[0].p2p.clear_invs()
|
||||||
|
|
||||||
# Set a filter of 15 sat/byte
|
# Set a filter of .15 sat/byte on test connection
|
||||||
self.nodes[0].p2p.send_and_ping(msg_feefilter(15000))
|
self.nodes[0].p2p.send_and_ping(msg_feefilter(150))
|
||||||
|
|
||||||
# Test that txs are still being received (paying 20 sat/byte)
|
# Test that txs are still being received by test connection (paying .15 sat/byte)
|
||||||
|
node1.settxfee(Decimal("0.00000150"))
|
||||||
txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
|
txids = [node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
|
||||||
assert allInvsMatch(txids, self.nodes[0].p2p)
|
assert allInvsMatch(txids, self.nodes[0].p2p)
|
||||||
self.nodes[0].p2p.clear_invs()
|
self.nodes[0].p2p.clear_invs()
|
||||||
|
|
||||||
# Change tx fee rate to 10 sat/byte and test they are no longer received
|
# Change tx fee rate to .1 sat/byte and test they are no longer received
|
||||||
node1.settxfee(Decimal("0.00010000"))
|
# by the test connection
|
||||||
|
node1.settxfee(Decimal("0.00000100"))
|
||||||
[node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
|
[node1.sendtoaddress(node1.getnewaddress(), 1) for x in range(3)]
|
||||||
self.sync_mempools() # must be sure node 0 has received all txs
|
self.sync_mempools() # must be sure node 0 has received all txs
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue