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

1. 
   gmaxwell commented at 11:10 pm on November 25, 2015: contributor

   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.

2. 
   gmaxwell commented at 11:12 pm on November 25, 2015: contributor

   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.

3. 
   sipa commented at 11:20 pm on November 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).

4. 
   sipa commented at 11:36 pm on November 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?
5. 
   gmaxwell commented at 11:40 pm on November 25, 2015: contributor

   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.

6. 
   dcousens commented at 0:22 am on November 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?

7. 
   gmaxwell commented at 0:32 am on November 26, 2015: contributor

   Invalid means that it is badly formatted, spends non-existing coins, scripts evaluate to false, etc.
8. 
   dcousens commented at 0:53 am on November 26, 2015: contributor

   Is there any reason to ever relay them?
9. 
   sipa commented at 0:57 am on November 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.
10. 
    gmaxwell commented at 0:59 am on November 26, 2015: contributor

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

11. 
    sipa commented at 1:03 am on November 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?
12. laanwj added the label P2P on Nov 26, 2015
13. 
    gmaxwell commented at 9:13 pm on November 26, 2015: contributor

    @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.
14. gmaxwell renamed this:
    Add whitelistforcerelay setting and default to off.
    Add whitelistforcerelay setting [PR changed to no longer default to off.]
    on Nov 28, 2015
15. 
    gmaxwell commented at 11:20 am on November 28, 2015: contributor

    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.
16. 
    sipa commented at 12:00 pm on November 28, 2015: member

    Needs rebase…
17. 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.

    c5b903af13
18. laanwj added the label Feature on Dec 3, 2015
19. 
    laanwj commented at 12:38 pm on January 28, 2016: member

    Needs rebase again (sorry) Also needs text for the 0.13 release notes as this is indeed a pretty big change.
20. 
    gmaxwell commented at 10:16 pm on January 28, 2016: contributor

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

21. 
    gmaxwell commented at 10:53 pm on January 28, 2016: contributor

    #7439 now, sorry. I couldn’t figure out how to update this with the branch having been deleted on my tree.
22. gmaxwell closed this on Jan 28, 2016

    ---

23. laanwj referenced this in commit 58a8574400 on Feb 1, 2016
24. laanwj referenced this in commit 86755bc85a on Feb 1, 2016
25. MarcoFalke locked this on Sep 8, 2021

Contributors
gmaxwell sipa dcousens laanwj

Labels
Feature P2P