Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 2 commits into from
Aug 16, 2019
Merged

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

maflcko merged 2 commits into from
Aug 16, 2019

Conversation

@NicolasDorier
Copy link
Contributor

@NicolasDorier NicolasDorier commented Aug 15, 2019

Reported by @MarcoFalke on #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.

@NicolasDorier NicolasDorier mentioned this pull request Aug 15, 2019
Merged
@fanquake fanquake added the P2P label Aug 15, 2019
@GChuf
Copy link
Contributor

GChuf commented Aug 15, 2019

Is this safe to do? What are the implications of connecting to a peer that would otherwise be banned?

@Sjors
Copy link
Member

Sjors commented Aug 15, 2019 •
edited
Loading

@Gapeman afaik that was the behavior before #16248 (which was not supposed to change existing behavior).

@NicolasDorier would be nice to have a test for this.

@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Aug 15, 2019

@Gapeman if your config specifically say that you don't want to ban a peer, then it should not be banned.

As @Sjors says, I just want to keep current behavior working as before.

I can take a look to see if I can make a test for that.

@NicolasDorier NicolasDorier force-pushed the fix/noban-banned branch 2 times, most recently from c5ea3bd to 72bf177 Compare August 15, 2019 13:27
@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Aug 15, 2019

I refactored it to properly scope some variables. Writing a test so such regression can't happen.

@NicolasDorier NicolasDorier force-pushed the fix/noban-banned branch 2 times, most recently from f235f8c to 4062967 Compare August 15, 2019 14:02
@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Aug 15, 2019

Fixed the bug, and added an anti regression test for setban

@naumenkogs
Copy link
Contributor

naumenkogs commented Aug 15, 2019

utACK a0d2e2b

@maflcko maflcko added this to the 0.19.0 milestone Aug 15, 2019
maflcko
maflcko reviewed Aug 15, 2019
View reviewed changes
Sjors
Sjors reviewed Aug 15, 2019
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis failed due to lint issue. Maybe rename rpc_setban.py to p2p_whitelist.py?

test/functional/rpc_setban.py Outdated
Copy link
Member

@Sjors Sjors Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe pass node in directly and split into assert_has_permission(self, node, permission) and assert_has_no_permission(self, node, permission)

Copy link
Contributor Author

@NicolasDorier NicolasDorier Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about assert_equal(True, self.hasPermision(...)) ?

That said, I don't think it should pass the nodes to this method. It is confusing (there is two nodes: the node that is permissioned, and the node on which we want to check permissions).

Copy link
Member

@Sjors Sjors Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert(self.hasPermision(...)) is even simpler.

You could pass both nodes. Or you could document at the top of the test file which hardcoded nodes do what.

Copy link
Contributor

@promag promag Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I think you could just inline this - it's longer as it is with the contains arg.

promag
promag reviewed Aug 16, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

test/functional/rpc_setban.py Outdated
Copy link
Contributor

@promag promag Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I think you could just inline this - it's longer as it is with the contains arg.

@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Aug 16, 2019

As @promag suggested, I instead just inlined the permission checked, it is actually more readable.

I also check in the logs that we really prevent connection because of the ban. I also added a test to setban x remove.

@NicolasDorier NicolasDorier force-pushed the fix/noban-banned branch 3 times, most recently from 769edfa to a825d91 Compare August 16, 2019 04:21
@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Aug 16, 2019

@Sjors rpc_setban is the right name, I really want to test setban, so I added it in the whitelist of authorized term.

@Sjors
Copy link
Member

Sjors commented Aug 16, 2019

Agree inline is more clear. utACK d117f45

@maflcko
Copy link
Member

maflcko commented Aug 16, 2019

ACK d117f45

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK d117f4541d4717e83c9396273e92960723622030
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUin2gv9GZKDSUbn62676zj0G6Igb8tzgC2M1Sf2KyEuIR+heCoVYGlyp3rXDbuc
22c5dyDXDnktsyMR3mwo3KLzT/MBY/wc8nDgdzkRUzWt6j+/q8Ex8NTnfbgyGXzr
vBE3bJOfA1y3DCCzpSar4uU/sPUAK+oJ3qF1+62rijDYpC0Jh1UaYMy7AdYQdYQ/
kyO3jorAiwIk0sE7SzJDq3rE7wlHzQ4LCZauCnVft/Y1rOY1IkmYfke3uapR4EVr
Zw7D40jY6RsEDLNh/1odaRqjm01v1uJ5d7mFnqOFCKA7RwdS2NQ0ElbfQ3fAUowm
V7Oola76XE8OvGsj9y+qOwBkX5iMgeNQANb+Pf6poB0ANDh0AKpIkxjSIGGbhKi2
XN9KGRaHLpNBeU69lQDrvsXEzyfl4kSLyO9/k7+qZAtgUTqyLDhi9beqS9Ta0OKF
X7RHymgGIn1zvTOY4WSoPiHIF5ceVDu9S5r/fxr9DHmwPWWEzOE9ztfcSiQw68tY
HRrWUvc+
=qI3j
-----END PGP SIGNATURE-----

Timestamp of file with hash 74a14b73630691da8ffa64e5b597d7fd7246787327bba54579da5c03c881ec01 -

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 16, 2019
Merge bitcoin#16618: [Fix] Allow connection of a noban banned peer
b80cdfe 
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
@maflcko maflcko merged commit d117f45 into bitcoin:master Aug 16, 2019
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
@NicolasDorier @luke-jr
[Fix] Allow connection of a noban banned peer
358051e 
Github-Pull: bitcoin#16618
Rebased-From: dc7529a
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
@NicolasDorier @luke-jr
Add test for setban
7a0fe38 
Github-Pull: bitcoin#16618
Rebased-From: d117f45
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 2, 2020
@NicolasDorier @deadalnix
[Fix] Allow connection of a noban banned peer
82899c9 
Summary:
 * Add test for setban

This is a backport of Core [[bitcoin/bitcoin#16618 | PR16618]]

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5935
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Aug 17, 2020
@NicolasDorier @ftrader
[Fix] Allow connection of a noban banned peer
cad767a 
Summary:
 * Add test for setban

This is a backport of Core [[bitcoin/bitcoin#16618 | PR16618]]

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5935
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 20, 2021
@PastaPastaPasta
Merge bitcoin#16618: [Fix] Allow connection of a noban banned peer
ce8b04f 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@Sjors Sjors Sjors left review comments

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

0.19.0

Development

Successfully merging this pull request may close these issues.

7 participants

@NicolasDorier @GChuf @Sjors @naumenkogs @maflcko @promag @fanquake