net: 'break' should be 'continue' here? #14775

pull dooglus wants to merge 1 commits into bitcoin:master from dooglus:patch-6 changing 1 files +1 −1

dooglus commented at 5:10 AM on November 21, 2018: contributor

If we 'break' rather than 'continue' here, we very rarely get to the 30 failed attempts needed to consider very recently tried nodes, since the break resets the nTries to zero.

'break' should be 'continue' here?

If we 'break' rather than 'continue' here, we very rarely get to the 30 failed attempts needed to consider very recently tried nodes, since the break resets the nTries to zero.

088c217624

fanquake added the label P2P on Nov 21, 2018
gmaxwell commented at 5:12 AM on November 21, 2018: contributor

Erp. You look correct to me.

utACK.
kallewoof commented at 5:20 AM on November 21, 2018: member

I'm not sure. If all addresses are invalid, you will go into an infinite loop with continue;. Note that there are multiple while loops going on here.
dooglus commented at 5:27 AM on November 21, 2018: contributor

I'm by no means sure either. The infinite loop can be fixed by moving the nTries++ and if (nTries > 100) test up to the top of the inner loop.
kallewoof commented at 5:40 AM on November 21, 2018: member
The relevant commit for this change appears to be https://github.com/bitcoin/bitcoin/commit/5fee401fe14aa6459428a26a82f764db70a6a0b9 by @sipa. Maybe he can shed light on the matter (though it was in 2012).

On line 1402-1402, this is removed:
```
if (!addr.IsIPv4() || !addr.IsValid() || setConnected.count(addr.GetGroup()))	
    continue;
```
and (line 1351-1352) this is added:
```
if (!addr.IsIPv4() || !addr.IsValid() || setConnected.count(addr.GetGroup()) || addr == addrLocalHost)
    break;
```
so the code explicitly went from continue to break in this commit.

in src/net.cpp:1870 in 088c217624

1866 | @@ -1867,7 +1867,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1867 |  
1868 |              // if we selected an invalid address, restart
1869 |              if (!addr.IsValid() || setConnected.count(addr.GetGroup()) || IsLocal(addr))
1870 | -                break;
1871 | +                continue;

promag commented at 1:46 PM on November 21, 2018:

By breaking it will jump to L1787, which is what "restart" means?

MarcoFalke commented at 7:18 PM on November 22, 2018: member

Looks like this change will eat all CPU. Would be nice to have a test to accompany this refactoring and ensure coverage or (in case it is a bugfix) a test that fails on current master and passes with this fix.
laanwj renamed this:
~~'break' should be 'continue' here?~~
net: 'break' should be 'continue' here?
on Nov 23, 2018
jonasschnelli assigned theuni on Dec 9, 2018
laanwj commented at 3:38 PM on January 8, 2019: member

Discussion here kind of died out
sipa commented at 3:50 PM on January 8, 2019: member

I'm pretty sure this PR is wrong. continue could be used, but then at least nTries would need to be incremented.

I also believe the original choice to restart the outer loop in this case was by design, but I don't remember what it was.
theuni commented at 11:18 PM on January 8, 2019: member

Agree with @sipa. I also can't recall the historical reasoning. It's very possible that that reasoning no longer applies with the new features that have been added over the years.

The selection algorithm has been tweaked enough times, IMO it'd be nice to dissect and document the current behavior before changing anything. It's not exactly clear, and I suspect there are several areas for improvement.
MarcoFalke commented at 10:01 PM on February 26, 2019: member

No activity after the pull was opened. Closing for now, let me know when you want to work on this again.
MarcoFalke closed this on Feb 26, 2019
DrahtBot commented at 10:21 PM on February 26, 2019: 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:
- #15486 ([net] Allow feeler connections to go to the same netgroup as existing outbound peers by sdaftuar)
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 locked this on Dec 16, 2021

Contributors

Labels

net: 'break' should be 'continue' here? #14775

Conflicts