NicolasDorier commented at 9:44 am on June 20, 2019: contributor

Motivation

In 0.19, bloom filter will be disabled by default. I tried to make a PR to enable bloom filter for whitelisted peers regardless of -peerbloomfilters.

Bloom filter have non existent privacy and server can omit filter’s matches. However, both problems are completely irrelevant when you connect to your own node. If you connect to your own node, bloom filters are the most bandwidth efficient way to synchronize your light client without the need of some middleware like Electrum.

It is also a superior alternative to BIP157 as it does not require to maintain an additional index and it would work well on pruned nodes.

When I attempted to allow bloom filters for whitelisted peer, my proposal has been NACKed in favor of a more flexible approach which should allow node operator to set fine grained permissions instead of a global whitelisted attribute.

Doing so will also make follow up idea very easy to implement in a backward compatible way.

Implementation details

The PR propose a new format for --white{list,bind}. I added a way to specify permissions granted to inbound connection matching white{list,bind}.

The following permissions exists:

ForceRelay
Relay
NoBan
BloomFilter
Mempool

Example:

-whitelist=bloomfilter@127.0.0.1/32.
-whitebind=bloomfilter,relay,noban@127.0.0.1:10020.

If no permissions are specified, NoBan | Mempool is assumed. (making this PR backward compatible)

When we receive an inbound connection, we calculate the effective permissions for this peer by fetching the permissions granted from whitelist and add to it the permissions granted from whitebind.

To keep backward compatibility, if no permissions are specified in white{list,bind} (e.g. --whitelist=127.0.0.1) then parameters -whitelistforcerelay and -whiterelay will add the permissions ForceRelay and Relay to the inbound node.

-whitelistforcerelay and -whiterelay are ignored if the permissions flags are explicitly set in white{bind,list}.

Follow up idea

Based on this PR, other changes become quite easy to code in a trivially review-able, backward compatible way:

Changing connect at rpc and config file level to understand the permissions flags.
Changing the permissions of a peer at RPC level.

fanquake added the label P2P on Jun 20, 2019

DrahtBot commented at 9:47 am on June 20, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16551 (test: Test that low difficulty chain fork is rejected by MarcoFalke)
#16548 (Make the global flag fDiscover an instance variable of CConnman by mmachicao)
#16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
#16273 (refactor: Remove unused includes by practicalswift)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15759 ([p2p] Add 2 outbound blocks-only connections by sdaftuar)
#14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

NicolasDorier force-pushed on Jun 20, 2019

NicolasDorier force-pushed on Jun 21, 2019

NicolasDorier renamed this:
~~[WIP] Make whitebind/whitelist permissions more flexible~~
Make whitebind/whitelist permissions more flexible
on Jun 21, 2019

NicolasDorier force-pushed on Jun 21, 2019

NicolasDorier commented at 4:57 pm on June 21, 2019: contributor

This PR is ready to be reviewed/merged.

in test/functional/p2p_permissions.py:61 in 5e899913bb outdated

56+        ["-whitelist=all@127.0.0.1"],
57+        ["forcerelay", "noban", "mempool", "bloomfilter", "relay"],
58+        True)
59+
60+        self.nodes[1].stop()
61+        self.nodes[1].wait_until_stopped()

AkioNak commented at 1:51 am on June 24, 2019:

nit: This 2 lines can be written as self.stop_node(1) .

NicolasDorier commented at 4:58 am on June 24, 2019:

thanks! fixed.

NicolasDorier force-pushed on Jun 24, 2019

in src/net_permissions.cpp:5 in f96d0524a9 outdated

0@@ -0,0 +1,144 @@
1+#include <net_permissions.h>

ajtowns commented at 3:26 am on June 25, 2019:

Missing copyright headers?

