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

Implement "feefilter" P2P message #7542

Merged
merged 4 commits into from Mar 21, 2016

Conversation

Projects
None yet
8 participants
@morcos
Member

morcos commented Feb 16, 2016

@laanwj laanwj added the P2P label Feb 17, 2016

@jonasschnelli

View changes

Show outdated Hide outdated src/main.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 22, 2016

Member

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

Member

jonasschnelli commented Feb 22, 2016

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

@jonasschnelli

View changes

Show outdated Hide outdated src/main.cpp
@paveljanik

View changes

Show outdated Hide outdated src/main.cpp
@paveljanik

View changes

Show outdated Hide outdated src/main.h
@paveljanik

View changes

Show outdated Hide outdated src/policy/fees.h
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 23, 2016

Contributor

Any volunteer to write some test cases?

Contributor

paveljanik commented Feb 23, 2016

Any volunteer to write some test cases?

@paveljanik

View changes

Show outdated Hide outdated src/protocol.h
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 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?

Contributor

paveljanik commented Feb 23, 2016

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Feb 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.

Member

morcos commented Feb 25, 2016

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

View changes

Show outdated Hide outdated src/main.cpp
@paveljanik

View changes

Show outdated Hide outdated src/wallet/wallet.cpp
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Mar 4, 2016

Member

Squashed and rebased

Member

morcos commented Mar 4, 2016

Squashed and rebased

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 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.

Member

MarcoFalke commented Mar 4, 2016

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

@MarcoFalke

View changes

Show outdated Hide outdated doc/release-notes.md
@MarcoFalke

View changes

Show outdated Hide outdated src/main.cpp
@MarcoFalke

View changes

Show outdated Hide outdated src/net.cpp
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 6, 2016

Member

Concept ACK ef3b850

Member

MarcoFalke commented Mar 6, 2016

Concept ACK ef3b850

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Mar 7, 2016

Member

@MarcoFalke OK I made your suggested change

Member

morcos commented Mar 7, 2016

@MarcoFalke OK I made your suggested change

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 9, 2016

Member

tiny nit: Add missing ampersand to CAmount nAbsurdFee?

@MarcoFalke

MarcoFalke Mar 9, 2016

Member

tiny nit: Add missing ampersand to CAmount nAbsurdFee?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 11, 2016

Member

ACK

Member

sdaftuar commented Mar 11, 2016

ACK

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Mar 15, 2016

Contributor

ACK

Contributor

paveljanik commented Mar 15, 2016

ACK

@MarcoFalke

View changes

Show outdated Hide outdated src/main.cpp

morcos added some commits Feb 12, 2016

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.
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Mar 21, 2016

Member

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

Member

morcos commented Mar 21, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 21, 2016

Member

utACK 0371797

Member

laanwj commented Mar 21, 2016

utACK 0371797

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

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

laanwj added a commit that referenced this pull request Mar 21, 2016

Merge #7542: Implement "feefilter" P2P message
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)
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();

This comment has been minimized.

@rebroad

rebroad Dec 11, 2016

Contributor

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

@rebroad

rebroad Dec 11, 2016

Contributor

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

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7542: Implement "feefilter" P2P message
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)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7542: Implement "feefilter" P2P message
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)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7542: Implement "feefilter" P2P message
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)

codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge #7542: Implement "feefilter" P2P message
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)

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

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

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment