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

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.

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.

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).

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?

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.

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?

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

Is there any reason to ever relay them?

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.

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).

@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?

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

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.

Needs rebase...

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

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

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