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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
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
download == noban. Try: nodes with the download permission may exceed target
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)) {
download permission. Maybe keep it noban?
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?
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.
Suggestions would be useful. Keep in mind that the documentation already says:
0download (allow getheaders during IBD, no disconnect after maxuploadtarget limit)
0// 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.
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.
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,
PF_DOWNLOAD here as PF_NOBAN set it.
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.
ACK 111109a1e79a9b9ef12a4718b2ab2ab2317fbabc.
(👍 on the needs release note tag)
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'],
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
# Make sure the default values in the command line documentation match the ones here and updated the help