Implement "feefilter" P2P message by morcos · Pull Request #7542 · bitcoin/bitcoin

morcos · 2016-02-16T20:18:49Z

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

jonasschnelli · 2016-02-22T08:16:07Z

src/main.cpp

Nit: indent.

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 · 2016-02-22T08:23:48Z

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

jonasschnelli · 2016-02-23T04:38:23Z

src/main.cpp

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

paveljanik · 2016-02-23T07:11:45Z

Any volunteer to write some test cases?

paveljanik · 2016-02-23T07:16:29Z

src/protocol.h

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

@morcos needs to request this.

paveljanik · 2016-02-23T07:18:02Z

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 · 2016-02-25T18:39:49Z

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.

paveljanik · 2016-03-03T18:13:09Z

src/main.cpp

its -> it is/it's

morcos · 2016-03-04T16:18:42Z

Squashed and rebased

maflcko · 2016-03-04T16:42:37Z

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

maflcko · 2016-03-05T17:41:19Z

doc/release-notes.md

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.

maflcko · 2016-03-06T22:12:14Z

Concept ACK ef3b850

morcos · 2016-03-07T16:07:14Z

@MarcoFalke OK I made your suggested change

maflcko · 2016-03-09T19:46:36Z

src/main.h

tiny nit: Add missing ampersand to CAmount nAbsurdFee?

sdaftuar · 2016-03-11T20:23:42Z

ACK

paveljanik · 2016-03-15T20:41:17Z

ACK

maflcko · 2016-03-19T23:01:26Z

src/main.cpp

Needs merge conflict resolved.

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.

morcos · 2016-03-21T14:49:46Z

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

laanwj · 2016-03-21T16:54:30Z

utACK 0371797

0371797 modify release-notes.md and bips.md (Alex Morcos) b536a6f Add p2p test for feefilter (Alex Morcos) 5fa66e4 Create SingleNodeConnCB class for RPC tests (Alex Morcos) 9e072a6 Implement "feefilter" P2P message. (Alex Morcos)

rebroad · 2016-12-11T04:51:28Z

src/main.cpp

+        if (pto->nVersion >= FEEFILTER_VERSION && GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
+            !(pto->fWhitelisted && GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY))) {
+            CAmount currentFilter = mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
+            int64_t timeNow = GetTimeMicros();


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

0371797 modify release-notes.md and bips.md (Alex Morcos) b536a6f Add p2p test for feefilter (Alex Morcos) 5fa66e4 Create SingleNodeConnCB class for RPC tests (Alex Morcos) 9e072a6 Implement "feefilter" P2P message. (Alex Morcos)

This reverts commit 11ac70a.

…ay#2025) This reverts commit 11ac70a.

…ay#2025)

laanwj added the P2P label Feb 17, 2016

jonasschnelli reviewed Feb 22, 2016
View reviewed changes

jonasschnelli reviewed Feb 23, 2016
View reviewed changes

src/main.cpp Outdated

Copy link

Contributor

jonasschnelli Feb 23, 2016

Choose a reason for hiding this comment

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

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

paveljanik reviewed Feb 23, 2016
View reviewed changes

paveljanik reviewed Mar 3, 2016
View reviewed changes

src/main.cpp Outdated

Copy link

Contributor

paveljanik Mar 3, 2016

Choose a reason for hiding this comment

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

its -> it is/it's

morcos force-pushed the feefilter branch from f9b6b86 to 6d33d5a Compare March 4, 2016 16:18

maflcko reviewed Mar 5, 2016
View reviewed changes

maflcko reviewed Mar 9, 2016
View reviewed changes

src/main.h Outdated

Copy link

Member

maflcko Mar 9, 2016

Choose a reason for hiding this comment

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

tiny nit: Add missing ampersand to CAmount nAbsurdFee?

maflcko reviewed Mar 19, 2016
View reviewed changes

src/main.cpp Outdated

Copy link

Member

maflcko Mar 19, 2016

Choose a reason for hiding this comment

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

Needs merge conflict resolved.

morcos added 4 commits March 21, 2016 10:46

Implement "feefilter" P2P message.

9e072a6

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.

Create SingleNodeConnCB class for RPC tests

5fa66e4

Add p2p test for feefilter

b536a6f

modify release-notes.md and bips.md

0371797

morcos force-pushed the feefilter branch from ba8e4ef to 0371797 Compare March 21, 2016 14:49

laanwj merged commit 0371797 into bitcoin:master Mar 21, 2016

laanwj mentioned this pull request Jun 6, 2016

"Tell other nodes to filter invs to us by our mempool min fee" message is too technical #8150

Closed

rebroad mentioned this pull request Aug 26, 2016

Provide bloom services to whitelisted nodes. #8587

Closed

rebroad reviewed Dec 11, 2016

View reviewed changes

codablock added a commit to codablock/dash that referenced this pull request Apr 11, 2018

Revert "Merge bitcoin#7542: Implement "feefilter" P2P message"

200253b

This reverts commit 11ac70a.

UdjinM6 pushed a commit to dashpay/dash that referenced this pull request Apr 11, 2018

Revert "Merge bitcoin#7542: Implement "feefilter" P2P message" (#2025)

8b4c419

This reverts commit 11ac70a.

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Revert "Merge bitcoin#7542: Implement "feefilter" P2P message" (dashp…

91b6c87

…ay#2025) This reverts commit 11ac70a.

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Mar 1, 2019

Revert "Merge bitcoin#7542: Implement "feefilter" P2P message" (dashp…

24dc8fc

…ay#2025)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

morcos commented Feb 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Feb 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Feb 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Feb 23, 2016

Uh oh!

morcos commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Mar 4, 2016

Uh oh!

maflcko commented Mar 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 6, 2016

Uh oh!

morcos commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Mar 11, 2016

Uh oh!

paveljanik commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Mar 21, 2016

Uh oh!

laanwj commented Mar 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants