Page MenuHomePhabricator

Increase seeder timeout for non-Tor connections
AbandonedPublic

Authored by nakihito on Apr 17 2020, 20:24.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

Currently, the seeder will timeout its connection with a node after 30
seconds on non-Tor connections. However, this can cause the connection
to timeout rather frequently because the node software also delays
sending addr messages by 30 seconds (see AVG_ADDRESS_BROADCAST_INTERVAL
in net_processing.cpp). Adding some buffer time for various situations
will prevent false negatives and unintended connection timeouts.
This change does not eliminate this issue. It simply reduces the probability that it will
occur.

Currently, it is too costly to pull AVG_ADDRESS_BROADCAST_INTERVAL
into the seeder, so a commment has also been added so the code shows up
if AVG_ADDRESS_BROADCAST_INTERVAL is grep'd for.

Test Plan
ninja
src/bitcoind --testnet -whitelist=127.0.0.1 -listen=1 -connect=0 -debug=net
src/seeder/bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org --testnet --wipeignore --wipeban

Seeder output should start as:

0/2 available (0 tried in 0s, 0 new, 2 active), 0 banned; 0 DNS requests, 0 db queries

After successfully connecting and receiving addrs, the seeder output should become:

0/695 available (2 tried in 4s, 541 new, 152 active), 0 banned; 0 DNS requests, 0 db queries

testing diff based off master:


bitcoind log from master failure using the above testing diff:

seeder output from master failure using the above testing diff:

bitcoind log using the testing diff above rebased off this patch:

Diff Detail

Repository
rABC Bitcoin ABC
Branch
IncreaseTimeoutForSeeder
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10283
Build 18387: Default Diff Build & Tests
Build 18386: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Apr 17 2020, 20:24
jasonbcox requested changes to this revision.Apr 17 2020, 22:48
jasonbcox added a subscriber: jasonbcox.

To make the test plan a little more compelling, is it easy to reproduce the problematic scenario that is fixed by this patch such that you can share logs to show the difference?

src/seeder/bitcoin.h
46

If you're making one a constant, why not the other? seems only fitting since you're working on this patch.

This is a nit. Or you can do this in another diff. Not a big deal either way.

This revision now requires changes to proceed.Apr 17 2020, 22:48

Reproduction is difficult~ish. I can add logs of this bug using one of my testing branches.

src/seeder/bitcoin.h
46

That will be a different patch. This patch already changes behavior in addition to adding a new constant. Adding another constant would broaden the scope and complexity of this patch too much I think.

nakihito edited the summary of this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Apr 20 2020, 18:05
deadalnix added a subscriber: deadalnix.

The test plan does demonstrate what the problem was before. What are looking at? What changed?

I think I understand the problem looking at net_processing, but there are many moving pieces and I'm really not sure.

This revision now requires changes to proceed.Apr 20 2020, 18:05
nakihito edited the test plan for this revision. (Show Details)

I cannot replicate this issue consistently.