-
Notifications
You must be signed in to change notification settings - Fork 35.7k
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
doc: noban precludes maxuploadtarget disconnects #18968
Conversation
In the future, the permission flag for block download (currently under the |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
Open-Close to re-run ci. See #15847 (comment) |
aaffe50
to
fa5af4c
Compare
Rebased |
fa5af4c
to
faf7e22
Compare
Can be reviewed with the git option --word-diff-regex=.
faf7e22
to
fa9604c
Compare
Rebased |
Concept ACK. |
@NicolasDorier As the author of permission flags you seem qualified to review 😸 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa9604c, I have reviewed the code and it looks OK, I agree it can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa9604c
@@ -23,7 +23,7 @@ longer serving historic blocks (blocks older than one week). | |||
Keep in mind that new nodes require other nodes that are willing to serve | |||
historic blocks. | |||
|
|||
Whitelisted peers will never be disconnected, although their traffic counts for | |||
Peers with the `noban` permission will never be disconnected, although their traffic counts for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe underscores the relation that you may have hungry whitelisted peers provoking ban of honest peers and that something a node operator should care about ? "Resource control of whitelisted peers is left to the node operator or the whitelisting policy deployed"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree but,
- There is the
TODO, exclude peers with noban permission
, so this will probably be fixed in the future by someone. - I want to limit the changes in this pull request to a pure replacement of the word
Whitelist
tonoban permission
.
fa9604c doc: noban precludes maxuploadtarget disconnects (MarcoFalke) fa3999f net: Reformat excessively long if condition into multiple lines (MarcoFalke) Pull request description: Whitelisting has been replaced by permission flags, so properly document this. See also bitcoin#10131 ACKs for top commit: hebasto: ACK fa9604c, I have reviewed the code and it looks OK, I agree it can be merged. ariard: ACK fa9604c Tree-SHA512: 5aee917ab9817719f01ec155487542118e17fa3d145ae7e4bc0e872b2cec39cde9e7fbdee2ae77e9a52700dd8bcc366de4224152e08e709d44d08e0d2f19c613
Summary: This is a backport of [[bitcoin/bitcoin#18968 | core#18968]] Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9277
Whitelisting has been replaced by permission flags, so properly document this. See also #10131