I came across a TODO in CConnman::RecordBytesSent that wanted to exclude increasing nMaxOutboundTotalBytesSentInCycle for peers with download permission
This PR will exclude increasing nMaxOutboundTotalBytesSentInCycle by adding a bool increaseMaxOutbound to method
CConnman::RecordBytesSent
Methods CConnman::PushMessage and CConnman::SocketHandler uses pnode->HasPermission to check if peer has download permission.
Should we make the new arg (increaseMaxOutbound) default to true?
2868 | @@ -2869,8 +2869,8 @@ void CConnman::RecordBytesSent(uint64_t bytes) 2869 | nMaxOutboundTotalBytesSentInCycle = 0; 2870 | } 2871 | 2872 | - // TODO, exclude peers with download permission 2873 | - nMaxOutboundTotalBytesSentInCycle += bytes; 2874 | + if (increaseMaxOutbound) 2875 | + nMaxOutboundTotalBytesSentInCycle += bytes;
Per doc/developer-notes.md, brackets are generally required for conditionals over one line due to CVE-2014-1266, e.g. the Apple "goto fail" vulnerability, https://dwheeler.com/essays/apple-goto-fail
- if (increaseMaxOutbound)
+ if (increaseMaxOutbound) {
nMaxOutboundTotalBytesSentInCycle += bytes;
+ }
1014 | @@ -1015,7 +1015,7 @@ class CConnman 1015 | 1016 | // Network stats 1017 | void RecordBytesRecv(uint64_t bytes); 1018 | - void RecordBytesSent(uint64_t bytes); 1019 | + void RecordBytesSent(uint64_t bytes, bool increaseMaxOutbound);
nit, per doc/developer-notes.md current naming style is snakecase, e.g. increase_max_outbound
Approach ACK
Updated with increase_max_outbound
Another way to handle given TODO is to drop it altogether :)
If the goal is to allow download peers to bypass the limit, this is already done:
if (m_connman.OutboundTargetReached(true) &&
(((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) &&
!pfrom.HasPermission(NetPermissionFlags::Download) // nodes with the download permission may exceed target
If the goal is to drop the DOWNLOAD part from stats displayed to the user, then I'm not sure it's a good idea? It's good to see we sent a lot of bytes to the DOWNLOAD peer.
If the goal is to allow download peers to bypass the limit, this is already done:
The logic crosses layers and files, so maybe belt and suspenders (and could add documentation).
If the goal is to drop the DOWNLOAD part from stats displayed to the user, then I'm not sure it's a good idea? It's good to see we sent a lot of bytes to the DOWNLOAD peer.
ISTM totalbytessent in getnettotals would be unaffected, only bytes_left_in_cycle would be changed.
(Maybe the getnettotals help could be updated too.)
Okay, let me restate my idea. The changes only applies to the nodes with DOWNLOAD permission.
In the current master, such nodes "waste" bytesSent which could be used towards MEMPOOL messages by downloading historic blocks. After this change, they won't be accounted: no matter how much historic blocks they downloaded, that won't affect bandwidth limits for MEMPOOL. Code:
if (m_connman.OutboundTargetReached(false) && !pfrom.HasPermission(NetPermissionFlags::Mempool))
{
if (!pfrom.HasPermission(NetPermissionFlags::NoBan))
{
LogPrint(BCLog::NET, "mempool request with bandwidth limit reached, disconnect peer=%d\n", pfrom.GetId());
pfrom.fDisconnect = true;
}
return;
}
No other bandwidth limits apply to these kinds of nodes. So I think this is the only behavior change this code provides.
This is also what's effectively gonna be in bytes_left_in_cycle to the user.
I'm not particularly concerned about peers with a DOWNLOAD permission exploiting this new extended limit on MEMPOOL messages, but I'm also not sure this change improves anything.
I think the limits w.r.t DOWNLOAD peers are currently counter-intuitive (both for devs and for users), and the first thing we should do here is document this stuff better. We also might want to decide what to do with MEMPOOL messages w.r.t DOWNLOAD-permissioned peers more explicitly.
After that, I think we could remove the given TODO either by applying currently suggested code or just dropping it.
2855 | @@ -2856,7 +2856,7 @@ void CConnman::RecordBytesRecv(uint64_t bytes) 2856 | nTotalBytesRecv += bytes; 2857 | } 2858 | 2859 | -void CConnman::RecordBytesSent(uint64_t bytes) 2860 | +void CConnman::RecordBytesSent(uint64_t bytes, bool increase_max_outbound)
I don't find the name of this new variable particularly informative... how about bypass_limit?
the first thing we should do here is document this stuff better.
:+1:
After seeing these comments and reviewing the code, I agree with @naumenkogs, this change isn't really worth it. So I'll close this and submit another PR which will only remove the TODO
Removing TODO: https://github.com/bitcoin/bitcoin/pull/23128