doc: noban precludes maxuploadtarget disconnects

MarcoFalke commented at 5:59 pm on May 13, 2020: member

Whitelisting has been replaced by permission flags, so properly document this. See also #10131

MarcoFalke commented at 6:00 pm on May 13, 2020: member

In the future, the permission flag for block download (currently under the noban umbrella) can hopefully be replaced by a specific download or block_download permission flag.

DrahtBot added the label Docs on May 13, 2020

DrahtBot added the label P2P on May 13, 2020

in src/init.cpp:446 in fa92226263 outdated

442@@ -442,7 +443,7 @@ void SetupServerArgs(NodeContext& node)
443     gArgs.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
444     gArgs.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
445     gArgs.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
446-    gArgs.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h), 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
447+    argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h). Limit does not apply to peers with 'noban' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

laanwj commented at 7:08 pm on May 13, 2020:

What is the gArgs→argsman change?

MarcoFalke commented at 7:28 pm on May 13, 2020:

They are identical, but long term gArgs will be removed from the Setup*Args helpers, to avoid conflicts in how gArgs is used. See #18926 and #18662

To minimize the git blame depth, I made the change while touching the line for other reasons. (Need to scroll right on GitHub to see the changes)

Though, I am happy to leave this out for later or split it into a new commit, if this is undesired.

MarcoFalke commented at 10:27 am on May 14, 2020:

Added a scripted diff to do the replacement in this function and be done with it

MarcoFalke commented at 9:32 pm on May 16, 2020:

Removed. This can be addressed later.

DrahtBot commented at 12:21 pm on May 14, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19174 (refactor: replace CConnman/BanMan pointers by references in net_processing.cpp by theStack)

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.

MarcoFalke commented at 2:33 pm on May 14, 2020: member

Open-Close to re-run ci. See #15847 (comment)

MarcoFalke closed this on May 14, 2020

MarcoFalke reopened this on May 14, 2020

MarcoFalke force-pushed on May 16, 2020

MarcoFalke force-pushed on May 17, 2020

MarcoFalke force-pushed on May 21, 2020

MarcoFalke commented at 2:59 pm on May 21, 2020: member

Rebased

laanwj added this to the "Blockers" column in a project

MarcoFalke force-pushed on May 30, 2020

DrahtBot added the label Needs rebase on Jun 4, 2020

net: Reformat excessively long if condition into multiple lines

Can be reviewed with the git option
--word-diff-regex=.

fa3999fe35

doc: noban precludes maxuploadtarget disconnects fa9604c46f

MarcoFalke force-pushed on Jun 4, 2020

MarcoFalke commented at 9:55 pm on June 4, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Jun 4, 2020

hebasto commented at 10:16 am on June 5, 2020: member

Concept ACK.

in src/net_processing.cpp:1517 in fa3999fe35 outdated

1514-    {
1515+    if (send &&
1516+        connman->OutboundTargetReached(true) &&
1517+        (((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.type == MSG_FILTERED_BLOCK) &&
1518+        !pfrom.HasPermission(PF_NOBAN)
1519+    ) {

hebasto commented at 11:17 am on June 5, 2020:

nit: if formatting is being done here, why do not follow our clang-format rules:

0        !pfrom.HasPermission(PF_NOBAN)) {

?

MarcoFalke commented at 11:24 am on June 5, 2020:

See the next commit, which needs the trailing space to inline the comment // ...

MarcoFalke commented at 11:25 am on June 5, 2020:

I am pretty sure clang-format-diff will pass on the second commit

hebasto commented at 11:28 am on June 5, 2020:

I am pretty sure clang-format-diff will pass on the second commit

Indeed! TIL :)

MarcoFalke commented at 11:18 am on June 5, 2020: member

@NicolasDorier As the author of permission flags you seem qualified to review :smile_cat: :pray:

hebasto approved

hebasto commented at 11:22 am on June 5, 2020: member

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

in doc/reduce-traffic.md:26 in fa9604c46f

22@@ -23,7 +23,7 @@ longer serving historic blocks (blocks older than one week).
23 Keep in mind that new nodes require other nodes that are willing to serve
24 historic blocks.
25 
26-Whitelisted peers will never be disconnected, although their traffic counts for
27+Peers with the `noban` permission will never be disconnected, although their traffic counts for

ariard commented at 9:57 pm on June 5, 2020:

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”

MarcoFalke commented at 0:15 am on June 6, 2020:

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 to noban permission.

ariard commented at 9:58 pm on June 5, 2020: member

ACK fa9604c

MarcoFalke merged this on Jun 6, 2020

MarcoFalke closed this on Jun 6, 2020

fanquake removed this from the "Blockers" column in a project

MarcoFalke deleted the branch on Jun 6, 2020

sidhujag referenced this in commit 925a6e6bb2 on Jun 8, 2020

Fabcien referenced this in commit 0de52488c8 on Mar 4, 2021

DrahtBot locked this on Feb 15, 2022

doc: noban precludes maxuploadtarget disconnects #18968

Conflicts