Skip to content

net: Avoid discouraging the onion proxy when one inbound onion misbehaves #21190

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
Closed
maflcko wants to merge 1 commit into from
Closed

net: Avoid discouraging the onion proxy when one inbound onion misbehaves #21190

maflcko wants to merge 1 commit into from

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Feb 15, 2021

No description provided.

@vasild
net: Avoid discouraging the onion proxy when one inbound onion misbeh…
d02e486 
…aves

Can be reviewed with --ignore-all-space
@maflcko
Copy link
Member Author

maflcko commented Feb 15, 2021

Stolen from @vasild from #20845 (review)

vasild
vasild approved these changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK d02e486

jonatack
jonatack reviewed Feb 15, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

utACK d02e486

A similar change is proposed to the eviction logic in #20197.

src/net_processing.cpp
if (pnode.addr.IsLocal() || pnode.m_inbound_onion) {
// We disconnect local or onion peers for bad behavior but don't discourage
// (since that would discourage all peers on the same address)
if (pnode.m_inbound_onion) {
Copy link
Member

@jonatack jonatack Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

micro-nit, could save a conditional and a level of nesting by testing for IsLocal() and then for m_inbound_onion, though I could see the argument for choosing to group similar cases

@DrahtBot DrahtBot added the P2P label Feb 15, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 15, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

This was referenced Feb 15, 2021
Merged
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 16, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@maflcko
Copy link
Member Author

maflcko commented Feb 16, 2021

It seems unlikely that one uses a non-local onion proxy, so I am going to close this for now. Someone else can pick this up. Maybe after #20845 is merged...

@maflcko maflcko closed this Feb 16, 2021
@maflcko maflcko deleted the 2102-netOnionInboundDiscourage branch February 16, 2021 12:39
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jonatack jonatack jonatack left review comments

@vasild vasild vasild approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Needs rebase P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maflcko @DrahtBot @vasild @jonatack