[WIP] Feeler code and debugging. #8596

pull rebroad wants to merge 1 commits into bitcoin:master from rebroad:FeelerFixes changing 3 files +13 −14

rebroad commented at 2:24 AM on August 26, 2016: contributor

Some changes to feeler code - improve debugging (to use less lines in debug.log). Change tried numbers from hours to days to make more readable.

Increase the frequency of feeler connections to average to 1 minute instead of 2 and move the definition of this variable to net.cpp instead of net.h as it's not used by any other .cpp files.

Also, disable requesting of headers to Feeler connections (since we usually disconnect before receiving them anyway), and move the disconnect logic to after the version message has been displayed (otherwise the debug.log often shows the version message after the disconnect message, which could be confusing).
Improvement to Feeler code and debugging. a0a2f0816a
rebroad renamed this:
~~Feeler code and debugging.~~
[WIP] Feeler code and debugging.
on Aug 26, 2016
rebroad renamed this:
~~[WIP] Feeler code and debugging.~~
Feeler code and debugging.
on Aug 26, 2016
jonasschnelli added the label P2P on Aug 26, 2016
MarcoFalke commented at 8:30 AM on August 26, 2016: member

If you are trying to achieve three different and orthogonal goals, I think it would make sense to split those into separate commits.

Or even two pulls: One for changing the p2p behavior, the other to change debug prints.
in src/net.cpp:None in a0a2f0816a
```
 384 | @@ -383,7 +385,7 @@ CNode* FindNode(const NodeId nodeid)
 385 |      return NULL;
 386 |  }
 387 |  
 388 | -CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure)
 389 | +CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, bool fFeeler)
```
EthanHeilman commented at 7:28 PM on August 26, 2016:

Is it worth the complexity of adding another flag to ConnectNode if all the flag does is alter a log message? Setting this flag false does not make the connection not a feeler connection.

sipa commented at 7:37 PM on August 26, 2016:

I think this is unnecessary and confusing, indeed. We shouldn't pass around state just for nitty logging purposes.

rebroad commented at 2:48 AM on August 27, 2016:

Ok. How should it be done?

rebroad commented at 2:51 AM on August 27, 2016:

Will fix.
in src/net.cpp:None in a0a2f0816a
```
  44 | @@ -45,6 +45,8 @@
  45 |  
  46 |  // We add a random period time (0 to 1 seconds) to feeler connections to prevent synchronization.
  47 |  #define FEELER_SLEEP_WINDOW 1
  48 | +/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
  49 | +static const int FEELER_INTERVAL = 60;
```
EthanHeilman commented at 7:30 PM on August 26, 2016:

If we are going to move the FEELER_INTERVAL to net.cpp why not move the other static const int intervals such as PING_INTERVAL or TIMEOUT_INTERVAL in net.h as well?

rebroad commented at 2:52 AM on August 27, 2016:

@EthanHeilman My next commit will remove FEELER_SLEEP_WINDOW as it's redundant. I.e. RAND(60)+RAND(1) = RAND(61)... i.e. we don't need two random numbers.
EthanHeilman commented at 4:29 PM on August 27, 2016:

We have two sources of randomness:

One FEELER_SLEEP_WINDOW is to protect against synchronization and must be random: see discussion on feeler pull request and discussion in design statement of feeler connections under risk mitigation,

The other FEELER_INTERVAL is to rate limit feeler connections but @sipa proposed it be random as well to limit practicability.

You didn't address my criticism about only moving FEELER_INTERVAL and not any of the other intervals, what does deleting FEELER_SLEEP_WINDOW have to do with that?
rebroad commented at 4:12 AM on August 29, 2016:

@EthanHeilman will address shortly sorry for the delay
in src/net.cpp:None in a0a2f0816a
```
  44 | @@ -45,6 +45,8 @@
  45 |  
  46 |  // We add a random period time (0 to 1 seconds) to feeler connections to prevent synchronization.
  47 |  #define FEELER_SLEEP_WINDOW 1
  48 | +/** Run the feeler connection loop once every 2 minutes or 120 seconds. **/
```
EthanHeilman commented at 7:31 PM on August 26, 2016:

Comment should reflect that FEELER_INTERVAL has been changed to 1 minute/60 seconds.

rebroad commented at 2:53 AM on August 27, 2016:

@Titled changed back to WIP - I was being way too optimistic!
rebroad renamed this:
~~Feeler code and debugging.~~
[WIP] Feeler code and debugging.
on Aug 27, 2016
gmaxwell commented at 1:33 AM on August 29, 2016: contributor

Increase the frequency of feeler connections to average to 1 minute instead of 2

Why??
rebroad commented at 4:25 AM on August 29, 2016: contributor

@gmaxwell Inpatience? I also don't see why it needs to take as long as it does to purge the unused addresses. Personally I'd rather it was shorter than 60 seconds. Perhaps it could be made a command-line option...
rebroad closed this on Sep 14, 2016
DrahtBot locked this on Sep 8, 2021

Contributors

Labels