Implement "feefilter" P2P message #7542

morcos commented at 8:18 PM on February 16, 2016: member

Please see mailing list discussion https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-February/012449.html

laanwj added the label P2P on Feb 17, 2016

in src/main.cpp:None in 452f116990 outdated

5166 | +    {
5167 | +        CAmount newFeeFilter = 0;
5168 | +        vRecv >> newFeeFilter;
5169 | +        if (MoneyRange(newFeeFilter)) {
5170 | +            {
5171 | +            LOCK(pfrom->cs_feeFilter);

jonasschnelli commented at 8:16 AM on February 22, 2016:

Nit: indent.

MarcoFalke commented at 10:31 AM on February 25, 2016:

I remember adding the clang-format script some time ago. Something like git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v should do all of this.

jonasschnelli commented at 8:23 AM on February 22, 2016: contributor

Nice Work! IMO relatively important PR to reduce p2p "noise". Concept ACK. Short code review.

in src/main.cpp:None in 452f116990 outdated

5161 | @@ -5146,6 +5162,19 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5162 |          }
5163 |      }
5164 |  
5165 | +    else if (strCommand == NetMsgType::FEEFILTER)
5166 | +    {
5167 | +        CAmount newFeeFilter = 0;
5168 | +        vRecv >> newFeeFilter;
5169 | +        if (MoneyRange(newFeeFilter)) {
5170 | +            {

jonasschnelli commented at 4:38 AM on February 23, 2016:

To unlock the thread-lock for the LogPrint at L5174.

in src/main.cpp:None in 452f116990 outdated

5657 | @@ -5629,6 +5658,29 @@ bool SendMessages(CNode* pto)
5658 |          if (!vGetData.empty())
5659 |              pto->PushMessage(NetMsgType::GETDATA, vGetData);
5660 |  
5661 | +        //
5662 | +        // Message: feefilter
5663 | +        //
5664 | +        // We want don't want white listed peers to filter txs to us if we have -whitelistforcerelay

paveljanik commented at 6:59 AM on February 23, 2016:

We want don't want?

in src/main.h:None in 452f116990 outdated

 123 | @@ -120,6 +124,8 @@ static const bool DEFAULT_ENABLE_REPLACEMENT = true;
 124 |  
 125 |  /** Maximum number of headers to announce when relaying blocks with headers message.*/
 126 |  static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8;
 127 | +/** Default for using fee filter*/

paveljanik commented at 7:00 AM on February 23, 2016:

SPC at the end missing

in src/policy/fees.h:None in 452f116990 outdated

 285 | @@ -286,4 +286,17 @@ class CBlockPolicyEstimator
 286 |      CFeeRate feeLikely, feeUnlikely;
 287 |      double priLikely, priUnlikely;
 288 |  };
 289 | +
 290 | +class FeeFilterRounder
 291 | +{
 292 | +public:
 293 | +    /** Create new FeeFilterRounder*/

paveljanik commented at 7:03 AM on February 23, 2016:

SPC at the end of comment

paveljanik commented at 7:11 AM on February 23, 2016: contributor

Any volunteer to write some test cases?

in src/protocol.h:None in 452f116990 outdated

 217 | @@ -218,7 +218,12 @@ extern const char *REJECT;
 218 |   * @see https://bitcoin.org/en/developer-reference#sendheaders
 219 |   */
 220 |  extern const char *SENDHEADERS;
 221 | -
 222 | +/**
 223 | + * The feefilter message tells the receiving peer not to inv us any txs
 224 | + * which do not meet the specified min fee rate.
 225 | + * @since protocol version 70013 as described by BIPXXX

paveljanik commented at 7:16 AM on February 23, 2016:

@luke-jr Can you please assign a BIP number?

luke-jr commented at 11:42 AM on February 25, 2016:

@morcos needs to request this.

paveljanik commented at 7:18 AM on February 23, 2016: contributor

What will we do when we sent feefilter to the peer and the peer sends us zero fee invs?

In the DIP draft, you wrote:

Upon receipt of a "feefilter" message, the node will be permitted, but not required, to filter transaction invs for transactions that fall below the feerate provided in the feefilter message interpreted as satoshis per kilobyte.

Is this what we want? Why should a protocol 70013 peer relay such invs to us?

morcos commented at 6:39 PM on February 25, 2016: member

Addressed the cleanup comments This has been submited as a PR for BIP 133. @paveljanik It's impossible to be sure there aren't invs or txs in flight with fee rates below the cut off. Also in general we don't have a good system for responding to p2p misbehavior. I think this could be extended later with that ability, but as of now it does not appear to open up any additional DoS attacks if you don't abide by the message.

in src/main.cpp:None in f9b6b860a8 outdated

5667 | +                    pto->PushMessage(NetMsgType::FEEFILTER, filterToSend);
5668 | +                    pto->lastSentFeeFilter = filterToSend;
5669 | +                }
5670 | +                pto->nextSendTimeFeeFilter = PoissonNextSend(timeNow, AVG_FEEFILTER_BROADCAST_INTERVAL);
5671 | +            }
5672 | +            // If the fee filter has changed substantially and its still more than MAX_FEEFILTER_CHANGE_DELAY

paveljanik commented at 6:13 PM on March 3, 2016:

its -> it is/it's

in src/wallet/wallet.cpp:None in f9b6b860a8 outdated

1264 | @@ -1265,9 +1265,11 @@ bool CWalletTx::RelayWalletTransaction()
1265 |      assert(pwallet->GetBroadcastTransactions());
1266 |      if (!IsCoinBase())
1267 |      {
1268 | -        if (GetDepthInMainChain() == 0 && !isAbandoned()) {
1269 | +        if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) {

paveljanik commented at 6:18 PM on March 3, 2016:

This have been merged now - can you please rebase?

morcos force-pushed on Mar 4, 2016

morcos commented at 4:18 PM on March 4, 2016: member

Squashed and rebased

MarcoFalke commented at 4:42 PM on March 4, 2016: member

Please mention the bip number in the github subject line. Also you'd need to update /doc/bips.md prior to merge.

in doc/release-notes.md:None in ef3b85040d outdated

  50 | @@ -51,6 +51,14 @@ The following outputs are affected by this change:
  51 |  
  52 |  ### P2P protocol and network code
  53 |  
  54 | +Fee filtering of invs (BIP 133)

MarcoFalke commented at 5:41 PM on March 5, 2016:

Nit: I am pretty sure this goes up to "Notable changes" (Example item) as this place down here is usually used for the detailed change log only.

in src/main.cpp:None in ef3b85040d outdated

 940 | @@ -938,7 +941,7 @@ std::string FormatStateMessage(const CValidationState &state)
 941 |  }
 942 |  
 943 |  bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const CTransaction& tx, bool fLimitFree,
 944 | -                              bool* pfMissingInputs, bool fOverrideMempoolLimit, const CAmount nAbsurdFee,
 945 | +                              bool* pfMissingInputs, CAmount* txFeeRate, bool fOverrideMempoolLimit, const CAmount nAbsurdFee,

MarcoFalke commented at 5:46 PM on March 5, 2016:

Nit: can you add the missing ampersand to CAmount nAbsurdFee as well here?

in src/net.cpp:None in ef3b85040d outdated

2067 | -
2068 | -
2069 | -
2070 | -
2071 | -void RelayTransaction(const CTransaction& tx)
2072 | +void RelayTransaction(const CTransaction& tx, CAmount fee)

MarcoFalke commented at 10:06 PM on March 6, 2016:

CAmount fee implies an absolute fee. Maybe CAmount feeRate?

Personally, I'd prefer to use CFeeRate in places except CNode. I.e. something like this should suffice:

diff --git a/src/main.cpp b/src/main.cpp
index 487564a..c0fccbf 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -944 +944 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
-                              bool* pfMissingInputs, CAmount* txFeeRate, bool fOverrideMempoolLimit, const CAmount nAbsurdFee,
+                              bool* pfMissingInputs, CFeeRate* txFeeRate, bool fOverrideMempoolLimit, const CAmount nAbsurdFee,
@@ -1101 +1101 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
-            *txFeeRate = CFeeRate(nFees, nSize).GetFeePerK();
+            *txFeeRate = CFeeRate(nFees, nSize);
@@ -1362 +1362 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
-                        bool* pfMissingInputs, CAmount* txFeeRate, bool fOverrideMempoolLimit, const CAmount nAbsurdFee)
+                        bool* pfMissingInputs, CFeeRate* txFeeRate, bool fOverrideMempoolLimit, const CAmount nAbsurdFee)
@@ -4829 +4829 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
-        CAmount txFeeRate = 0;
+        CFeeRate txFeeRate = CFeeRate(0);
@@ -4863 +4863 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
-                    CAmount orphanFeeRate = 0;
+                    CFeeRate orphanFeeRate = CFeeRate(0);
diff --git a/src/main.h b/src/main.h
index 5c006b2..fe4abb5 100644
--- a/src/main.h
+++ b/src/main.h
@@ -293 +293 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
-                        bool* pfMissingInputs, CAmount* txFee, bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0);
+                        bool* pfMissingInputs, CFeeRate* txFee, bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0);
diff --git a/src/net.cpp b/src/net.cpp
index 294d2d4..22e8b26 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2066 +2066 @@ instance_of_cnetcleanup;
-void RelayTransaction(const CTransaction& tx, CAmount fee)
+void RelayTransaction(const CTransaction& tx, CFeeRate fee)
@@ -2074 +2074 @@ void RelayTransaction(const CTransaction& tx, CAmount fee)
-void RelayTransaction(const CTransaction& tx, CAmount fee, const CDataStream& ss)
+void RelayTransaction(const CTransaction& tx, CFeeRate fee, const CDataStream& ss)
@@ -2097 +2097 @@ void RelayTransaction(const CTransaction& tx, CAmount fee, const CDataStream& ss
-            if (fee < pnode->minFeeFilter)
+            if (fee.GetFeePerK() < pnode->minFeeFilter)
diff --git a/src/net.h b/src/net.h
index 0b3400b..3d4117d 100644
--- a/src/net.h
+++ b/src/net.h
@@ -776,2 +776,2 @@ class CTransaction;
-void RelayTransaction(const CTransaction& tx, CAmount fee);
-void RelayTransaction(const CTransaction& tx, CAmount fee, const CDataStream& ss);
+void RelayTransaction(const CTransaction& tx, CFeeRate fee);
+void RelayTransaction(const CTransaction& tx, CFeeRate fee, const CDataStream& ss);
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 5c93846..931eaeb 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -821 +821 @@ UniValue sendrawtransaction(const UniValue& params, bool fHelp)
-    CAmount txFee = 0;
+    CFeeRate txFee = CFeeRate(0);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 586fc69..0070ad1 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1270 +1270 @@ bool CWalletTx::RelayWalletTransaction()
-            RelayTransaction((CTransaction)*this, feeRate.GetFeePerK());
+            RelayTransaction((CTransaction)*this, feeRate);

morcos commented at 1:04 PM on March 7, 2016:

@MarcoFalke I wasn't sure about that. I didn't love the idea of introducing CFeeRate into net.cpp and RelayTransaction, but if people prefer it that way I don't have a strong opinion.

MarcoFalke commented at 4:41 PM on March 7, 2016:

I mostly care about having it in the mempool, so the code can be reused by other logic than just feefilter.

Thanks for addressing the nits.

MarcoFalke commented at 10:12 PM on March 6, 2016: member

Concept ACK ef3b850

morcos commented at 4:07 PM on March 7, 2016: member

@MarcoFalke OK I made your suggested change

in src/main.h:None in ba8e4ef00d outdated

 289 | @@ -284,7 +290,7 @@ void PruneAndFlush();
 290 |  
 291 |  /** (try to) add transaction to memory pool **/
 292 |  bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree,
 293 | -                        bool* pfMissingInputs, bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0);
 294 | +                        bool* pfMissingInputs, CFeeRate* txFeeRate, bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0);

MarcoFalke commented at 7:46 PM on March 9, 2016:

tiny nit: Add missing ampersand to CAmount nAbsurdFee?

sdaftuar commented at 8:23 PM on March 11, 2016: member

ACK

paveljanik commented at 8:41 PM on March 15, 2016: contributor

ACK

Implement "feefilter" P2P message.

The "feefilter" p2p message is used to inform other nodes of your mempool min fee which is the feerate that any new transaction must meet to be accepted to your mempool.  This will allow them to filter invs to you according to this feerate.

9e072a6e66

Create SingleNodeConnCB class for RPC tests 5fa66e4682

Add p2p test for feefilter b536a6fc83

modify release-notes.md and bips.md 0371797e2a

in src/main.cpp:None in ba8e4ef00d outdated

2503 | @@ -2498,7 +2504,7 @@ bool static DisconnectTip(CValidationState& state, const Consensus::Params& cons
2504 |          // ignore validation errors in resurrected transactions
2505 |          list<CTransaction> removed;
2506 |          CValidationState stateDummy;
2507 | -        if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL, true)) {
2508 | +        if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL, NULL, true)) {
2509 |              mempool.remove(tx, removed, true);

MarcoFalke commented at 11:01 PM on March 19, 2016:

Needs merge conflict resolved.

morcos force-pushed on Mar 21, 2016

morcos commented at 2:49 PM on March 21, 2016: member

Squashed 1 commit and rebased to address merge conflict (trivial)

laanwj commented at 4:54 PM on March 21, 2016: member

utACK 0371797

laanwj merged this on Mar 21, 2016

laanwj closed this on Mar 21, 2016

laanwj referenced this in commit c946a15075 on Mar 21, 2016

in src/main.cpp:None in 0371797e2a

5874 | +        //
5875 | +        // We don't want white listed peers to filter txs to us if we have -whitelistforcerelay
5876 | +        if (pto->nVersion >= FEEFILTER_VERSION && GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
5877 | +            !(pto->fWhitelisted && GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY))) {
5878 | +            CAmount currentFilter = mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
5879 | +            int64_t timeNow = GetTimeMicros();

rebroad commented at 4:51 AM on December 11, 2016:

why not use nNow variable which is also set to GetTimeMicros() earlier in this function?

codablock referenced this in commit 118a089acf on Sep 16, 2017

codablock referenced this in commit 6c81247f5e on Sep 19, 2017

codablock referenced this in commit bd069a7628 on Dec 9, 2017

codablock referenced this in commit 11ac70af9e on Dec 19, 2017

codablock referenced this in commit 7e5172802e on Apr 11, 2018

codablock referenced this in commit 1e94f56d6d on Apr 11, 2018

codablock referenced this in commit 200253b1f6 on Apr 11, 2018

UdjinM6 referenced this in commit 67caa8eb59 on Apr 11, 2018

UdjinM6 referenced this in commit 8b4c419ed6 on Apr 11, 2018

andvgal referenced this in commit 91b6c87c8e on Jan 6, 2019

CryptoCentric referenced this in commit 24dc8fc137 on Mar 1, 2019

MarkLTZ referenced this in commit a1d9318ca6 on Apr 28, 2019

MarcoFalke locked this on Sep 8, 2021