in src/net_permissions.h:16 in f96d0524a9 outdated

 7+enum CNetPermissionFlags
 8+{
 9+    PF_NONE = 0,
10+    // Can query bloomfilter even if -peerbloomfilters is false
11+    PF_BLOOMFILTER = (1U << 1),
12+    // Always relay transactions, even if already in mempool or rejected from policy

ajtowns commented at 3:31 am on June 25, 2019:

“relay transactions from this peer” ? (as opposed to relay them to the peer)

in src/net.h:664 in f96d0524a9 outdated

658@@ -657,6 +659,10 @@ class CNode
659      */
660     std::string cleanSubVer GUARDED_BY(cs_SubVer){};
661     bool m_prefer_evict{false}; // This peer is preferred for eviction.
662+    CNetPermissionFlags permissionFlags{PF_NONE};

ajtowns commented at 4:00 am on June 25, 2019:

Should CNode::permissionFlags be private instead of public?

NicolasDorier commented at 5:43 am on June 25, 2019:

Actually we can’t because we are affecting it from outside of the function. You think we should add the flag as a parameter to the ctor?

ajtowns commented at 6:49 am on June 25, 2019:

I tried:

     bool m_prefer_evict{false}; // This peer is preferred for eviction.
+private:
     CNetPermissionFlags permissionFlags{PF_NONE};
+public:
     bool HasPermission(CNetPermissionFlags permission) const {

and it compiled fine for me? CConnman is a friend class already, so it’s functions can still play with it.

in src/net.cpp:66 in f96d0524a9 outdated

62@@ -62,8 +63,7 @@ static constexpr int DUMP_PEERS_INTERVAL = 15 * 60;
63 enum BindFlags {
64     BF_NONE         = 0,
65     BF_EXPLICIT     = (1U << 0),
66-    BF_REPORT_ERROR = (1U << 1),
67-    BF_WHITELIST    = (1U << 2),
68+    BF_REPORT_ERROR = (1U << 1)

ajtowns commented at 4:01 am on June 25, 2019:

No need to remove the trailing comma, it’s not javascript :)

in src/net.cpp:995 in f96d0524a9 outdated

986     pnode->AddRef();
987-    pnode->fWhitelisted = whitelisted;
988+    pnode->permissionFlags = permissionFlags;
989+    // If the permission flags are a superset of the default flags, then we have the expected
990+    // behavior of the old fWhitelisted
991+    pnode->fWhitelisted = pnode->HasPermission(PF_DEFAULT);

ajtowns commented at 4:24 am on June 25, 2019:

Could drop fWhiltelisted from CNode and replace it in CNodeStats with

bool IsWhitelisted() const { return (permissionFlags & PF_DEFAULT) == PF_DEFAULT; }

NicolasDorier commented at 6:03 am on June 25, 2019:

I don’t think it should be replaced. I think it should be completely removed when we decide to not put the flag in getpeerinfo.

ajtowns commented at 6:46 am on June 25, 2019:

Well, could just put the logic in directly in getpeerinfo too – it’s also in RPCConsole::updateNodeDetail at present. If you’re redoing all the logic to not use fWhitelist, maybe it already makes sense to drop it from the getpeerinfo though?

NicolasDorier commented at 10:00 am on June 25, 2019:

@ajtowns good point. I don’t know if it should be dropped, I expect lot’s of people depending on it. But you are right, no need to pass around the fWhitelisted and I can move that in getpeerinfo.

in src/rpc/net.cpp:189 in f96d0524a9 outdated

184@@ -184,6 +185,11 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
185             obj.pushKV("inflight", heights);
186         }
187         obj.pushKV("whitelisted", stats.fWhitelisted);
188+        UniValue permissions(UniValue::VARR);
189+        for (const auto permission : CNetPermissions::ToStrings(stats.permissionFlags)) {

ajtowns commented at 4:34 am on June 25, 2019:

Should be const auto& (or const std::string&) I think

in src/net.cpp:910 in f96d0524a9 outdated

903@@ -900,7 +904,21 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
904         }
905     }
906 
907-    bool whitelisted = hListenSocket.whitelisted || IsWhitelistedRange(addr);
908+    CNetPermissionFlags permissionFlags = hListenSocket.permissions;
909+    permissionFlags = static_cast<CNetPermissionFlags>(permissionFlags | GetWhitelistPermissionFlags(addr));
910+    bool noban = (permissionFlags & CNetPermissionFlags::PF_NOBAN) != 0;
911+    bool allowLegacy = (permissionFlags & CNetPermissionFlags::PF_ISDEFAULT) != 0;

ajtowns commented at 4:41 am on June 25, 2019:

Is the logic there right? allowLegacy is true if either hListenSocket has default whitelist perms or the addr has default whitelist perms. Shouldn’t it be false if hListenSocket has default whitelist perms (due to whitebind) but addr has whitelist perms other than default (ie, not PF_NONE but not PF_ISDEFAULT either)?

NicolasDorier commented at 5:36 am on June 25, 2019:

So PF_ISDEFAULT in this case means that either whitebind or whitelist does not use @ syntax to define specific permissions. (PF_ISDEFAULT does not mean that the default perm, contrary to PF_DEFAULT, are being used, it just mean that we want to keep legacy behavior. I think I should rename this flag, this is confusing)

To keep backward compatibility if either of them use the old syntax, you want to take into account -whiterelay and -whiteforcerelay. I think the logic is right.

NicolasDorier commented at 5:56 am on June 25, 2019:

Will rename PF_ISDEFAULT to PF_ISIMPLICIT

in src/net.cpp:907 in f96d0524a9 outdated

903@@ -900,7 +904,21 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
904         }
905     }
906 
907-    bool whitelisted = hListenSocket.whitelisted || IsWhitelistedRange(addr);
908+    CNetPermissionFlags permissionFlags = hListenSocket.permissions;
909+    permissionFlags = static_cast<CNetPermissionFlags>(permissionFlags | GetWhitelistPermissionFlags(addr));

ajtowns commented at 5:03 am on June 25, 2019:

I think adding:

inline bool HasNetPermissionFlag(CNetPermissionFlags flags, CNetPermissionFlags f)
{
    return (flags & f) == f;
}
inline void AddNetPermissionFlag(CNetPermissionFlags& flags, CNetPermissionFlags f)
{
    flags = static_cast<CNetPermissionFlags>(flags | f);
}
inline void ClearNetPermissionFlag(CNetPermissionFlags& flags, CNetPermissionFlags f)
{
    flags = static_cast<CNetPermissionFlags>(flags | ~f);
}

makes using the NetPermFlags a bit clearer fwiw.

NicolasDorier commented at 7:06 am on June 25, 2019:

Not against it, but wouldn’t it better to have a templated version somewhere else?

Where could I put such utility function?

ajtowns commented at 7:30 am on June 25, 2019:

Or template the bitwise ops directly, maybe – http://blog.bitwigglers.org/using-enum-classes-as-type-safe-bitmasks/ ? Doesn’t seem like there’s an existing header file that would fit into to me.

NicolasDorier commented at 9:50 am on June 25, 2019:

It seems to me such change should be better done globally across the project separately, this is not trivial to review.

ajtowns commented at 11:53 am on June 25, 2019:

I don’t think there’s much call for it elsewhere, there’s just a couple of static_cast for SIGHASH in core_write.cpp and one for ServiceFlags in net.cpp, and some in leveldb. Going by:

$ grep 'static_cast<[^>*&]*>[(][^)]*[|&~^]' -rI | grep -v qt.*moc | grep -v paymentrequest.pb.cc | grep -v CNetPermissionFlags

NicolasDorier commented at 12:40 pm on June 27, 2019:

fair enough, doing it for net_permissions then

ajtowns commented at 5:10 am on June 25, 2019: member

ApproachACK – looked over the code, checked it compiled, haven’t even run the tests though. Having the code check for !pfrom-HasPermission(PF_RELAY) seems an improvement on !pfrom->Whitelisted || !gArgs.GetBoolArg(...).

NicolasDorier force-pushed on Jun 25, 2019

NicolasDorier commented at 9:52 am on June 25, 2019: contributor

Because forcerelay should set the permission relay as well, I added some tests around it. I also added a test to see if the permissions get combined correctly between whitelist and whitebind.

NicolasDorier force-pushed on Jun 25, 2019

MarcoFalke commented at 4:20 pm on June 25, 2019: member

Looks like the gui does not compile

0qt/rpcconsole.cpp:1123:51: error: ‘const class CNodeStats’ has no member named ‘fWhitelisted’

NicolasDorier force-pushed on Jun 26, 2019

NicolasDorier commented at 6:34 am on June 26, 2019: contributor

@MarcoFalke thanks, fixed.

Travis fails on unrelated test wallet_encryption, a kick-in-the-box can effectively fix it. Is it possible to run travis again?

in src/net.h:320 in 40bdd96e2f outdated

