Make whitebind/whitelist permissions more flexible by NicolasDorier · Pull Request #16248 · bitcoin/bitcoin

NicolasDorier · 2019-06-20T09:44:14Z

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:

[email protected]/32.
-whitebind=bloomfilter,relay,[email protected]: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.

DrahtBot · 2019-06-20T09:47:30Z

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.

laanwj · 2019-08-08T11:29:54Z

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

maflcko · 2019-08-08T18:11:14Z

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.

NicolasDorier · 2019-08-11T02:34:07Z

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

laanwj · 2019-08-14T15:06:58Z

re-ACK c5b404e

c5b404e Add functional tests for flexible whitebind/list (nicolas.dorier) d541fa3 Replace the use of fWhitelisted by permission checks (nicolas.dorier) ecd5cf7 Do not disconnect peer for asking mempool if it has NO_BAN permission (nicolas.dorier) e5b26de Make whitebind/whitelist permissions more flexible (nicolas.dorier) Pull request description: # Motivation In 0.19, bloom filter will be disabled by default. I tried to make [a PR](#16176) 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](#16176 (comment)) 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: * `[email protected]/32`. * `-whitebind=bloomfilter,relay,[email protected]: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. ACKs for top commit: laanwj: re-ACK c5b404e Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577

src/net.cpp

maflcko

Concept ACK.

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

src/net.cpp

src/net_permissions.cpp

maflcko · 2019-08-14T19:17:24Z

src/net_processing.cpp


            // Check whether periodic sends should happen
-            bool fSendTrickle = pto->fWhitelisted;
+            bool fSendTrickle = pto->HasPermission(PF_NOBAN);


Why is this noban and not relay?

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.

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

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

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

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

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.

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.

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

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

src/net.cpp

test/functional/p2p_permissions.py

Sjors · 2019-08-15T11:53:36Z

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

NicolasDorier · 2019-08-15T13:07:53Z

@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 · 2019-08-15T13:54:22Z

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

fa27c55 util: Move ResolveErrMsg to util/error (MarcoFalke) Pull request description: Pull request #16248 (comment) duplicated the body of this util function. The whole point of the util function is to be shared, so do that here as a fixup to #16248 ACKs for top commit: Sjors: utACK fa27c55 ryanofsky: utACK fa27c55 Tree-SHA512: e2b25ae05082fe9d0ee94bdc7d51f801bd9f78e8fc2b141e9a313e008dbb8a77653fe876e111c802c676859c6b76c37a673d1f8cfbe7ad25607a5ffcffde19fd

@MarcoFalke

d117f45 Add test for setban (nicolas.dorier) dc7529a [Fix] Allow connection of a noban banned peer (nicolas.dorier) Pull request description: Reported by @MarcoFalke on bitcoin#16248 (comment) The bug would mean that if the peer connecting to you is banned, but whitelisted without specific permissions, it would not be able to connect to the node. The solution is just to move the same line below. ACKs for top commit: Sjors: Agree inline is more clear. utACK d117f45 MarcoFalke: ACK d117f45 Tree-SHA512: 0fed39acb1e8db67bb0bf4c4de3ad034ae776f38d55bd661f1ae0e1a4c6becaf1824ab46ed8279f2f31df3f4b29ff56461d8b167d3e9cece62cfe58b5a912811

…missions 66ad754 [Doc] Add documentation for the new whitelist permissions (nicolas.dorier) Pull request description: Documenting the new feature bitcoin#16248 . Ping Sjors . ACKs for top commit: Sjors: Indeed, re-ACK 66ad754 Tree-SHA512: e6860bb6fae921287da7920a8db534e6a1a23871dd78dd6da030f00adf23e204cd23b194d67361bf34d4ef5a7815fc3fd7c81a3f2f35e4cfbe6ee2f2e6daec25

Github-Pull: bitcoin#16248 Rebased-From: e5b26de

Github-Pull: bitcoin#16248 Rebased-From: ecd5cf7

Github-Pull: bitcoin#16248 Rebased-From: d541fa3

Github-Pull: bitcoin#16248 Rebased-From: c5b404e

amitiuttarwar · 2020-12-23T19:57:23Z

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 added the P2P label Jun 20, 2019

NicolasDorier mentioned this pull request Jun 20, 2019

Allow NODE_BLOOM services for whitelisted peers #16176

Closed

NicolasDorier force-pushed the feature/permissions branch 26 times, most recently from c3fa8ca to 5306e3f Compare June 21, 2019 02:42

NicolasDorier changed the title ~~[WIP] Make whitebind/whitelist permissions more flexible~~ Make whitebind/whitelist permissions more flexible Jun 21, 2019

NicolasDorier added 4 commits August 11, 2019 11:33

Make whitebind/whitelist permissions more flexible

e5b26de

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

ecd5cf7

Replace the use of fWhitelisted by permission checks

d541fa3

Add functional tests for flexible whitebind/list

c5b404e

NicolasDorier force-pushed the feature/permissions branch from 2ca0190 to c5b404e Compare August 11, 2019 02:34

laanwj merged commit c5b404e into bitcoin:master Aug 14, 2019

maflcko reviewed Aug 14, 2019

View reviewed changes

src/net.cpp Show resolved Hide resolved

maflcko reviewed Aug 14, 2019

View reviewed changes

NicolasDorier mentioned this pull request Aug 15, 2019

[Fix] Allow connection of a noban banned peer #16618

Merged

maflcko mentioned this pull request Aug 15, 2019

util: Move ResolveErrMsg to util/error #16620

Merged

NicolasDorier mentioned this pull request Aug 16, 2019

doc: Add documentation for the new whitelist permissions #16629

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019

Make whitebind/whitelist permissions more flexible

e077460

Github-Pull: bitcoin#16248 Rebased-From: e5b26de

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019

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

a4ee744

Github-Pull: bitcoin#16248 Rebased-From: ecd5cf7

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019

Replace the use of fWhitelisted by permission checks

6ed5258

Github-Pull: bitcoin#16248 Rebased-From: d541fa3

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019

Add functional tests for flexible whitebind/list

4d451e9

Github-Pull: bitcoin#16248 Rebased-From: c5b404e

nopara73 mentioned this pull request Dec 21, 2019

Upgrade to Bitcoin Core 19 + Small Improvements WalletWasabi/WalletWasabi#2899

Merged

Bayernatoor mentioned this pull request Feb 6, 2020

Custom node constant disconnect and reconnect bisq-network/bisq#3900

Closed

Conversation

NicolasDorier commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Implementation details

Follow up idea

Uh oh!

DrahtBot commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Aug 8, 2019

Uh oh!

maflcko commented Aug 8, 2019

Uh oh!

NicolasDorier commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 14, 2019

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Aug 15, 2019

Uh oh!

NicolasDorier commented Aug 15, 2019

Uh oh!

Sjors commented Aug 15, 2019

Uh oh!

amitiuttarwar commented Dec 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

NicolasDorier commented Jun 20, 2019 •

edited

Loading

DrahtBot commented Jun 20, 2019 •

edited

Loading

NicolasDorier commented Aug 11, 2019 •

edited

Loading

NicolasDorier Aug 15, 2019 •

edited

Loading

NicolasDorier Aug 16, 2019 •

edited

Loading