Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make whitebind/whitelist permissions more flexible #16248

Merged
merged 4 commits into from Aug 14, 2019

Conversation

@NicolasDorier
Copy link
Member

commented Jun 20, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

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 NicolasDorier force-pushed the NicolasDorier:feature/permissions branch 26 times, most recently from c3fa8ca to 5306e3f Jun 20, 2019

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

@NicolasDorier NicolasDorier force-pushed the NicolasDorier:feature/permissions branch from 19b1cd2 to 2ca0190 Aug 7, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

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

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Almost, still get this one:

net_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]
            int len = commaSeparator == std::string::npos ? permissions.length() - readen : commaSeparator - readen;
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~
net_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]
            if (commaSeparator != std::string::npos) readen++; // We read ","
                ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~
2 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

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2019

@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 NicolasDorier force-pushed the NicolasDorier:feature/permissions branch from 2ca0190 to c5b404e Aug 11, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

re-ACK c5b404e

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Aug 14, 2019
Merge #16248: Make whitebind/whitelist permissions more flexible
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:
  * `-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.

ACKs for top commit:
  laanwj:
    re-ACK c5b404e

Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577
src/net.cpp Show resolved Hide resolved
@MarcoFalke
Copy link
Member

left a comment

Concept ACK.

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

src/net.cpp Show resolved Hide resolved
src/net_permissions.cpp Show resolved Hide resolved
@@ -3786,7 +3792,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
pto->vInventoryBlockToSend.clear();

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 14, 2019

Member

Why is this noban and not relay?

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Aug 15, 2019

Author Member

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 15, 2019

Member

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

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Aug 16, 2019

Author Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 16, 2019

Member

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

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Aug 16, 2019

Author Member

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

src/net.cpp Show resolved Hide resolved
test/functional/p2p_permissions.py Show resolved Hide resolved
@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

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

@NicolasDorier

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

@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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

There are other command line arguments with multi-line explanations, so I don't think that's a huge issue:
Schermafbeelding 2019-08-15 om 15 53 49

MarcoFalke added a commit that referenced this pull request Aug 16, 2019
Merge #16620: util: Move ResolveErrMsg to util/error
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 added a commit to MarcoFalke/bitcoin that referenced this pull request Aug 16, 2019
Merge bitcoin#16618: [Fix] Allow connection of a noban banned peer
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
MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Aug 26, 2019
Merge bitcoin#16629: doc: Add documentation for the new whitelist per…
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 27, 2019
Merge bitcoin#16629: doc: Add documentation for the new whitelist per…
…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
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.