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

Fix and improve relay from whitelisted peers #7106

Merged
merged 1 commit into from Nov 29, 2015

Conversation

Projects
None yet
4 participants
@sipa
Member

sipa commented Nov 26, 2015

This makes sure that retransmits by a whitelisted peer also actually result in a retransmit. This was a bug introduced in 9524c4d (which is in 0.11.1).

Further, this changes the logic to never relay in case we would assign a DoS score, as we expect to get DoS banned ourselves as a result. This part may be acceptable as an alternative to #7099.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 26, 2015

Member

utACK-we-should-do-this-over-nothing. I will test too.

I still think that whitelist drastically changing our P2P behavior (instead of merely bypassing resource limits/banning/eviction) is highly surprising, and undermines the utility of whitelisting if we're unable to unlink them; so I'd rebase 7099 on top of this and be open to making it default to on.

I think long term we should have another p2p message type which means "force relay this transaction" which we ignore the force part for non-whitelisted peers. ... we we don't manage to get initial transaction broadcast out of the basic p2p relaying mechanism first.

Member

gmaxwell commented Nov 26, 2015

utACK-we-should-do-this-over-nothing. I will test too.

I still think that whitelist drastically changing our P2P behavior (instead of merely bypassing resource limits/banning/eviction) is highly surprising, and undermines the utility of whitelisting if we're unable to unlink them; so I'd rebase 7099 on top of this and be open to making it default to on.

I think long term we should have another p2p message type which means "force relay this transaction" which we ignore the force part for non-whitelisted peers. ... we we don't manage to get initial transaction broadcast out of the basic p2p relaying mechanism first.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 26, 2015

Member

Would you oppose adding a non-debug log to this every time it does a forced relay and any time it fails to do one (due to DOS)?

Having it logged would help a lot with someone putting whitelisting things they shouldn't while this behavior exists (like mining software) and spamming the network.

Member

gmaxwell commented Nov 26, 2015

Would you oppose adding a non-debug log to this every time it does a forced relay and any time it fails to do one (due to DOS)?

Having it logged would help a lot with someone putting whitelisting things they shouldn't while this behavior exists (like mining software) and spamming the network.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 26, 2015

Member

@gmaxwell Done.

Member

sipa commented Nov 26, 2015

@gmaxwell Done.

Fix and improve relay from whitelisted peers
This makes sure that retransmits by a whitelisted peer also actually
result in a retransmit.

Further, this changes the logic to never relay in case we would assign
a DoS score, as we expect to get DoS banned ourselves as a result.

@jonasschnelli jonasschnelli added the P2P label Nov 27, 2015

@gmaxwell gmaxwell modified the milestones: 0.11.0, 0.12.0 Nov 27, 2015

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 27, 2015

Member

ACK

Member

gmaxwell commented Nov 27, 2015

ACK

@gmaxwell gmaxwell merged commit a9f3d3d into bitcoin:master Nov 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

gmaxwell added a commit that referenced this pull request Nov 29, 2015

Merge pull request #7106
a9f3d3d Fix and improve relay from whitelisted peers (Pieter Wuille)
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 29, 2015

Member

Just curious, if we're willing to relay rejected transactions from whitelisted peers, why not also relay their orphan transactions? The only reason I can think of is it would result in duplicate announcements, but only if the whitelisted peer is sending them out of order; and there might well be use cases where a transaction that is an orphan for you would not be an orphan for your peers (eg if you're running with a small mempool).

Member

sdaftuar commented Nov 29, 2015

Just curious, if we're willing to relay rejected transactions from whitelisted peers, why not also relay their orphan transactions? The only reason I can think of is it would result in duplicate announcements, but only if the whitelisted peer is sending them out of order; and there might well be use cases where a transaction that is an orphan for you would not be an orphan for your peers (eg if you're running with a small mempool).

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 30, 2015

Fix and improve relay from whitelisted peers
This makes sure that retransmits by a whitelisted peer also actually
result in a retransmit.

Further, this changes the logic to never relay in case we would assign
a DoS score, as we expect to get DoS banned ourselves as a result.

Github-Pull: #7106
Rebased-From: a9f3d3d

zkbot pushed a commit to zcash/zcash that referenced this pull request Sep 20, 2016

zkbot
Auto merge of #1411 - bitcartel:master_bitcoin_7106, r=daira
Upstream patch: Fix and improve relay from whitelisted peers

bitcoin/bitcoin#7106
a9f3d3d

An extra commit modifies the log message string, otherwise there are are a number of commits that need be to backported to add methods e.g. GetDebugMessage.  These commits modify the interface in consensus/validation.h so there are conflicts to be resolved. e.g.
9003c7c
a9ac95c
5f12263
fbf44e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment