Page MenuHomePhabricator

[seeder] Fix such that seeder node awaits both headers and addr messages
ClosedPublic

Authored by PiRK on Mon, Oct 14, 19:01.

Details

Summary

The current code assumes that if the ADDR message arrives, the node
is done. As such, it can miss out on the HEADERS message and incorrectly
mark nodes as bad.

This commit fixes the situation by making it that the node must both
receive ADDR messages *and* HEADERS messages to be satisfied.

Note that in some cases it won't be expecting ADDR messages (if
calling code didn't supply a vAddr pointer), and so in that case it will
just await a HEADERS reply iff we have a checkpoint.

With this commit, the seeder header verification facility should be working
now as expected.

Note that we had to update the unit tests to work ok with the logic
changes.

This is a partial backport of bchn#1820
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=4f544286f0e2092e0eb466708784f041d5462645

Depends on D16500

Test Plan

TBD (we need to run the seeder for a very long time or else tweak some code to make the node acceptation logic faster)

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK published this revision for review.Tue, Oct 15, 05:09
Fabien requested changes to this revision.Tue, Oct 15, 07:50
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/seeder/bitcoin.cpp
181 ↗(On Diff #50090)

this is useless ?

src/seeder/test/p2p_messaging_tests.cpp
135 ↗(On Diff #50090)
This revision now requires changes to proceed.Tue, Oct 15, 07:50

remove useless return, fix comment nit, use MainNet testing setup for new test (needed after changing HasCheckPoint in parent diff)

This revision is now accepted and ready to land.Tue, Oct 15, 19:33