Make whitebind/whitelist permissions more flexible #16248

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:

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

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.

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

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16248 | PR16248]] : bitcoin/bitcoin@e5b26de Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5928

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16248 | PR16248]] : bitcoin/bitcoin@ecd5cf7 Depends on D5928 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D5929

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16248 | PR16248]] : bitcoin/bitcoin@d541fa3 Depends on D5928 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5932

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16248 | PR16248]] : bitcoin/bitcoin@c5b404e Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5934

Summary: Pull request description: Pull request bitcoin/bitcoin#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 bitcoin/bitcoin@fa27c55 --- Backport of Core [[bitcoin/bitcoin#16620 | PR16620]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7045

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

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16248 | PR16248]] : bitcoin/bitcoin@e5b26de Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5928

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16248 | PR16248]] : bitcoin/bitcoin@c5b404e Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5934

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16248 | PR16248]] : bitcoin/bitcoin@ecd5cf7 Depends on D5928 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D5929

7160c34 util: Move ResolveErrMsg to util/error (MarcoFalke) Pull request description: Pull request bitcoin/bitcoin#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 7160c34 ryanofsky: utACK 7160c34 Tree-SHA512: e2b25ae05082fe9d0ee94bdc7d51f801bd9f78e8fc2b141e9a313e008dbb8a77653fe876e111c802c676859c6b76c37a673d1f8cfbe7ad25607a5ffcffde19fd

@MarcoFalke

05066cf Add test for setban (nicolas.dorier) 48e0bd3 [Fix] Allow connection of a noban banned peer (nicolas.dorier) Pull request description: Reported by @MarcoFalke on bitcoin/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 05066cf MarcoFalke: ACK 05066cf Tree-SHA512: 0fed39acb1e8db67bb0bf4c4de3ad034ae776f38d55bd661f1ae0e1a4c6becaf1824ab46ed8279f2f31df3f4b29ff56461d8b167d3e9cece62cfe58b5a912811

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

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

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

bitcoin deleted a comment from amin1969-crypto Feb 9, 2021

bitcoin locked and limited conversation to collaborators Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make whitebind/whitelist permissions more flexible #16248

Make whitebind/whitelist permissions more flexible #16248

Uh oh!

NicolasDorier commented Jun 20, 2019 •

edited

Loading

Uh oh!

DrahtBot commented Jun 20, 2019 •

edited

Loading

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!

Uh oh!

Make whitebind/whitelist permissions more flexible #16248

Make whitebind/whitelist permissions more flexible #16248

Uh oh!

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!

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!

Uh oh!

NicolasDorier commented Jun 20, 2019 •

edited

Loading

DrahtBot commented Jun 20, 2019 •

edited

Loading