316@@ -315,14 +317,14 @@ class CConnman
317 private:
318     struct ListenSocket {
319         SOCKET socket;
320-        bool whitelisted;
321+        CNetPermissionFlags permissions;

MarcoFalke commented at 1:10 pm on June 26, 2019:

nit with regard to coding style:

Newly introduced classes don’t have the C prefix
Members start with m_, m_snake_case
if statements are either in one line or the code blocks are wrapped in {}.

0        NetPermissionFlags m_permissions;

Also, you may use clang-format-diff and yapf-diff. https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md#writing-code

laanwj commented at 11:14 am on June 27, 2019: member

Concept ACK, thanks for working on this. I think un-bundling “whitelisting” is something long needed.

NicolasDorier force-pushed on Jun 27, 2019

NicolasDorier commented at 2:19 pm on June 27, 2019: contributor

I refactored to follow the guidelines, making things a bit more readable as @MarcoFalke suggested. I removed some bitwise magic like @ajtowns suggested.

The only point that I did not do: I use enum over enum class. There is no easy to read way to make flags with enum class. :(

NicolasDorier commented at 2:58 pm on June 27, 2019: contributor

Now enforcing ForceRelay => Relay at the enum level. It makes the code more obviously correct.

NicolasDorier force-pushed on Jun 27, 2019

NicolasDorier commented at 2:45 am on June 28, 2019: contributor

Ok fully addressed all the nits.

NicolasDorier commented at 3:14 am on June 29, 2019: contributor

btw, as a follow up PR, I may add a RPC command to change the peer’s permission. I often needed that. I remember @jnewbery tried to do it a while ago for whitelisting a peer.

NicolasDorier commented at 3:29 am on June 29, 2019: contributor

Another idea for follow up PR: allowing the use of permissions for connect. (both at config and RPC level) in a backward compatible way.

NicolasDorier force-pushed on Jun 29, 2019

NicolasDorier commented at 4:31 am on June 29, 2019: contributor

@ajtowns I reverted the decision of having a IsLegacyWhitelist() method instead of passing around the old fWhitelisted. Instead I renamed fWhitelisted to m_legacyWhitelisted to prevent people from using it.

The previous logic was, if the permissions granted were including noban | mempool then fwhitelisted must be true, so removing fwhitelisted and replacing it with a single IsLegacyWhitelist in NodeStats made sense.

However, I thought that later on, if RPC command connect support permission flags, and that no flags are passed, I don’t want to interprete it as noban | mempool permissions, and even if noban and mempool are specified, I would not want the IsLegacyWhitelist to return true.

Reverting from IsLegacyWhitelisted to m_legacyWhitelisted means I am sure that a peer is considered whitelisted ONLY in the narrow case of:

Inbound connection
No permissions specified in either white{list,bind}

So now the permission parsing code does not make assumption about which permission to set if the user did not specified any flag. (ISIMPLICIT)

in src/net_permissions.cpp:35 in a4ec0818ca outdated

30+            else if (permission == "mempool") NetPermissions::AddFlag(flags, PF_MEMPOOL);
31+            else if (permission == "all") NetPermissions::AddFlag(flags, PF_ALL);
32+            else if (permission == "relay") NetPermissions::AddFlag(flags, PF_RELAY);
33+            else if (permission.length() == 0); // Allow empty entries
34+            else {
35+                if (error != NULL) *error = strprintf(_("Invalid P2P permission: '%s'"), permission);

practicalswift commented at 9:32 am on July 2, 2019:

Nit: Use nullptr instead of NULL (see rationale in developer notes). Applies throughout PR :-)

in src/net_permissions.cpp:21 in a4ec0818ca outdated

16+    }
17+    else {
18+        size_t offset = 0;
19+        const auto permissions = str.substr(0, permissionSeparator);
20+        while (offset < permissions.length()) {
21+            const int permissionSeparator = permissions.find(',', offset);

practicalswift commented at 9:33 am on July 2, 2019:

This permissionSeparator shadows permissionSeparator in the outer context.

in src/net_permissions.cpp:9 in a4ec0818ca outdated

0@@ -0,0 +1,99 @@
1+// Copyright (c) 2009-2018 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <net_permissions.h>
6+#include <util/system.h>
7+#include <netbase.h>
8+
9+bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, std::string* error)

practicalswift commented at 9:36 am on July 2, 2019:

Use std::string& error instead of std::string* error (see recommendation in developer notes).

Nit: Use another variable name to avoid shadowing error included from util/system.h.

NicolasDorier commented at 8:48 am on July 3, 2019:

I was using pointer because I don’t want to burden the user to retrieve the error. But meh… everywhere we use it we need the error so I will change to ref.

in src/net.h:322 in b7d1b9f470 outdated

322         SOCKET socket;
323-        bool whitelisted;
324-
325-        ListenSocket(SOCKET socket_, bool whitelisted_) : socket(socket_), whitelisted(whitelisted_) {}
326+        inline void AddSocketPermissionFlags(NetPermissionFlags& flags) const { NetPermissions::AddFlag(flags, m_permissions); }
327+        ListenSocket(SOCKET socket_, NetPermissionFlags permissions_) : socket(socket_), m_permissions(permissions_) {}

mzumsande commented at 4:13 pm on July 2, 2019:

This gives me compiler warnings (“warning: ‘CConnman::ListenSocket::socket’ will be initialized after [-Wreorder] SOCKET socket;”), suggest to change the order somewhere (in the constructor or in the struct).

in src/test/netbase_tests.cpp:333 in b7d1b9f470 outdated

328+    NetWhitebindPermissions whitebindPermissions;
329+    NetWhitelistPermissions whitelistPermissions;
330+
331+    // Detect invalid white bind
332+    BOOST_CHECK(!NetWhitebindPermissions::TryParse("", whitebindPermissions, &error));
333+    BOOST_CHECK(error.find("Cannot resolve -whitebind address") != -1);

mzumsande commented at 4:14 pm on July 2, 2019:

Suggest to use std::string::npos instead of -1 here and below (prevents “comparison between signed and unsigned” compiler warnings).

jnewbery commented at 6:50 pm on July 2, 2019: member

Big concept ACK. The whitelist logic should really be unbundled and rationalized. I’m not sold on the approach though. Adding structure to the bitcoin.conf file for certain options requires a bunch of custom parsing code to be added and maintained, and introduces complexity for users.

One alternative approach would be to build on the work that @ryanofsky has done here: #15935 and specify the whitelist config as json, either in the settings.json file or as a separate peers.json (or similar) file. The advantages to that approach are:

no custom parsing code needs to be reviewed/maintained, since it’s all contained in the univalue library that we already use.
the json format is widely used and understood, so users don’t need to learn a new config format
(with #15935), bitcoind can write to json, which means very little additional code is needed for the whitelist settings to be writable at runtime and persist across restarts (as suggested here: #16248 (comment))

NicolasDorier commented at 6:22 am on July 3, 2019: contributor

The parsing logic is contained, easy to review and widely tested. (100% code coverage)

Adding structure to the bitcoin.conf file for certain options requires a bunch of custom parsing code to be added and maintained, and introduces complexity for users.

I disagree, I want to use this feature myself, and just extending the parsing code of whitebind and whitelist is the easiest way for me to use it. Having separate configuration file via JSON is more difficult.

the json format is widely used and understood, so users don’t need to learn a new config format

With JSON they must know JSON and the flags. With my solution, they only need to know the flags and ,.

sipa commented at 6:27 am on July 3, 2019: member

I haven’t thought enough about the separate config file idea, and what kind of settings ought to go where to have a strong opinion.

However, what about this: permitting individual settings (like the whitelisting ones) to have JSON values as values? (anywhere, whether specified from bitcoin.conf or the command line). That would equally avoid custom per-option parsing logic, without spreading out what gets stored where.

NicolasDorier commented at 8:42 am on July 3, 2019: contributor

I personally prefer the parsing method. The code is simple and does not require lot’s of maintenance. Also I prefer reading -whitebind=bloom,noban@127.0.0.1:2727 than -whitebind={interface:"127.0.0.1:2727",bloom:"true", noban:"true"}

It is overkill to use json for a comma separated list when a dumb split saves the day.

NicolasDorier force-pushed on Jul 3, 2019

NicolasDorier commented at 11:32 am on July 3, 2019: contributor

Addressed nits of @mzumsande and @practicalswift. I added some comments on the flag parsing method.

jnewbery commented at 10:26 pm on July 3, 2019: member

I still think the configuration serialization should be designed with the longer-term goal of making this config updatable at runtime and persistent across restarts. For that, the bitcoin.conf file is not suitable (since bitcoind can’t write to it), so we either:

come up with our own serialization format, and write custom code that can parse and write to that format, then test, review and maintain that code and fix the bugs and corner cases that come up.
use an existing and well-supported serialization format like JSON, yaml, toml, etc. We already have support for JSON in Bitcoin Core, so it seems the path of least resistance to use that.

We are going to want to make this setting updatable at runtime, so I don’t think we should use a format now that can’t be written to by bitcoind without writing a bunch more code.

NicolasDorier commented at 8:57 am on July 4, 2019: contributor

@jnewbery I don’t believe that the goal of having a new json object conflict with this PR.

As a user, I have no use for a long term json file for configuring permissions, it will cause me more trouble than anything over this solution, so I will not work on it. (I will need change to my dockerfiles to support putting permissions files in the right folder, and build the file via bash script, instead of just adding -whitebind=noban,mempool,bloom@127.0.0.1:8333 to my config file)

This PR does not prevent you from later having a json file if you wish, it will just need to compute the union of permissions granted by the different methods.

It is also 100% backward compatible, so people not wanting to take advantage of the syntax of this PR can just simply ignore it.

The parsing code is too simple to be of any concern of maintainability.

jnewbery commented at 12:33 pm on July 4, 2019: member

This PR does not prevent you from later having a json file if you wish, it will just need to compute the union of permissions granted by the different methods.

It is also 100% backward compatible, so people not wanting to take advantage of the syntax of this PR can just simply ignore it.

This seems confusing. Having multiple ways for config to be specified leads to confusing interactions. For example, if we just take the union of permissions, then I could start bitcoind with -whitebind=noban,mempool,bloom@127.0.0.1:8333, then decide that 127.0.0.1:8333 doesn’t need bloom permissions and disable it with an RPC call. When bitcoind restarts, then the permission reappears.

NicolasDorier commented at 1:16 pm on July 4, 2019: contributor

@jnewbery most likely people will not use both at the same time. This is concern for a corner case.

This can be solved by later in two ways:

Either no runtime permission can be changed if white{bind/list} of the peer have specific permissions set.
Either the permissions set at runtime should not be persisted, leaving to the client the responsability to override peer’s permission when it detects it got connected.

ajtowns commented at 1:57 am on July 5, 2019: member

It could also be resolved by having the dynamic permissions override the static config. This is something that would need to be resolved even if both bitcoin.conf and settings.json were in json format, though.

sipa commented at 2:32 am on July 5, 2019: member

@NicolasDorier Also consider that in the future we may want to extend whitelisting with more features like bandwidth limiting, or restricting to bip150-like authenticated peers. I think keeping things extensible may be hard with a custom format like this.

NicolasDorier commented at 4:38 am on July 5, 2019: contributor

@sipa I see, sadly I don’t have time to work on something of this scope.

I wished to do this PR because I only want one simple thing: Allow bloom filters for whitelisted peers as bloom filter will be off by default in the next release.

BTCPayServer is exposing a whitebind accessible via a (confidential) Tor3 address that other wallets like Wasabi wallet, Bisq or any wallet can connect to.

I need bloom filter on this interface, as bloom filter is the best way to sync your own wallet without wasting bandwidth. Without a solution in the next release, it means I need to explicitly activate bloom filter even for non whitelisted peer, and I don’t want this as it exposes well known DDoS vectors.

I think that until there is a concrete proposal with the full scope of what such JSON file will cover and its format, we should not block a feature that is useful now and that is not a maintenance burden.

100% of this PR is entirely compatible with any other JSON proposal. (the code verifying the permissions does not assume it comes from my custom format)

In the case later you do such proposal, it is very easy to ignore the JSON if ever my custom format is in use so we don’t have non obvious interactions.

The parsing code is not a problem to maintain, look at the code, this is nothing more than a split on @ followed by a split on ,. The part converting flags into string is useful in any case for both proposal.

If really you are against this PR and opt for a JSON, I will close this PR as I really don’t want to spend month of bike-shedding on the format of such file in the expectation it get merged in 0.21.

sipa commented at 4:59 am on July 5, 2019: member

I’m not suggesting you doing anything around a separate json config file, nor around any of the other future features. I just suspect it’d be much easier to just use JSON in the existing whitebind config setting rather than a custom parser, especially if we suspect it will be extended later with additional options.

NicolasDorier commented at 7:41 am on July 5, 2019: contributor

@sipa so basically you mean having --whitebind=[noban,bloom]@127.0.0.1:8333 instead of --whitebind=noban,bloom@127.0.01:8333 ?

NicolasDorier commented at 7:51 am on July 5, 2019: contributor

Well not against it but I don’t see the point too much.

It will not make the code simple and more compact, it just replace a couple of split by some univalue parsing. In terms of line of code this will be exactly the same.
Later, if you want to do another format you could consider doing it by using $ instead of @ as separator.

Though if it is really all that is needed to get ACKs, I would do.

NicolasDorier commented at 1:35 pm on July 10, 2019: contributor

@sipa can you tell me more specifically what you want?

--whitebind=["noban","bloom"]@127.0.0.1:8333
--whitebind={permissions: ["noban","bloom"], address: "127.0.0.1:8333"}
--whiterules={rule: [{permissions: ["noban","bloom"], bind: "127.0.0.1:8333"}]}

?

NicolasDorier commented at 1:36 pm on July 10, 2019: contributor

@jnewbery please anything more specific than “we want JSON based stuff” would help me.

JSON format whatever…

MarcoFalke commented at 1:46 pm on July 10, 2019: member

Just to add to the list of options:

-whitebind={"127.0.0.1:8338" : ["noban","bloom"] , "127.0.0.2" : []}

NicolasDorier commented at 8:06 am on July 11, 2019: contributor

@MarcoFalke you mean as an additional way to configure it, keeping the noban,bloom@127.0.0.1:8338 format? I wish we keep the current format really because dynamically creating json files (like adding rules) is bash is a pain in the ass.

I have the case where different process can modify the bitcoin config, all being able to add their own whitebind (like is the case currently), editing such JSON will be a major pain over just adding --whitebind="noban,bloom@127.0.0.1:8338"

laanwj commented at 6:50 pm on July 11, 2019: member

JSON on the command line? I’m not sure. That would be a first. I feel like there’s a lot of scope creep going on here. I think this PR should be focused on adding this functionality (maybe we can even have it for 0.19?), not on redefining how options are parsed. That can be discussed somewhere else.

NicolasDorier commented at 10:06 am on July 12, 2019: contributor

I agree, but would do anything for ACKs to get that into 0.19.0, I would clean the toilet (hell, even eat veggies) to get ACKs rather than enabling bloom filter on 0.19.0 for on btcpay installs by default.

laanwj approved

laanwj commented at 9:22 am on July 16, 2019: member

code review ACK da6d750f5864836ecb3f3fe3f6e7e448e4342f14

laanwj added this to the milestone 0.19.0 on Jul 16, 2019

DrahtBot commented at 6:15 pm on July 24, 2019: member

DrahtBot added the label Needs rebase on Jul 24, 2019

NicolasDorier force-pushed on Jul 26, 2019

NicolasDorier commented at 3:56 am on July 26, 2019: contributor

rebased

fanquake removed the label Needs rebase on Jul 26, 2019

NicolasDorier commented at 8:18 am on July 26, 2019: contributor

Test failing, but probably unrelated to this PR.

00 stderr bitcoind: validation.cpp:4526: void CChainState::CheckBlockIndex(const Consensus::Params &): Assertion `setBlockIndexCandidates.count(pindex)' failed. [0m

mzumsande commented at 8:36 am on July 26, 2019: member

yes, it happens in other branches too, see #16444.

MarcoFalke commented at 1:28 pm on August 6, 2019: member

unsigned Approach ACK 1ad7106670344735677e958c35efa81d5d04126e

Sjors commented at 2:38 pm on August 6, 2019: member

Concept ACK. Maybe tag with 0.19 given #16152?

I think the simple noban,bloom@127.0.0.1:8338 format is good enough. The ability to offer bloom filters to selected peers seems worth the extra complexity, and it reduces confusion of what -whitelist currently does.

I suggest that we keep the -whitelist argument simple, and use a future RPC for more complex rules. Such an RPC call could refuse to touch entries that are in the config file, and perhaps -whitelist could be deprecated eventually.

The parser looks simple enough to me (and I don’t write parsers for a living).

I’m not a fan of an extra config file, partially because JSON is tedious to write. If you have to use some tool to produce config files, then you might as well use the RPC.

Getting a bunch of compiler warnings on macOS for the tests:

0In file included from test/net_tests.cpp:13:
1./net.h:324:73: warning: field 'socket' will be initialized after field 'm_permissions' [-Wreorder]
2        ListenSocket(SOCKET socket_, NetPermissionFlags permissions_) : socket(socket_), m_permissions(permissions_) {}
3                                                                        ^
41 warning generated.
5  CXX      test/test_bitcoin-txindex_tests.o
6  CXX      test/test_bitcoin-txvalidation_tests.o
7test/netbase_tests.cpp:379:54: warning: comparison of integers of different signs: 'std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
8    BOOST_CHECK(error.find("Invalid P2P permission") != -1);
9                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~

In QT too:

0In file included from ./qt/bantablemodel.h:8:
1./net.h:324:73: warning: field 'socket' will be initialized after field 'm_permissions' [-Wreorder]
2        ListenSocket(SOCKET socket_, NetPermissionFlags permissions_) : socket(socket_), m_permissions(permissions_) {}
3                                                                        ^

I get a Mempool sync timed out for wallet_listtransactions.py line 179, which suggests something did change in the default behavior. p2p_sendheaders.py also failed for me during the first run at Part 5.

NicolasDorier commented at 9:52 am on August 7, 2019: contributor

@Sjors I will rebase, this was based on a commit where test has been broken for some reason. Will fix the warnings as well.

NicolasDorier force-pushed on Aug 7, 2019

NicolasDorier commented at 3:29 pm on August 7, 2019: contributor

Rebased, tests passing @Sjors (Warnings should be fixed)

Sjors commented at 8:59 am on August 8, 2019: member

Almost, still get this one:

0net_permissions.cpp:28:38: warning: comparison of integers of different signs: 'const int' and 'const typename basic_string<char, char_traits<char>, allocator<char> >::size_type' (aka 'const unsigned long') [-Wsign-compare]
1            int len = commaSeparator == std::string::npos ? permissions.length() - readen : commaSeparator - readen;
2                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~
3net_permissions.cpp:32:32: warning: comparison of integers of different signs: 'const int' and 'const typename basic_string<char, char_traits<char>, allocator<char> >::size_type' (aka 'const unsigned long') [-Wsign-compare]
4            if (commaSeparator != std::string::npos) readen++; // We read ","
5                ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~
62 warnings generated.

The first commit is rather large. Consider introducing TryParsePermissionFlags in a separate commit, and testing it directly instead of, or in addition to, indirectly via NetWhite[list/bind]Permissions. And another commit where you switch to using NetWhite[list/bind]Permissions::TryParse, but don’t process the flags yet.

There are a bunch of existing functional tests that use -whitelist=, exploiting various features. Maybe add a commit that switches these tests to use the minimum set of flags they need? That provides some additional assurance that these flags work as expected. In p2p_relay.py where -whitelistrelay and -whitelistforcerelay are tested, those tests could be duplicated to check that equivalent flag works (with global -whitelistrelay=0).

laanwj commented at 11:29 am on August 8, 2019: member

IMO improving the tests can be done in a follow-up PR let’s get rid of the warning though

MarcoFalke commented at 6:11 pm on August 8, 2019: member

Agree with @laanwj. Lets switch to the new logic in tests in a follow up. I am happy to do that, but I’d rather not have this pull grow even more.

Make whitebind/whitelist permissions more flexible e5b26deaaa

Do not disconnect peer for asking mempool if it has NO_BAN permission ecd5cf7ea4

Replace the use of fWhitelisted by permission checks d541fa3918

Add functional tests for flexible whitebind/list c5b404e8f1

NicolasDorier commented at 2:34 am on August 11, 2019: contributor

@Sjors if you look the C++ tests I am testing it very strongly with all kind of edge cases. It does not make sense to test the parse flag in isolation as it is not used in the code that way.

I should have fixed the warnings now. I am unsure though because I am developing on windows and msvc says it is good.

NicolasDorier force-pushed on Aug 11, 2019

laanwj commented at 3:06 pm on August 14, 2019: member

re-ACK c5b404e8f1973afe071a07c63ba1038eefe13f0f

laanwj merged this on Aug 14, 2019

laanwj closed this on Aug 14, 2019

laanwj referenced this in commit 67be6d7a17 on Aug 14, 2019

in src/net.cpp:914 in c5b404e8f1

910+    const bool noban = NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN);
911+    bool legacyWhitelisted = false;
912+    if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_ISIMPLICIT)) {
913+        NetPermissions::ClearFlag(permissionFlags, PF_ISIMPLICIT);
914+        if (gArgs.GetBoolArg("-whitelistforcerelay", false)) NetPermissions::AddFlag(permissionFlags, PF_FORCERELAY);
915+        if (gArgs.GetBoolArg("-whitelistrelay", false)) NetPermissions::AddFlag(permissionFlags, PF_RELAY);

MarcoFalke commented at 6:39 pm on August 14, 2019:

Why are the fallbacks set to false and not the DEFAULT_ value?

NicolasDorier commented at 7:14 am on August 15, 2019:

Because the constant is in a different file which is not in imported into the scope of this file.

Looking at other GetBoolArg references in this file, you can notice that all of them have the same issue, none of them reference DEFAULT_

MarcoFalke commented at 2:16 pm on August 15, 2019:

So is it safe to do that? Under what circumstances would it lead to bugs?

MarcoFalke commented at 1:42 pm on August 16, 2019:

I guess you are fixing it in #16631?

NicolasDorier commented at 1:44 pm on August 16, 2019:

@MarcoFalke yes, I misunderstood your question. I thought you were asking me to use the DEFAULT_ const instead of hard coding the value. But you were saying that the DEFAULT_ is not the same as what I hardcoded.

in src/net.cpp:909 in c5b404e8f1

902@@ -904,7 +903,20 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
903         }
904     }
905 
906-    bool whitelisted = hListenSocket.whitelisted || IsWhitelistedRange(addr);
907+    NetPermissionFlags permissionFlags = NetPermissionFlags::PF_NONE;
908+    hListenSocket.AddSocketPermissionFlags(permissionFlags);
909+    AddWhitelistPermissionFlags(permissionFlags, addr);
910+    const bool noban = NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::PF_NOBAN);

MarcoFalke commented at 6:41 pm on August 14, 2019:

Why is this checked before the flag is set in line 916?

NicolasDorier commented at 7:18 am on August 15, 2019:

Thanks to report this, I will make a PR fixing it.

NicolasDorier commented at 7:37 am on August 15, 2019:

https://github.com/bitcoin/bitcoin/pull/16618

in src/net_permissions.cpp:74 in c5b404e8f1

69+    if (!TryParsePermissionFlags(str, flags, offset, error)) return false;
70+
71+    const std::string strBind = str.substr(offset);
72+    CService addrBind;
73+    if (!Lookup(strBind.c_str(), addrBind, 0, false)) {
74+        error = strprintf(_("Cannot resolve -%s address: '%s'").translated, "whitebind", strBind);

MarcoFalke commented at 7:04 pm on August 14, 2019:

Why is this not using ResolveErrMsg like before?

NicolasDorier commented at 7:21 am on August 15, 2019:

Because it is defined in init.cpp

MarcoFalke commented at 2:10 pm on August 15, 2019:

Fixed in #16620

in src/net_processing.cpp:3795 in c5b404e8f1

3791@@ -3786,7 +3792,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3792             pto->vInventoryBlockToSend.clear();
3793 
3794             // Check whether periodic sends should happen
3795-            bool fSendTrickle = pto->fWhitelisted;
3796+            bool fSendTrickle = pto->HasPermission(PF_NOBAN);

MarcoFalke commented at 7:17 pm on August 14, 2019:

Why is this noban and not relay?

NicolasDorier commented at 7:23 am on August 15, 2019:

To keep backward compatibility.

Before, if the peer was whitelisted, it did not matter whether he was also in whitelistrelay, this would be still be true in all cases.

MarcoFalke commented at 1:56 pm on August 15, 2019:

But relay is also set to true for backward compatibility, no?

NicolasDorier commented at 4:25 am on August 16, 2019:

only if the peer is also in -whitelistrelay , else it is not.

MarcoFalke commented at 1:18 pm on August 16, 2019:

Which is enabled by default for exactly that reason, no?

NicolasDorier commented at 2:31 pm on August 16, 2019:

Well, regardless, the old code does not look -whitelistrelay is 1 or 0 to determine fSendTrickle.

This would be a breaking change to make it depends on it. I am quite neutral to that, I am unsure what fSendTrickle is doing, but I think for this PR it should preserve the old behavior.

Another alternative is to have a specific permission just for that and turn it on if no specific permission are set regardless of -whitelistrelay

sdaftuar commented at 3:20 pm on October 22, 2019:

I just encountered this piece of code and found this to be pretty confusing. It seems to me that the behavior of relaying transactions immediately without waiting for our poisson timers to fire should be its own “net permission”, and not piggy-backed onto NOBAN.

MarcoFalke commented at 3:38 pm on October 22, 2019:

According to @NicolasDorier it is needed for backward compatibility. However, it should still be possible to split up into its own permission while retaining backward compatibility.

Sjors commented at 4:17 pm on October 22, 2019:

If we want to spilt this permission, it’s best to do that before 0.19 is final.

MarcoFalke commented at 4:43 pm on October 22, 2019:

Splitting it would be a new feature, so it might be a bit late for 0.19

in src/net.cpp:979 in c5b404e8f1

973@@ -962,9 +974,15 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
974     uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();
975     CAddress addr_bind = GetBindAddress(hSocket);
976 
977-    CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addr, CalculateKeyedNetGroup(addr), nonce, addr_bind, "", true);
978+    ServiceFlags nodeServices = nLocalServices;
979+    if (NetPermissions::HasFlag(permissionFlags, PF_BLOOMFILTER)) {
980+        nodeServices = static_cast<ServiceFlags>(nodeServices | NODE_BLOOM);

MarcoFalke commented at 7:19 pm on August 14, 2019:

Where is this from and why is it needed? I couldn’t find it in previous code

NicolasDorier commented at 7:20 am on August 15, 2019:

The initial problem I wanted to solve is that, because in 0.19 we remove BLOOM_FILTER by default, I wanted the ability to keep it only for whitelisted peer. Because this was a simple change, I decided to add it here instead of separate PR.

MarcoFalke commented at 1:55 pm on August 15, 2019:

Ah, I figured. Was a bit weird to see this behavior change in a “refactoring” commit

in test/functional/p2p_permissions.py:42 in c5b404e8f1

37+        True)
38+
39+        # Let's make sure permissions are merged correctly
40+        # For this, we need to use whitebind instead of bind
41+        # by modifying the configuration file.
42+        ip_port = "127.0.0.1:{}".format(p2p_port(1))

MarcoFalke commented at 7:27 pm on August 14, 2019:

Doesn’t p2p_port(1) return the port of node 1, so in the config of node 1 you are whitelisting itself? Dumb question, but I can’t see how this test passes.

NicolasDorier commented at 7:29 am on August 15, 2019:

Because we are using whitebind. This basically mean: Consider anybody connecting to this port as “whitelisted”.

Because node 0 connect to node 1, this is the right port.

MarcoFalke commented at 1:55 pm on August 15, 2019:

Thanks, makes sense.

MarcoFalke commented at 7:28 pm on August 14, 2019: member

Concept ACK.

Some dumb questions, since I have difficulties understanding the code.

Sjors commented at 11:53 am on August 15, 2019: member

There should be a followup to document the new syntax in bitcoind help (gArgs.AddArg("whitelist=<IP address or network>).

NicolasDorier commented at 1:07 pm on August 15, 2019: contributor

@Sjors I can do that, though I am unsure where is the best place for me to document properly this. I think the full description might be too long for the command line?

Sjors commented at 1:54 pm on August 15, 2019: member

There are other command line arguments with multi-line explanations, so I don’t think that’s a huge issue:

MarcoFalke referenced this in commit 21a165325e on Aug 16, 2019

MarcoFalke referenced this in commit b80cdfec9a on Aug 16, 2019

MarcoFalke referenced this in commit adff8fe321 on Aug 26, 2019

sidhujag referenced this in commit e1511ab3e2 on Aug 27, 2019

luke-jr referenced this in commit e0774606bd on Sep 3, 2019

luke-jr referenced this in commit a4ee744266 on Sep 3, 2019

luke-jr referenced this in commit 6ed5258a07 on Sep 3, 2019

luke-jr referenced this in commit 4d451e94fd on Sep 3, 2019

deadalnix referenced this in commit 57ac9a6bbb on May 2, 2020

deadalnix referenced this in commit aab89ee14a on May 2, 2020

deadalnix referenced this in commit 2d7438dab2 on May 2, 2020

deadalnix referenced this in commit 965225afe7 on May 2, 2020

deadalnix referenced this in commit 35f36b8ef1 on Jul 28, 2020

monstrobishi referenced this in commit 244328c4a2 on Jul 30, 2020

ftrader referenced this in commit 5ce5d1ebd5 on Aug 17, 2020

ftrader referenced this in commit d2f013ba4e on Aug 17, 2020

ftrader referenced this in commit d4090662ca on Aug 17, 2020

ShengguangXiao referenced this in commit 0139ccd62b on Aug 28, 2020

ShengguangXiao referenced this in commit b72ae741c9 on Aug 28, 2020

ShengguangXiao referenced this in commit b9123020d2 on Aug 28, 2020

amitiuttarwar commented at 7:57 pm on December 23, 2020: contributor

This PR introduced the field permissions to the getpeerinfo RPC, but did not add it to the RPCHelpMan. I’ve opened #20756 to make the documentation consistent :)

fanquake deleted a comment on Feb 9, 2021

fanquake locked this on Feb 9, 2021

Make whitebind/whitelist permissions more flexible #16248

Motivation

Implementation details

Follow up idea

Conflicts