RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions")

luke-jr commented at 6:45 pm on August 20, 2020: member

If we were going to continue support for “whitelisted”, we should have probably made it true if any permission flag was set, rather than only if “default permissions” were used.

This corrects the description, and deprecates it.

DrahtBot added the label RPC/REST/ZMQ on Aug 20, 2020

DrahtBot commented at 7:50 pm on August 20, 2020: member

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

Conflicts

No conflicts as of last run.

luke-jr force-pushed on Aug 20, 2020

in src/rpc/net.cpp:122 in 6396aa38f8 outdated

118@@ -119,7 +119,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
119                             {
120                                 {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"},
121                             }},
122-                            {RPCResult::Type::BOOL, "whitelisted", "Whether the peer is whitelisted"},
123+                            {RPCResult::Type::BOOL, "whitelisted", /* optional */ true, "Whether the peer is whitelisted with default permissions (DEPRECATED)"},

MarcoFalke commented at 6:14 am on August 21, 2020:

Could mention when it is returned/not returned similar to bascore:

0src/rpc/net.cpp:                            {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"},

MarcoFalke commented at 6:17 am on August 21, 2020: member

ACK 6396aa38f8ca6ecda55610e6c43e5b4108e2f171

in test/functional/p2p_permissions.py:166 in 6396aa38f8 outdated

162@@ -150,10 +163,14 @@ def check_tx_relay(self):
163         )
164 
165     def checkpermission(self, args, expectedPermissions, whitelisted):
166+        if whitelisted is not None: args = [*args, '-deprecatedrpc=whitelisted']

MarcoFalke commented at 6:18 am on August 21, 2020:

You’ll either need to fix the linter or this test.

0test/functional/p2p_permissions.py:166:35: E701 multiple statements on one line (colon)

jonatack commented at 6:20 am on August 21, 2020: member

Concept ACK

laanwj commented at 12:30 pm on August 23, 2020: member

Does this need a release notes mention? Concept ACK

laanwj commented at 2:27 pm on August 24, 2020: member

Travis gives the following new Python lint error:

0test/functional/p2p_permissions.py:166:35: E701 multiple statements on one line (colon)

Edit: oh already reported.

luke-jr force-pushed on Aug 24, 2020

luke-jr commented at 2:45 pm on August 24, 2020: member

Changes done

in test/lint/lint-python.sh:36 in 531198bcef outdated

32@@ -33,7 +33,6 @@ enabled=(
33     E401 # multiple imports on one line
34     E402 # module level import not at top of file
35     E502 # the backslash is redundant between brackets
36-    E701 # multiple statements on one line (colon)

laanwj commented at 7:03 am on August 27, 2020:

Meh on this, I don’t think a change of code style is necessary in this PR

jonatack commented at 9:21 am on August 27, 2020:

Agree, could lead to future style bikeshedding.

luke-jr commented at 12:54 pm on August 27, 2020:

Necessary or not, this is our current code style for C++ and seems quite reasonable…

Where is the basis for banning it? It’s not like it’s dangerous syntax.

jnewbery commented at 2:51 pm on August 27, 2020:

In C++, if code blocks are denoted by braces. Having a single line if statement indented without braces is dangerous because it’s easy to misread a subsequent indented line as part of the if block. These things have caused bugs in production systems, which is why the style guide disallows it.

Python is a completely different language. If blocks are denoted by indentation. The same considerations don’t apply.

Please remove this unnecessary style change.

MarcoFalke commented at 8:17 am on August 27, 2020: member

cr ACK 531198bcef9f6213d588f8539dd05f4b1cca0781

No opinion on the linter, I am fine with either alternative.

MarcoFalke added this to the milestone 0.21.0 on Aug 27, 2020

in src/rpc/net.cpp:122 in 531198bcef outdated

118@@ -119,7 +119,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
119                             {
120                                 {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"},
121                             }},
122-                            {RPCResult::Type::BOOL, "whitelisted", "Whether the peer is whitelisted"},
123+                            {RPCResult::Type::BOOL, "whitelisted", /* optional */ true, "Whether the peer is whitelisted with default permissions (DEPRECATED, returned only if config option -deprecatedrpc=whitelisted is passed)"},

jonatack commented at 8:40 am on August 27, 2020:

This line is much longer than any of the other ones. Consider newlining to display it in two lines (perhaps do the same with the help at line 134 while here).

jonatack commented at 8:45 am on August 27, 2020:

luke-jr commented at 12:54 pm on August 27, 2020:

Isn’t formatting the job of RPCHelpMan?

MarcoFalke commented at 1:14 pm on August 27, 2020:

RPCHelpMan doesn’t (yet) add line breaks.

Regardless, this string will go away on November 1st, so no need to over-optimize something that will be removed.

laanwj commented at 9:07 am on August 27, 2020: member

To be honest I think it’s pretty un-python-y to have multiple statements on one line. I’ve never seen it before at all.

MarcoFalke commented at 9:12 am on August 27, 2020: member

Heh, I’ve also never seen it before. Though, it shouldn’t cause any bugs, so if it makes @luke-jr happy and travis is green, then I won’t complain :sweat_smile:

practicalswift commented at 1:34 am on August 29, 2020: contributor

NACK on the unrelated test/lint/lint-python.sh change.

The PR author’s personal style preference is not a good reason to deviate from the well-established idiomatic style which the Python community has settled on and even formalised via PEP-8. Doing so would just open up for future bikeshedding where we currently run no such risk.

The language’s style guide (PEP-8, https://www.python.org/dev/peps/pep-0008/) couldn’t be more clear on this:

Compound statements (multiple statements on the same line) are generally discouraged

StopAndDecrypt commented at 11:55 am on September 6, 2020: none

Edit: I’ve redirected my concerns here.

localcryptosMichael commented at 1:14 pm on September 6, 2020: none

@StopAndDecrypt, this PR seems to have nothing to do with race, political correctness, feelings, or anything else you wrote.

gmaxwell commented at 2:58 pm on September 6, 2020: contributor

The change to permissions isn’t a terminology change its an important change in functionality which has been slowly progressing for years.

One of the original motivations for the whitelisting support was selectively bypassing banning/disconnection logic instead of doing so for everything localhost so you could be sure your own stuff didn’t get banned without losing your protections towards Tor peers.

But then armory was kind of broken and depended on its host node relaying its transactions no matter what and whitelisting (even by the time it was originally merged) gained the property of force relaying everything that the whitelisted peer sent. But actually made whitelist mostly unusable for the original purpose, because if you whitelisted ordinary nodes the whitelisting node would start spamming the crap out of other peers with redundant broadcasts. (it would even cause you to relay invalid transactions for a while and get yourself banned by other peers if any were sent to you from a whitelisted peer)

And so the tension has continued throughout all of whitelist’s life: people keep wanting to defeat some protection or limiter selectively for particular trusted peers or test harnessses, but then binning that same behaviour change in with all other whitelisted behaviour changes breaks your ability to use whitelisting for some other purpose where that particular behaviour change is highly undesirable. The only solution is to replace whitelisting with granular properties which can be applied on a connection-by-connection basis.

luke-jr force-pushed on Sep 12, 2020

luke-jr commented at 5:42 pm on September 12, 2020: member

Don’t like the precedent for arbitrary style rules overriding readability, but regardless..

Both changes made.

MarcoFalke commented at 7:21 am on September 20, 2020: member

re-ACK 98871538de03073fa67d64c20403d0b373ed77c1

DrahtBot added the label Needs rebase on Sep 22, 2020

jnewbery commented at 10:36 am on September 23, 2020: member

utACK 98871538de03073fa67d64c20403d0b373ed77c1 but needs rebase

MarcoFalke commented at 8:32 am on October 14, 2020: member

@luke-jr Would be good to get this into the next release. I am happy to pick up and rebase, if you don’t get around to it

RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") d681a28219

RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg 5b57dc5458

luke-jr commented at 2:17 pm on October 14, 2020: member

Rebased

luke-jr force-pushed on Oct 14, 2020

luke-jr requested review from MarcoFalke on Oct 14, 2020

luke-jr requested review from jonatack on Oct 14, 2020

luke-jr requested review from jnewbery on Oct 14, 2020

luke-jr requested review from laanwj on Oct 14, 2020

in test/functional/p2p_blocksonly.py:62 in 5b57dc5458

58@@ -59,7 +59,7 @@ def run_test(self):
59 
60         self.log.info('Check that txs from peers with relay-permission are not rejected and relayed to others')
61         self.log.info("Restarting node 0 with relay permission and blocksonly")
62-        self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"])
63+        self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly", '-deprecatedrpc=whitelisted'])

jonatack commented at 3:12 pm on October 14, 2020:

This change doesn’t seem to be needed, e.g. the test doesn’t fail without it unless you also assert on the whitelist field (in which case, use double quotes like the other args in this line).

MarcoFalke commented at 3:49 pm on October 14, 2020:

The whitelist field has been asserted on before the rebase, so this is probably an unsolved silent conflict

DrahtBot removed the label Needs rebase on Oct 14, 2020

laanwj commented at 9:11 am on October 15, 2020: member

ACK 5b57dc5458800e56b4dddfeb32a1813804a62b0f (apart from @jonatack nit)

MarcoFalke commented at 9:46 am on October 15, 2020: member

The test can be adjusted after feature freeze

MarcoFalke merged this on Oct 15, 2020

MarcoFalke closed this on Oct 15, 2020

sidhujag referenced this in commit 12256d1e5a on Oct 16, 2020

amitiuttarwar commented at 8:27 pm on December 23, 2020: contributor

I didn’t see a release note for this change, so I added some to the wiki

The getpeerinfo RPC no longer returns the whitelisted field by default. This field will be fully removed in the next major release. It can be accessed with the configuration option -deprecatedrpc=getpeerinfo_whitelisted. However, it is recommended to instead use the permissions field to understand if specific privileges have been granted to the peer. (#19770)

please let me know (or edit the wiki) if I’ve miscommunicated anything

Fabcien referenced this in commit 26109b3a50 on Nov 24, 2021

DrahtBot locked this on Feb 15, 2022

RPC: getpeerinfo: Deprecate “whitelisted” field (replaced by “permissions”) #19770

Conflicts