Page MenuHomePhabricator

Allow Seeder to handle receiving HEADER messages
AbandonedPublic

Authored by nakihito on Nov 13 2019, 20:55.

Details

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

In order to filter nodes based on whether they follow chain params or
not, the seeder needs to be able to handle the HEADER
messages after sending a GETHEADER message. This diff only covers the
seeder's ability to receive and process the new type of message and ban
nodes that are not following the correct chain.

Note: the seeder cannot actually reach this new logic yet because it has no
method of requesting a HEADERS message.

Depends on D4436

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
NewMsgTypesForSeeder
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Owners added a reviewer: Restricted Owners Package.Nov 13 2019, 20:56

Rebase, split test into two different tests, removed NOTFOUND message functionality because it is not necessary, added differentiating logic for an empty HEADERS message and a non-empty HEADERS message. Also added some more constants.

nakihito retitled this revision from Allow Seeder to handle receiving NOTFOUND and HEADER messages to Allow Seeder to handle receiving HEADER messages.Nov 14 2019, 04:25
nakihito edited the summary of this revision. (Show Details)

Squashed some missed changed.

deadalnix requested changes to this revision.Nov 14 2019, 15:33

The iff description doesn't match what this does, because it clearly does more than simply receiving and processing the headers messages, it also filter/ban nodes based on it.

src/seeder/bitcoin.cpp
177 ↗(On Diff #14122)

Banning based on this doesn't seems to make a whole lot of sense.

First, as far as I can tell, a node that do not have the assumevalid block will simply return the 2000 first block it knows, so it's not going to trip here, but worse:
1/ You'll ban nodes doing IBD.
2/ You won't have the information to know if the node is a BCH node or note based on the 2000 first blocks. It could be following the BTC or the BSV chain or simply not having finished IBD.

Lastly, assumevalid means that we assume the block is valid (I know, right ?!). The block being valid doesn't necessarily imply that no other block is valid. But we assume this specific one is.

What you actually want to compare against are checkpoints.

181 ↗(On Diff #14122)

Why is it that you want to continue to process messages from that node AFTER you decided to ban it?

185 ↗(On Diff #14122)

Clearly, you did not gpt a version message here, but headers.

src/seeder/bitcoin.h
8 ↗(On Diff #14122)

There is no modification in that header, so it is unlikely that new includes are required.

src/seeder/test/seeder_tests.cpp
48 ↗(On Diff #14122)

This is not a message that exists.

183 ↗(On Diff #14122)

The lack of if statement bellow makes it very clear that this is NOT the size of the message. the size of the message is also very clearly not 0 or 1.

396 ↗(On Diff #14122)

Presumably you'd want to test that the node is not banned.

410 ↗(On Diff #14122)

This is not really testing what we need/want to be testing. We want to make sure we keep nodes that follow a certain chain and reject these that do not. There are intermediary steps here, but it is clear that this test is not helping because the does the completely wrong thing, and the test effectively test the corresponding wrong behavior.

This revision now requires changes to proceed.Nov 14 2019, 15:33
nakihito edited the summary of this revision. (Show Details)

Seeder now requests the blocks that come after the most recent checkpoint and uses that information to determine whether the peer is on the correct chain.

Investigating build failure.

src/seeder/bitcoin.cpp
177 ↗(On Diff #14122)

I have changed the code to check against a checkpoint (the most recent checkpoint specifically). Additionally, I have verified in D4442 that the behavior of a node in response to a GETHEADERS message with a locator hash that the node does not have is to send a HEADERS message without any headers.

I think nodes that are undergoing IBD and have yet to reach the latest checkpoint should be banned. They are not up to date and likely have low up time either because they just started up or because they are periodically down. Whatever the reason, they are not good seeds at the time of being checked. Because bans are not permanent, the seeder will likely come back around to a node that was undergoing IDB at a later time after it has sync'd more of the chain.

Rebased and updated to reflect changes D4436.