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

Add whitelistforcerelay setting [PR changed to no longer default to off.] #7099

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@gmaxwell
Member

gmaxwell commented Nov 25, 2015

Previously the node would relay all transactions from whitelisted
peers, including invalid ones which will cause instant bannind.

This is a pretty big footgun.

It also gets in the way of some useful reasons for whitelisting
peers-- for example, bypassing bandwidth limitations.

The purpose of this forced relaying is for specialized gateway
applications where a node is being used as a P2P connection
filter and multiplexer, but where you don't want it getting
in the way of broadcast. People doing this know they're doing
this, so this change makes it a setting and defaults it to off.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 25, 2015

Member

From the "things I am tired of patching out locally".

Full disclosure: this behavior has surprised me twice and caused me to waste hours of time. I think it's generally nuts as a default. I believe in and agree with the applications for it, but I think they're pretty specialized.

Member

gmaxwell commented Nov 25, 2015

From the "things I am tired of patching out locally".

Full disclosure: this behavior has surprised me twice and caused me to waste hours of time. I think it's generally nuts as a default. I believe in and agree with the applications for it, but I think they're pretty specialized.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 25, 2015

Member

This seems like a pretty big change, and removing the original reason for whitelisted peers as a default: running a wallet behind an edge router and making it able to rebroadcast.

How about always relaying for whitelisted peers, except when the transactions are invalid (but do relay when they were already in the mempool).

Member

sipa commented Nov 25, 2015

This seems like a pretty big change, and removing the original reason for whitelisted peers as a default: running a wallet behind an edge router and making it able to rebroadcast.

How about always relaying for whitelisted peers, except when the transactions are invalid (but do relay when they were already in the mempool).

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 25, 2015

Member

Wait a minute! That's already broken by the recently-added "if (AlreadyHave(inv))" test before the AcceptToMemoryPool call. We could just move the white relay behaviour there I think?

Member

sipa commented Nov 25, 2015

Wait a minute! That's already broken by the recently-added "if (AlreadyHave(inv))" test before the AcceptToMemoryPool call. We could just move the white relay behaviour there I think?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 25, 2015

Member

The forced relaying has made whitelistning useless to me since day one: It means I cannot whitelist my mining software (so that it won't get banned), it means I cannot whitelist the relay node client (so that it won't get banned and can populate my mempool). It means I cannot whitelist nodes inside my network (that run various sets of software and sometimes spam invalid transactions) to bypass the bandwidth limiter. I have never heard from anyone but armory users using it in that gatewayed mode.

The invalid tx relay in particular is problematic. It means that as validity rules change you can't protect yourself from getting banned for sending invalid things by being a whitelisted client behind an updated node.

There are three states to consider for transactions from a whitelisted peer: They've sent us something invalid (for example an armory wallet which isn't yet enforcing lowS), they've sent us something our policy rejects (our fee policy is more restrictive than the network, or we're blocksonly), or they've sent us something we've already relayed.

Each one of these precludes the above uses for whitelisting I have, except maybe the last (I believe RNC and p2pool will both potentially repeat transactions-- but that could be fixed).

Maybe we should unconditionally never forward invalid transactions; and if we did, I wouldn't mind putting this back to default on. Although, I still think the multipliexer usage is very specialized and surprising, and doesn't fit with the usage of whitelisting to bypass resource limits; and so I think this should be possible to disable.

Member

gmaxwell commented Nov 25, 2015

The forced relaying has made whitelistning useless to me since day one: It means I cannot whitelist my mining software (so that it won't get banned), it means I cannot whitelist the relay node client (so that it won't get banned and can populate my mempool). It means I cannot whitelist nodes inside my network (that run various sets of software and sometimes spam invalid transactions) to bypass the bandwidth limiter. I have never heard from anyone but armory users using it in that gatewayed mode.

The invalid tx relay in particular is problematic. It means that as validity rules change you can't protect yourself from getting banned for sending invalid things by being a whitelisted client behind an updated node.

There are three states to consider for transactions from a whitelisted peer: They've sent us something invalid (for example an armory wallet which isn't yet enforcing lowS), they've sent us something our policy rejects (our fee policy is more restrictive than the network, or we're blocksonly), or they've sent us something we've already relayed.

Each one of these precludes the above uses for whitelisting I have, except maybe the last (I believe RNC and p2pool will both potentially repeat transactions-- but that could be fixed).

Maybe we should unconditionally never forward invalid transactions; and if we did, I wouldn't mind putting this back to default on. Although, I still think the multipliexer usage is very specialized and surprising, and doesn't fit with the usage of whitelisting to bypass resource limits; and so I think this should be possible to disable.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 26, 2015

Contributor

Maybe we should unconditionally never forward invalid transactions; and if we did, I wouldn't mind putting this back to default on.

Do you mean invalid in terms of node policy, or invalid in terms of its badly formatted / scripts evaluate false?

Contributor

dcousens commented Nov 26, 2015

Maybe we should unconditionally never forward invalid transactions; and if we did, I wouldn't mind putting this back to default on.

Do you mean invalid in terms of node policy, or invalid in terms of its badly formatted / scripts evaluate false?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 26, 2015

Member

Invalid means that it is badly formatted, spends non-existing coins, scripts evaluate to false, etc.

Member

gmaxwell commented Nov 26, 2015

Invalid means that it is badly formatted, spends non-existing coins, scripts evaluate to false, etc.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 26, 2015

Contributor

Is there any reason to ever relay them?

Contributor

dcousens commented Nov 26, 2015

Is there any reason to ever relay them?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 26, 2015

Member

I think the only use for whitelist's relay behaviour is to relay things that are already in the mempool, or perhaps things that were refused due to policy. I don't think it should ever relay actually invalid things.

Member

sipa commented Nov 26, 2015

I think the only use for whitelist's relay behaviour is to relay things that are already in the mempool, or perhaps things that were refused due to policy. I don't think it should ever relay actually invalid things.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 26, 2015

Member

I gave an example: something which is invalid due to mempool script verify flags... it's arguably that these should be relayed for the same reason as other policy violations.

If we don't mind not relaying those, I'll go fix this to not relay invalids, fix the fact that it's currently broken entirely (as sipa mentioned) and default this to ON (and make it soft-set off by blockonly, of course).

Member

gmaxwell commented Nov 26, 2015

I gave an example: something which is invalid due to mempool script verify flags... it's arguably that these should be relayed for the same reason as other policy violations.

If we don't mind not relaying those, I'll go fix this to not relay invalids, fix the fact that it's currently broken entirely (as sipa mentioned) and default this to ON (and make it soft-set off by blockonly, of course).

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 26, 2015

Member

@gmaxwell We don't ever DoS ban for failing a non-consensus rule in scripts. Maybe that criterion could be reuse: don't ever relay (not even to whitelisted peers) if our own validation logic set a non-zero DoS score?

Member

sipa commented Nov 26, 2015

@gmaxwell We don't ever DoS ban for failing a non-consensus rule in scripts. Maybe that criterion could be reuse: don't ever relay (not even to whitelisted peers) if our own validation logic set a non-zero DoS score?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 26, 2015

Member

@sipa indeed. In any case, the fact that whitelist relay hasn't worked since 0.11.1 without complaint supports my belief that almost no one uses it / expects it.

Member

gmaxwell commented Nov 26, 2015

@sipa indeed. In any case, the fact that whitelist relay hasn't worked since 0.11.1 without complaint supports my belief that almost no one uses it / expects it.

@gmaxwell gmaxwell changed the title from Add whitelistforcerelay setting and default to off. to Add whitelistforcerelay setting [PR changed to no longer default to off.] Nov 28, 2015

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 28, 2015

Member

Rebased on #7106 and in consideration of the fact that it makes the node not relay invalid transactions, I changed the default here to on. I still think making this configurable is important.

Member

gmaxwell commented Nov 28, 2015

Rebased on #7106 and in consideration of the fact that it makes the node not relay invalid transactions, I changed the default here to on. I still think making this configurable is important.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 28, 2015

Member

Needs rebase...

Member

sipa commented Nov 28, 2015

Needs rebase...

Add whitelistforcerelay to control forced relaying.
Also renames whitelistalwaysrelay.

Nodes relay all transactions from whitelisted peers, this
 gets in the way of some useful reasons for whitelisting
 peers-- for example, bypassing bandwidth limitations.

The purpose of this forced relaying is for specialized gateway
 applications where a node is being used as a P2P connection
 filter and multiplexer, but where you don't want it getting
 in the way of (re-)broadcast.

This change makes it configurable with whitelistforcerelay.

@laanwj laanwj added the Feature label Dec 3, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 28, 2016

Member

Needs rebase again (sorry)
Also needs text for the 0.13 release notes as this is indeed a pretty big change.

Member

laanwj commented Jan 28, 2016

Needs rebase again (sorry)
Also needs text for the 0.13 release notes as this is indeed a pretty big change.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 28, 2016

Member

@laanwj It's not a big change: it was updated to default to off-- I got tired of arguing about it, and sipa fixed the most grave of the misbehavior (sending invalid transactions) with it turned on in 0.12.

I can rebase.

Member

gmaxwell commented Jan 28, 2016

@laanwj It's not a big change: it was updated to default to off-- I got tired of arguing about it, and sipa fixed the most grave of the misbehavior (sending invalid transactions) with it turned on in 0.12.

I can rebase.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 28, 2016

Member

#7439 now, sorry. I couldn't figure out how to update this with the branch having been deleted on my tree.

Member

gmaxwell commented Jan 28, 2016

#7439 now, sorry. I couldn't figure out how to update this with the branch having been deleted on my tree.

@gmaxwell gmaxwell closed this Jan 28, 2016

laanwj added a commit that referenced this pull request Feb 1, 2016

Merge #7439: Add whitelistforcerelay to control forced relaying. [#7099
… redux]

89d113e Blacklist -whitelistalwaysrelay; replaced by -whitelistrelay. (Gregory Maxwell)
325c725 Add whitelistforcerelay to control forced relaying. (Gregory Maxwell)

laanwj added a commit that referenced this pull request Feb 1, 2016

Add whitelistforcerelay to control forced relaying. [#7099 redux]
- Add whitelistforcerelay to control forced relaying.

Also renames whitelistalwaysrelay.

Nodes relay all transactions from whitelisted peers, this
 gets in the way of some useful reasons for whitelisting
 peers-- for example, bypassing bandwidth limitations.

The purpose of this forced relaying is for specialized gateway
 applications where a node is being used as a P2P connection
 filter and multiplexer, but where you don't want it getting
 in the way of (re-)broadcast.

This change makes it configurable with whitelistforcerelay.

- Blacklist -whitelistalwaysrelay; replaced by -whitelistrelay.

Github-Pull: #7439
Rebased-From: 325c725 89d113e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment