net: Extract download permission from noban #19191
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
This was referenced Jun 7, 2020
|
Concept ACK. |
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Jun 12, 2020
Github-Pull: bitcoin#19191 Rebased-From: 111109a
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Jun 12, 2020
Github-Pull: bitcoin#19191 Rebased-From: 111109a
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Jun 15, 2020
Github-Pull: bitcoin#19191 Rebased-From: 111109a
|
utACK 111109a |
This was referenced Jun 17, 2020
|
ACK 111109a. (👍 on the needs release note tag) |
|
utACK 111109a |
|
utACK 111109a, but needs rebase. |
|
Concept ACK |
jonatack
reviewed
Jul 8, 2020
|
Added release notes and addressed feedback by @jonatack |
|
ACK fa0540c |
|
re-utACK fa0540c |
This was referenced Jul 9, 2020
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Aug 15, 2020
Github-Pull: bitcoin#19191 Rebased-From: 111109a
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Mar 22, 2021
Summary: ``` It should be possible to grant nodes in a local network (e.g. home, university, enterprise, ...) permission to download blocks even after the maxuploadtarget is hit. Currently this is only possible by setting the noban permission, which has some adverse effects, especially if the peers can't be fully trusted. Fix this by extracting a download permission from noban. ``` Backport of [[bitcoin/bitcoin#19191 | core#19191]]. Depends on D9324 and D9325. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9326
This was referenced Apr 8, 2021
laanwj
added a commit
that referenced
this pull request
May 11, 2021
…:Bind() 36fb036 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack) 4e0d578 test: add net permissions noban/download unit test coverage (Jon Atack) dde69f2 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack) Pull request description: This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected. Since #19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers. The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them. The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this. ACKs for top commit: theStack: re-ACK 36fb036 ☕ vasild: ACK 36fb036 hebasto: ACK 36fb036, I have reviewed the code and it looks OK, I agree it can be merged. kallewoof: Code review ACK 36fb036 Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 11, 2021
…onnman::Bind() 36fb036 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack) 4e0d578 test: add net permissions noban/download unit test coverage (Jon Atack) dde69f2 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack) Pull request description: This is a bugfix follow-up to bitcoin#16248 and bitcoin#19191 that was noticed in bitcoin#21506. Both v0.21 and master are affected. Since bitcoin#19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers. The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them. The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this. ACKs for top commit: theStack: re-ACK 36fb036 ☕ vasild: ACK 36fb036 hebasto: ACK 36fb036, I have reviewed the code and it looks OK, I agree it can be merged. kallewoof: Code review ACK 36fb036 Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It should be possible to grant nodes in a local network (e.g. home, university, enterprise, ...) permission to download blocks even after the maxuploadtarget is hit.
Currently this is only possible by setting the
nobanpermission, which has some adverse effects, especially if the peers can't be fully trusted.Fix this by extracting a
downloadpermission fromnoban.