RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") #19770
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
eb4606a to
6396aa3
Compare
|
ACK 6396aa3 |
|
Concept ACK |
|
Does this need a release notes mention? |
|
Travis gives the following new Python lint error: Edit: oh already reported. |
6396aa3 to
531198b
Compare
|
Changes done |
|
cr ACK 531198b No opinion on the linter, I am fine with either alternative. |
|
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. |
|
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 😅 |
|
NACK on the unrelated 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:
|
|
Edit: I've redirected my concerns here. |
|
@StopAndDecrypt, this PR seems to have nothing to do with race, political correctness, feelings, or anything else you wrote. |
|
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. |
531198b to
9887153
Compare
|
Don't like the precedent for arbitrary style rules overriding readability, but regardless.. Both changes made. |
|
re-ACK 9887153 |
|
utACK 9887153 but needs rebase |
|
@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 |
|
Rebased |
9887153 to
5b57dc5
Compare
| @@ -59,7 +59,7 @@ def run_test(self): | |||
|
|
|||
| self.log.info('Check that txs from peers with relay-permission are not rejected and relayed to others') | |||
| self.log.info("Restarting node 0 with relay permission and blocksonly") | |||
| self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"]) | |||
| self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly", '-deprecatedrpc=whitelisted']) | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
The whitelist field has been asserted on before the rebase, so this is probably an unsolved silent conflict
|
The test can be adjusted after feature freeze |
…(replaced by "permissions") 5b57dc5 RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg (Luke Dashjr) d681a28 RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (Luke Dashjr) Pull request description: 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. ACKs for top commit: laanwj: ACK 5b57dc5 Tree-SHA512: a2e2137f8be8110357c1b2fef2c923fa8c7c4a49b0b2b3a2d78aedf12f8ed5cc7e140018a21b37e6ec7770ed4007542aeef7ad4558973901b107e8e0f81d6003
|
I didn't see a release note for this change, so I added some to the wiki
please let me know (or edit the wiki) if I've miscommunicated anything |
…ions") Summary: > 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. This is a backport of [[bitcoin/bitcoin#19770 | core#19770]] Release notes are from https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.21.0.md Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10500
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.