Page MenuHomePhabricator

Allow Seeder to handle receiving HEADER messages
Changes PlannedPublic

Authored by nakihito on Wed, Nov 13, 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 OK
Unit
No Unit Test Coverage

Event Timeline

nakihito created this revision.Wed, Nov 13, 20:55
Owners added a reviewer: Restricted Owners Package.Wed, Nov 13, 20:56
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Nov 13, 20:56
nakihito planned changes to this revision.Wed, Nov 13, 20:56
nakihito updated this revision to Diff 14120.Thu, Nov 14, 04:24

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 planned changes to this revision.Thu, Nov 14, 04:24
nakihito retitled this revision from Allow Seeder to handle receiving NOTFOUND and HEADER messages to Allow Seeder to handle receiving HEADER messages.Thu, Nov 14, 04:25
nakihito edited the summary of this revision. (Show Details)
nakihito updated this revision to Diff 14122.Thu, Nov 14, 04:37

Squashed some missed changed.

nakihito planned changes to this revision.Thu, Nov 14, 04:37
nakihito requested review of this revision.Thu, Nov 14, 04:41
deadalnix requested changes to this revision.Thu, Nov 14, 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.Thu, Nov 14, 15:33
nakihito edited the summary of this revision. (Show Details)Fri, Nov 15, 06:45
nakihito edited the summary of this revision. (Show Details)
nakihito edited the summary of this revision. (Show Details)Fri, Nov 15, 06:47
nakihito updated this revision to Diff 14144.Fri, Nov 15, 12:25

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.

nakihito planned changes to this revision.Fri, Nov 15, 13:01

Investigating build failure.

nakihito updated this revision to Diff 14158.Fri, Nov 15, 20:19

Rebase.

nakihito added inline comments.Fri, Nov 15, 20:41
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.

nakihito planned changes to this revision.Mon, Nov 18, 22:49