Page MenuHomePhabricator

Seeder: Verify that nodes are on the correct chain
ClosedPublic

Authored by PiRK on Jul 15 2024, 07:39.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC74baf6484f44: Seeder: Verify that nodes are on the correct chain
Summary

Since there have been 2 splits of the BCH chain that haven't modified
their network magic, the seeder considers a lot of nodes as good that
are actually on the wrong chain.

To fix this, the seeder will check the headers of all nodes synced
higher than the last checkpoint's height. To do this, a P2P getheaders
with the last checkpoint's hash as locator is sent, the first returned
header's previous block hash is compared to checkpoint's hash and the
node is banned if they do not match.

This is a backport of bchn#1040

Note that the nodes are considered "good" until proven otherwise, so the seeder will still mistakenly consider BCH nodes to be good for a limited duration (until the HEADERS message is received), or even permanently if the HEADERS message is not yet received when the connection is closed.
However, this is still a net improvement compared to the previous behavior, as most BCH nodes are now banned and not written to the database. The behavior can be improved further in a separate diff (see https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=4c8604800ba9ea67c61dd8c7c426d260004d853e)

Test Plan
  • ninja check-bitcoin-seeder
  • Uncomment the wrong chain print in src/seeder/bitcoin.cpp
  • ninja
  • Run a mainnet seeder: src/seeder/bitcoin-seeder -dumpinterval=60
  • Observe node versions being banned, should not include eCash nodes.
  • Observe which nodes are ending up as good in dnsseed.dump: awk '{ if ($2 == 1) { print } }' dnsseed.dump

Event Timeline

PiRK requested review of this revision.Jul 15 2024, 07:39
src/seeder/bitcoin.h
26–32 ↗(On Diff #48624)

I'm not sure what the best approach is to include this constant instead of duplicating it. Currently it is defined in net_processing.cpp. It should be moved to somewhere that can be included here without inflating the seeder includes too much, but where?

Fabien requested changes to this revision.Jul 15 2024, 08:36
Fabien added a subscriber: Fabien.

You should add tests as you go

src/seeder/bitcoin.cpp
127 ↗(On Diff #48624)

uint64_t. Note that ReadCompactSize returns an uint64_t but raises an exception if the size is larger than uint64_t MAX_SIZE = 0x02000000.
This means that nCound cannot overflow, but also that we need to make sure the seeder will catch the exception and won't crash if such a message is sent.

139 ↗(On Diff #48624)

Which is not what the below code checks.

src/seeder/bitcoin.h
26–32 ↗(On Diff #48624)

Could be net_processing.h. Also it should be an uint64_t since you're comparing to another uint64_t.

29 ↗(On Diff #48624)

layout

This revision now requires changes to proceed.Jul 15 2024, 08:36

improve comment, set range_check=false when calling ReadCompactSize to prevent the function from throwing if the net message is intentionnaly bad, don't duplicate MAX_HEADERS_RESULTS but include it via net_processing.h.

I prefer not to touch the type of MAX_HEADERS_RESULTS in this diff, as I don't want to risk having a seeder commit break something elsewhere in the codebase.

PiRK planned changes to this revision.Jul 15 2024, 10:02

This one should be relatively easy to test (but hopefully without having to add a dependency to the main unit test framework like the source material does)

src/seeder/bitcoin.cpp
128 ↗(On Diff #48628)

Skipping the check is not the right approach, the call can throw even without this. Just check the exceptions are properly catched, it's likely the case already otherwise any deserialization error could crash the seeder

PiRK planned changes to this revision.Jul 15 2024, 13:10
src/seeder/bitcoin.cpp
128 ↗(On Diff #48628)

I confirm that there is code catching the error in main.cpp and marking the node as bad in such a case.

no need to disable the range_check, the error is caught in main.cpp::ThreadCrawler

Fabien added inline comments.
src/seeder/bitcoin.cpp
142 ↗(On Diff #48630)

The empty check is not very useful as e.g. the getheaders message as no such check, but it doesn't hurt

This revision is now accepted and ready to land.Jul 15 2024, 15:01
src/seeder/bitcoin.cpp
142 ↗(On Diff #48630)

Yes, it was noted in recent BCHN discussions that the seeder cannot work for new chains that don't yet have a checkpoint. There is some work that can be backported in case we ever need a seeder for such a chain, and in the meantime it is best to have new code assume that it should work with or without checkpoints