net: Extract download permission from noban #19191

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2006-netPerDow changing 11 files +43 −29

MarcoFalke commented at 3:05 PM on June 6, 2020: member

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.
MarcoFalke added the label P2P on Jun 6, 2020
DrahtBot cross-referenced this on Jun 7, 2020 from issue doc: Extract net permissions doc by MarcoFalke
DrahtBot commented at 1:15 AM on June 7, 2020: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)
- #18077 (net: Add NAT-PMP port forwarding support by hebasto)
- #14866 ([wip] util: Improve evaluation of includeconf lines in network sections of the config file by AkioNak)
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.
DrahtBot cross-referenced this on Jun 7, 2020 from issue refactor: Error message bilingual_str consistency by laanwj
DrahtBot cross-referenced this on Jun 7, 2020 from issue Cache responses to GETADDR to prevent topology leaks by naumenkogs
DrahtBot cross-referenced this on Jun 7, 2020 from issue net: Add blockfilters white{bind,list} permission flag by luke-jr
naumenkogs commented at 7:06 AM on June 7, 2020: member

Concept ACK.

in src/net_processing.cpp:1515 in 1f0084e3ad outdated

1511 | @@ -1512,7 +1512,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c
1512 |      if (send &&
1513 |          connman->OutboundTargetReached(true) &&
1514 |          (((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.type == MSG_FILTERED_BLOCK) &&
1515 | -        !pfrom.HasPermission(PF_NOBAN) // never disconnect nodes with the noban permission
1516 | +        !pfrom.HasPermission(PF_DOWNLOAD) // never disconnect nodes with the download permission

Sjors commented at 10:43 AM on June 8, 2020:

This comment is confusing, because it suggests download == noban. Try: nodes with the download permission may exceed target

MarcoFalke commented at 11:36 AM on June 8, 2020:

Thanks, fixed.

in src/net_processing.cpp:2742 in 1f0084e3ad outdated
```
2744 | @@ -2745,7 +2745,7 @@ bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
2745 |          }
2746 |  
2747 |          LOCK(cs_main);
2748 | -        if (::ChainstateActive().IsInitialBlockDownload() && !pfrom.HasPermission(PF_NOBAN)) {
2749 | +        if (::ChainstateActive().IsInitialBlockDownload() && !pfrom.HasPermission(PF_DOWNLOAD)) {
```
Sjors commented at 10:46 AM on June 8, 2020:

This exception was strange to begin with. But I don't think it has anything to do with download permission. Maybe keep it noban?

MarcoFalke commented at 11:44 AM on June 8, 2020:

Why is is strange? The literal motivation for this is that nodes with this permission can use us "as a block source" (#6974).

Imagine you spin up an archival node, disconnect it from the network, but make it available in you internal network. Nodes with the download permission should be able to use your archival node as a block source, no?

Sjors commented at 5:09 PM on June 8, 2020:

Ah, that PR explains the problem. Even nodes that are almost synced can be IsInitialBlockDownload(), presumably also nodes that have been disconnected for a few days? In that case the new permission makes sense. Would be good to explain in a comment.
MarcoFalke commented at 5:11 PM on June 8, 2020:

Suggestions would be useful. Keep in mind that the documentation already says:

download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)
Sjors commented at 5:26 PM on June 8, 2020:

// Allow getheaders while IsInitialBlockDownload is true, such as after prolonged disconnection.

Documenting that here instead of just in the bitcoind help seems useful, especially IsInitialBlockDownload has a confusing name.
MarcoFalke commented at 12:21 PM on June 15, 2020:

IsInitialBlockDownload has a confusing name

I think IsInitialBlockDownload is self-explanatory, so I will leave the documentation as is for now. I am happy to review follow-up documentation improvements, though.
Sjors commented at 10:52 AM on June 8, 2020: member

Concept ACK. Some minor feedback on faeb4f4fd4ba07063213e5a7fda797b2c53cc29f.
MarcoFalke force-pushed on Jun 8, 2020
luke-jr approved
luke-jr commented at 4:12 AM on June 11, 2020: member

utACK
DrahtBot cross-referenced this on Jun 11, 2020 from issue banman: Limit resources consumed by misbehaving node deprioitisation by luke-jr
luke-jr referenced this in commit 06a8ba1723 on Jun 12, 2020
luke-jr referenced this in commit fee488f784 on Jun 12, 2020
luke-jr referenced this in commit 46af7c1918 on Jun 15, 2020
Sjors commented at 12:02 PM on June 15, 2020: member

utACK 111109a1e79a9b9ef12a4718b2ab2ab2317fbabc
MarcoFalke added the label Needs release note on Jun 15, 2020
in src/net_permissions.h:35 in 111109a1e7 outdated
```
  30 |      PF_MEMPOOL = (1U << 5),
  31 |  
  32 |      // True if the user did not specifically set fine grained permissions
  33 |      PF_ISIMPLICIT = (1U << 31),
  34 | -    PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL,
  35 | +    PF_ALL = PF_BLOOMFILTER | PF_FORCERELAY | PF_RELAY | PF_NOBAN | PF_MEMPOOL | PF_DOWNLOAD,
```
NicolasDorier commented at 11:53 AM on June 16, 2020:

nit: no need to add PF_DOWNLOAD here as PF_NOBAN set it.

MarcoFalke commented at 1:15 PM on June 16, 2020:

I tried to mimic what PF_FORCERELAY was doing. Also, my long term plan is to remove the implicit download permission from noban, but that is up for another discussion.

luke-jr commented at 9:38 PM on June 16, 2020:

Agree that being explicit here is best.
DrahtBot cross-referenced this on Jun 17, 2020 from issue [tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar
DrahtBot cross-referenced this on Jun 18, 2020 from issue Replace automatic bans with discouragement filter by sipa
promag commented at 10:22 PM on July 5, 2020: member

ACK 111109a1e79a9b9ef12a4718b2ab2ab2317fbabc.

(👍 on the needs release note tag)
naumenkogs commented at 10:47 AM on July 7, 2020: member

utACK 111109a1e79a9b9ef12a4718b2ab2ab2317fbabc
DrahtBot added the label Needs rebase on Jul 7, 2020
sipa commented at 6:38 PM on July 7, 2020: member

utACK 111109a1e79a9b9ef12a4718b2ab2ab2317fbabc, but needs rebase.
jonatack commented at 4:17 AM on July 8, 2020: contributor

Concept ACK
in test/functional/p2p_permissions.py:42 in 111109a1e7 outdated
```
  38 | @@ -39,21 +39,21 @@ def run_test(self):
  39 |          self.checkpermission(
  40 |              # default permissions (no specific permissions)
  41 |              ["-whitelist=127.0.0.1"],
  42 | -            ["relay", "noban", "mempool"],
  43 | +            ["relay", "noban", "mempool", 'download'],
```
jonatack commented at 4:55 AM on July 8, 2020:

If the default -whitelist permissions are changed by this PR, consider pulling in the changes from https://github.com/jonatack/bitcoin/commits/whitelist-whitebind-doc-improvements.

nit: follow style for quotes already in use in this test

MarcoFalke commented at 10:51 AM on July 9, 2020:

Ah, good point. Added a comment in the test # Make sure the default values in the command line documentation match the ones here and updated the help
MarcoFalke removed the label Needs release note on Jul 9, 2020
MarcoFalke force-pushed on Jul 9, 2020
net: Extract download permission from noban fa0540cd46
MarcoFalke force-pushed on Jul 9, 2020
MarcoFalke commented at 10:51 AM on July 9, 2020: member

Added release notes and addressed feedback by @jonatack
DrahtBot removed the label Needs rebase on Jul 9, 2020
jonatack commented at 1:51 PM on July 9, 2020: contributor

ACK fa0540c
Sjors commented at 2:02 PM on July 9, 2020: member

re-utACK fa0540cd46eaf44d9e1a9f91c3a937986826c4fa
fanquake requested review from naumenkogs on Jul 9, 2020
fanquake requested review from sipa on Jul 9, 2020
MarcoFalke merged this on Jul 9, 2020
MarcoFalke closed this on Jul 9, 2020
MarcoFalke deleted the branch on Jul 9, 2020
DrahtBot cross-referenced this on Jul 9, 2020 from issue net: Add NAT-PMP port forwarding support by hebasto
DrahtBot cross-referenced this on Jul 9, 2020 from issue [wip] util: Improve evaluation of includeconf lines in network sections of the config file by AkioNak
luke-jr referenced this in commit 74c9a19501 on Aug 15, 2020
deadalnix referenced this in commit 56f61132d7 on Mar 22, 2021
jonatack cross-referenced this on Apr 8, 2021 from issue p2p, refactor: make NetPermissionFlags an enum class by jonatack
jonatack cross-referenced this on Apr 9, 2021 from issue p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() by jonatack
laanwj referenced this in commit e175a20769 on May 11, 2021
sidhujag referenced this in commit beef02c60d on May 11, 2021
bitcoin locked this on Feb 15, 2022