p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() #21644

jonatack · 2021-04-09T19:48:51Z

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.

DrahtBot · 2021-04-09T21:13:53Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p, refactor: make NetPermissionFlags an enum class #21506 (p2p, refactor: make NetPermissionFlags an enum class by jonatack)

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.

vasild

ACK 693af714ea264cd5edeadecaa0e7762cfe377e5a

Some non-blocker comments below.

src/net.cpp

src/test/netbase_tests.cpp

vasild · 2021-04-14T07:45:19Z

...If the intent is for this conditional to return false only for noban...

I think it is - the reasoning must be that we do not want to advertise our listen address if it has noban.

PF_NOBAN is a multi-flag that includes PF_DOWNLOAD, so the conditional in CConnman::Bind() using a bitwise AND will return the same result for both the "noban" status and the "download" status. Example: `PF_DOWNLOAD` is `0b1000000` `PF_NOBAN` is `0b1010000` This makes a check like `flags & PF_NOBAN` return `true` even if `flags` is equal to `PF_DOWNLOAD`. If `[email protected]:8765` is specified, then `1.1.1.1:8765` should be added to the list of local addresses. We only want to avoid adding to local addresses (that are advertised) a whitebind that has a `noban@` flag. As a result of a mis-check in `CConnman::Bind()` we would not have added `1.1.1.1:8765` to the local addresses in the example above. Co-authored-by: Vasil Dimov <[email protected]>

jonatack · 2021-04-14T16:26:21Z

Thanks for the excellent review, @vasild. I added your commit description example from #21668, incorporated your suggestions, followed up on your #21644 (comment) with the last commit, and updated all the commit messages and the PR description.

src/net_permissions.h

vasild

ACK 2d34c4874775f9e1fc7e8f02a9817a6898461abd

theStack

ACK 2d34c4874775f9e1fc7e8f02a9817a6898461abd

Thanks for fixing!

nit: It's generally better to have more tests rather than less, but personally I don't see much gain in also repeatedly testing the flags with bitmasks, e.g. this part

    BOOST_CHECK_EQUAL(download.m_flags, NetPermissionFlags::PF_DOWNLOAD);
    BOOST_CHECK(download.m_flags != NetPermissionFlags::PF_NOBAN);
    BOOST_CHECK(download.m_flags & NetPermissionFlags::PF_DOWNLOAD);
    BOOST_CHECK(download.m_flags & NetPermissionFlags::PF_NOBAN);

is semantically equivalent to

    BOOST_CHECK_EQUAL(download.m_flags, NetPermissionFlags::PF_DOWNLOAD);
    BOOST_CHECK(NetPermissionFlags::PF_DOWNLOAD != NetPermissionsFlags::PF_NOBAN);
    BOOST_CHECK(NetPermissionFlags::PF_DOWNLOAD & NetPermissionsFlags::PF_DOWNLOAD); // tautology?
    BOOST_CHECK(NetPermissionFlags::PF_DOWNLOAD & NetPermissionsFlags::PF_NOBAN);

i.e. everything after the first line doesn't test the result related to the previous tryParse-call, but only what bits the PF_ enums consist of internally, which could potentially be confusing to readers of this test at this place.

to clarify/test the relationship and NetPermissions operations involving the NetPermissionFlags PF_NOBAN and PF_DOWNLOAD. Co-authored-by: Vasil Dimov <[email protected]>

NetPermissions::ClearFlag() is currently only called in the codebase with an `f` value of NetPermissionFlags::PF_ISIMPLICIT. If that should change in the future, ClearFlag() should not be called with `f` being a subflag of a multiflag, e.g. NetPermissionFlags::PF_RELAY or NetPermissionFlags::PF_DOWNLOAD, as that would leave `flags` in an invalid state corresponding to none of the existing NetPermissionFlags. Therefore, allow only calling ClearFlag with the implicit flag for now. Co-authored-by: Vasil Dimov <[email protected]>

jonatack · 2021-04-18T14:41:16Z

@theStack Thanks for having a look! Dropped the bitwise AND test assertions; they would be removed in #21506 anyway so we may as well not add them.

theStack

re-ACK 36fb036 ☕

vasild

ACK 36fb036

hebasto

Approach ACK 36fb036.

This PR clearly shows how fragile "multibit flags" are. We definitely shouldn't introduce new ones.

From the second "test: add net permissions noban/download unit test coverage" commit I expected that it will fail without the first one. Is it possible to improve tests to prevent regressions in the future?

jonatack · 2021-04-29T17:20:01Z

@hebasto Thanks. Follow-up #21506 prevents this class of bugs.

hebasto

ACK 36fb036, I have reviewed the code and it looks OK, I agree it can be merged.

jonatack · 2021-05-11T10:16:22Z

Thanks @vasild, @theStack, and @hebasto for the reviews and ACKs but this bugfix isn't being merged and I am cleaning up some loose ends before being less present here for some seminars. Up for grabs.

kallewoof

Code review ACK 36fb036

Seems like ClearFlag could be renamed ClearImplicitFlag and remove 2nd argument instead of the assert stuff, but that would make the diff bigger, so no big deal.

jonatack · 2021-05-11T10:47:23Z

@kallewoof agree, I thought about removing the argument too, could go one way or the other.

…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

src/net.cpp

7075f60 scripted-diff: update noban documentation in net_processing.cpp (Jon Atack) a95540c scripted-diff: rename NetPermissionFlags enumerators (Jon Atack) 810d092 p2p, refactor: make NetPermissionFlags a uint32 enum class (Jon Atack) 7b55a94 p2p: NetPermissions::HasFlag() pass flags param by value (Jon Atack) 91f6e6e scripted-diff: add NetPermissionFlags scopes where not already present (Jon Atack) Pull request description: While reviewing #20196, I noticed the `NetPermissionFlags` enums are frequently called as if they were scoped, yet are still global. This patch upgrades `NetPermissionFlags` to a scoped class enum and updates the enumerator naming, similarly to #19771. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum-enumerations for more info. This change would eliminate the class of bugs like #20196 (comment) and #21644, as only defined operations on the flags would compile. ACKs for top commit: laanwj: Code review ACK 7075f60 vasild: ACK 7075f60 Tree-SHA512: 7fcea66ee499f059efc78c934b5f729b3c8573fe304dee2c27c837c2f662b89324790568246d75b2a574cf9f059b42d3551d928996862f4358055eb43521e6f4

maflcko · 2021-05-22T07:54:50Z

Only backport to 0.21 needed

PF_NOBAN is a multi-flag that includes PF_DOWNLOAD, so the conditional in CConnman::Bind() using a bitwise AND will return the same result for both the "noban" status and the "download" status. Example: `PF_DOWNLOAD` is `0b1000000` `PF_NOBAN` is `0b1010000` This makes a check like `flags & PF_NOBAN` return `true` even if `flags` is equal to `PF_DOWNLOAD`. If `[email protected]:8765` is specified, then `1.1.1.1:8765` should be added to the list of local addresses. We only want to avoid adding to local addresses (that are advertised) a whitebind that has a `noban@` flag. As a result of a mis-check in `CConnman::Bind()` we would not have added `1.1.1.1:8765` to the local addresses in the example above. Co-authored-by: Vasil Dimov <[email protected]> Github-Pull: bitcoin#21644 Rebased-From: dde69f2

maflcko · 2021-05-22T08:01:16Z

Backported in #22022

PF_NOBAN is a multi-flag that includes PF_DOWNLOAD, so the conditional in CConnman::Bind() using a bitwise AND will return the same result for both the "noban" status and the "download" status. Example: `PF_DOWNLOAD` is `0b1000000` `PF_NOBAN` is `0b1010000` This makes a check like `flags & PF_NOBAN` return `true` even if `flags` is equal to `PF_DOWNLOAD`. If `[email protected]:8765` is specified, then `1.1.1.1:8765` should be added to the list of local addresses. We only want to avoid adding to local addresses (that are advertised) a whitebind that has a `noban@` flag. As a result of a mis-check in `CConnman::Bind()` we would not have added `1.1.1.1:8765` to the local addresses in the example above. Co-authored-by: Vasil Dimov <[email protected]> Github-Pull: bitcoin#21644 Rebased-From: dde69f2

jonatack mentioned this pull request Apr 9, 2021

p2p, refactor: make NetPermissionFlags an enum class #21506

Merged

DrahtBot added the P2P label Apr 9, 2021

DrahtBot mentioned this pull request Apr 10, 2021

Allow whitelisting outgoing connections #17167

Closed

jonatack changed the title ~~net: use NetPermissions::HasFlag() in CConnman::Bind()~~ p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() Apr 11, 2021

This was referenced Apr 13, 2021

net: properly handle PF_NOBAN in CConnman::Bind() #21668

Closed

net: fix GetListenPort() to derive the proper port #20196

Merged

vasild approved these changes Apr 14, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/test/netbase_tests.cpp Outdated Show resolved Hide resolved

src/test/netbase_tests.cpp Outdated Show resolved Hide resolved

src/test/netbase_tests.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the NetPermissionFlags-noban-bugfix branch from 693af71 to acef962 Compare April 14, 2021 16:16

vasild reviewed Apr 14, 2021

View reviewed changes

src/net_permissions.h Outdated Show resolved Hide resolved

jonatack force-pushed the NetPermissionFlags-noban-bugfix branch 2 times, most recently from ab2a398 to 2d34c48 Compare April 14, 2021 18:49

vasild approved these changes Apr 15, 2021

View reviewed changes

theStack approved these changes Apr 18, 2021

View reviewed changes

jonatack and others added 2 commits April 18, 2021 16:32

test: add net permissions noban/download unit test coverage

4e0d578

to clarify/test the relationship and NetPermissions operations involving the NetPermissionFlags PF_NOBAN and PF_DOWNLOAD. Co-authored-by: Vasil Dimov <[email protected]>

jonatack force-pushed the NetPermissionFlags-noban-bugfix branch from 2d34c48 to 36fb036 Compare April 18, 2021 14:40

theStack approved these changes Apr 18, 2021

View reviewed changes

vasild approved these changes Apr 19, 2021

View reviewed changes

hebasto reviewed Apr 29, 2021

View reviewed changes

hebasto approved these changes Apr 29, 2021

View reviewed changes

jonatack closed this May 11, 2021

kallewoof reviewed May 11, 2021

View reviewed changes

jonatack reopened this May 11, 2021

laanwj merged commit e175a20 into bitcoin:master May 11, 2021

jonatack deleted the NetPermissionFlags-noban-bugfix branch May 11, 2021 11:14

rebroad reviewed May 15, 2021

View reviewed changes

src/net.cpp Show resolved Hide resolved

maflcko added Needs backport (0.19) and removed Needs backport (0.19) labels May 22, 2021

maflcko removed the Needs backport (0.21) label May 22, 2021

hebasto mentioned this pull request May 22, 2021

Final backports for 0.21.2rc1 #22022

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge e175a20 into merged_master (Bitcoin PR bitcoin/bitcoin#21644)

edd5794

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() #21644

p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() #21644

Uh oh!

Conversation

jonatack commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild commented Apr 14, 2021

Uh oh!

jonatack commented Apr 14, 2021

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Apr 29, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented May 11, 2021

Uh oh!

Uh oh!

maflcko commented May 22, 2021

Uh oh!

maflcko commented May 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jonatack commented Apr 9, 2021 •

edited

Loading

DrahtBot commented Apr 9, 2021 •

edited

Loading

jonatack commented Apr 18, 2021 •

edited

Loading

jonatack commented May 11, 2021 •

edited

Loading