Always drop the least preferred HB peer when adding a new one. #9199

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:remove_high_bandwidth_zombies changing 1 files +2 −3

gmaxwell commented at 2:55 AM on November 22, 2016: contributor

When a BIP152 HB-mode peer is in the least preferred position and disconnects, they will not be by ForNode on the next loop. They will continue to sit in that position and prevent deactivating HB mode for peers that are still connected.

There is no reason for them to stay in the list if already gone, so drop the first element unconditionally if there are too many.

Fixes issue #9163.

Always drop the least preferred HB peer when adding a new one.

When a BIP152 HB-mode peer is in the least preferred position and
 disconnects, they will not be by ForNode on the next loop. They
 will continue to sit in that position and prevent deactivating
 HB mode for peers that are still connected.

There is no reason for them to stay in the list if already gone,
 so drop the first element unconditionally if there are too many.

Fixes issue #9163.

ca8549d2bd

gmaxwell commented at 2:55 AM on November 22, 2016: contributor

Needs backport.
fanquake added this to the milestone 0.13.2 on Nov 22, 2016
fanquake added the label Needs backport on Nov 22, 2016

in src/main.cpp:None in ca8549d2bd

 528 | @@ -529,12 +529,11 @@ void MaybeSetPeerAsAnnouncingHeaderAndIDs(const CNodeState* nodestate, CNode* pf
 529 |          if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
 530 |              // As per BIP152, we only get 3 of our peers to announce
 531 |              // blocks using compact encodings.
 532 | -            bool found = connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion](CNode* pnodeStop){
 533 | +            connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion](CNode* pnodeStop){
 534 |                  connman.PushMessage(pnodeStop, NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion);
 535 |                  return true;

sipa commented at 3:11 AM on November 22, 2016:

Can this line go as well?

gmaxwell commented at 4:29 AM on November 22, 2016:

meh, the prototype of the lambda here is that it returns bool, just dropping the return should produce a warning that a function with a non-void return type can fail to return a value if the compiler is at all sane. Changing the prototype of ForNode seems unreasonable to me... the return value should be useful.

sipa commented at 4:29 AM on November 22, 2016:

Ok!

theuni commented at 6:12 PM on November 22, 2016:

Since there's only 1 user of ForNode, and since it's terribly awkward for that use, I think we should just replace it by adding an extra:

CConnman::PushMessage(NodeId id, ...);

We can do that as a follow-up to #9128.

gmaxwell commented at 5:53 PM on November 22, 2016: contributor

@TheBlueMatt please reivew
morcos commented at 6:25 PM on November 22, 2016: member

seems reasonable to me utACK
theuni commented at 6:41 PM on November 22, 2016: member

Why not just erase the id from lNodesAnnouncingHeaderAndIDs in FinalizeNode()?
sdaftuar commented at 6:52 PM on November 22, 2016: member

@theuni This still seems cleaner because I think sometimes nodes hang out in a semi-disconnected state sometimes, with a delay between when they're removed from vNodes and when FinalizeNode gets called.
theuni commented at 6:57 PM on November 22, 2016: member

@sdaftuar Ah yes, good point.
sdaftuar commented at 10:18 PM on November 22, 2016: member

utACK
sipa commented at 10:18 PM on November 22, 2016: member

utACK
TheBlueMatt commented at 8:35 AM on November 23, 2016: member

utACK ca8549d2bd32f17f8b69d1edbe3f2976fba504b4
gmaxwell commented at 9:10 AM on November 23, 2016: contributor

Why not just erase the id from lNodesAnnouncingHeaderAndIDs in FinalizeNode()?

I thought about that, but it's actually irrelevant until a new peer sends a block in any case... and would still need this code regardless, to care care of peers that are removed but not finalized yet.

It might make more sense in the future if we increase the list beyond three in order to have some 'spares' that can be activated when a HB selected peer goes offline (so when a HB peer goes offline we'd activate one of the spares)... certainly not something to do for a fix that needs to get backported though. :)
gmaxwell commented at 10:50 PM on November 23, 2016: contributor

FWIW, on my test host a HB selected peer dropped and it successfully removed HB status from a peer two blocks later with this change.
sipa merged this on Nov 24, 2016
sipa closed this on Nov 24, 2016
sipa referenced this in commit 407d9232ef on Nov 24, 2016
laanwj referenced this in commit c6e45d6d62 on Dec 2, 2016
laanwj removed the label Needs backport on Dec 2, 2016
laanwj commented at 7:22 AM on December 2, 2016: member

Backported in #9264, removing tag.
laanwj referenced this in commit da5a16b11d on Dec 2, 2016
laanwj added the label P2P on Dec 19, 2016
gladcow referenced this in commit 599968e9e7 on Mar 5, 2018
gladcow referenced this in commit 0ac41a7926 on Mar 8, 2018
gladcow referenced this in commit df8c826a16 on Mar 13, 2018
gladcow referenced this in commit 3f50661d4f on Mar 14, 2018
gladcow referenced this in commit 7ef2ebc078 on Mar 15, 2018
gladcow referenced this in commit 31094f3838 on Mar 15, 2018
gladcow referenced this in commit c7263be51e on Mar 15, 2018
gladcow referenced this in commit 7aec47a044 on Mar 15, 2018
gladcow referenced this in commit 24d458409f on Mar 24, 2018
gladcow referenced this in commit 7773173742 on Apr 4, 2018
UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
lateminer referenced this in commit 6689f09a0a on Oct 24, 2018
andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.13.2

Linked (view graph)

#9163 inundated with cmpctblocks #9264 0.13.2 backports