Page MenuHomePhabricator

[seeder] Require good seeds to have latest checkpoint
Needs RevisionPublic

Authored by jasonbcox on Feb 28 2020, 23:46.

Details

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

If a peer provides headers that do not match the checkpoint, ban them.
This will not only provide higher quality seeds but also make it easier to supply
seeds for both BCHN and BCHA networks.

Depends on D5682 and D6283
Depends on D8673 for the test plan.

Test Plan

Build + unit tests:

ninja bitcoin-seeder check-seeder

Manual tests:

cd build/src

Between each test, cleanup seeder files with:

rm dns*

(1) IBD:

mkdir ibdtest
./bitcoind -testnet -datadir=ibdtest

Let the node run for a while to begin IBD.

./bitcoin-seeder -host=localhost -ns=localhost -port=8888 -mbox=noemail@bitcoinabc.org --testnet --wipeban --wipeignore -overridednsseed=localhost

The seeder should attempt to connect to the localhost node.

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

Notably, 0/x available, x should become larger than 2, and x new and x active should grow larger than 0.

(2) Up-to-date chain:

./bitcoind -testnet -whitelist=127.0.0.1 -listen=1 -connect=0
./bitcoin-seeder -host=localhost -ns=localhost -port=8888 -mbox=noemail@bitcoinabc.org --testnet --wipeban --wipeignore -overridednsseed=localhost

Output from the seeder should be similar to:

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

If you leave the seeder running for long enough, 127.0.0.1 will appear in dnsseed.dump

(3) Wrong chain:

./bitcoind -testnet -whitelist=127.0.0.1 -listen=1 -connect=0
./bitcoin-cli -testnet reconsiderblock 000000000fc2ff8fc6585b71961c6ca0ecea24fa52746cb5484256019891e448
./bitcoin-cli -testnet invalidateblock 00000000062c7f32591d883c99fc89ebe74a83287c0f2b7ffeef72e62217d40b   # hash of the Axion checkpoint
./bitcoin-seeder -host=localhost -ns=localhost -port=8888 -mbox=noemail@bitcoinabc.org --testnet --wipeban --wipeignore -overridednsseed=localhost
0/722 available (2 tried in 1s, 672 new, 48 active), 0 banned; 0 DNS requests, 0 db queries

The banning logic is a bit hard to test here since the ban will only occur if the node has lots of peers. This is mostly a sanity check that we still get peers from this node.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
NewMsgTypesForSeeder
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9756
Build 17391: Default Diff Build & Tests
Build 17390: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
src/seeder/bitcoin.cpp
194 ↗(On Diff #18925)

Any reason this comment needs three lines?

198 ↗(On Diff #18925)

Maybe clarify this bit with a comment for posterity:

// We're only finished if the ADDR_SOFT_CAP has been reached
src/seeder/test/p2p_messaging_tests.cpp
261 ↗(On Diff #18925)

Also test for the case where some addrs are received before and after headers. This way, we'll know that none were dropped and that the seeder still handles this case properly without making assumptions about the internals.

This revision now requires changes to proceed.Apr 23 2020, 00:16

Added comments, fixed naming, and increased unit testing coverage.

Seeder bans all nodes that return header messages where the first block is NOT a block that comes after the most recent checkpoint. Additionally, only nodes that return a correct header message are considered good. Nodes that return no header are considered bad.

nakihito requested review of this revision.May 1 2020, 21:25
nakihito retitled this revision from Seeder requires seeds to have block after most recent checkpoint in its chain to Seeder requires good seeds to have block after most recent checkpoint in its chain.
nakihito edited the summary of this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)

The unit test coverage is pretty good but the test plan is a giant warning sign about how critical it is that we get moving on functional tests for the seeder.

deadalnix requested changes to this revision.May 17 2020, 16:57
deadalnix added inline comments.
src/seeder/bitcoin.cpp
127 ↗(On Diff #19353)

Looks like this is a refactoring that can be done on its own.

162 ↗(On Diff #19353)

Why is this still playing with bans?

203 ↗(On Diff #19353)

This is fragile spaghetti code. Now the processing of these messages will be spread half way accross various blocks of code. It also doesn't really make sense to me why addr needs to trigger a getheaders request.

src/seeder/bitcoin.h
19 ↗(On Diff #19353)

MAX_HEADERS_RESULTS ?

This revision now requires changes to proceed.May 17 2020, 16:57
nakihito retitled this revision from Seeder requires good seeds to have block after most recent checkpoint in its chain to Seeder requires good seeds to have block after most recent checkpoint in its chain and bans those that don't.May 20 2020, 18:19
nakihito edited the summary of this revision. (Show Details)

MAX_HEADERS_PER_MESSAGE -> MAX_HEADERS_RESULTS. Restructured messaging sequence to request headers before addrs.

deadalnix requested changes to this revision.May 27 2020, 12:21

It seems very difficult to me to ensure this correct in any meaningful way. There are many moving pieces and they are not weel encapsulated and are done all at once.

There are at least two obvious components in this diff. 1/ is the mechanism that triggers the request for headers and the other the processing of the response. Separating both and testing them individually would be way preferable.

It is also unclear to me how cases such as the node being in IBD - in which case it won't respond the getheaders is handled.

src/seeder/bitcoin.cpp
142 ↗(On Diff #20378)

The correct behavior upon receiving an oversized message is to ban. You really should be looking at how this is already implemented in the node instead of redoing all the mistake that were done in the past reinventing the wheel.

src/seeder/bitcoin.h
19 ↗(On Diff #20378)

This already exists in the codebase.

This revision now requires changes to proceed.May 27 2020, 12:21

Rebased off D6283. Restored message request order to version -> verack -> addr -> header because the state transitions are simpler when a node is IBD. Added server module when building to make use of MAX_HEADERS_RESULTS.

nakihito edited the test plan for this revision. (Show Details)

Rebased and changed the finish state to reflect changes to the getheaders trigger.

nakihito requested review of this revision.Jun 6 2020, 00:41
jasonbcox edited reviewers, added: nakihito; removed: jasonbcox.
jasonbcox retitled this revision from Seeder requires good seeds to have block after most recent checkpoint in its chain and bans those that don't to [seeder] Require good seeds to have latest checkpoint.Dec 12 2020, 00:12
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox edited the summary of this revision. (Show Details)
  • Refactor and simplify the logic tracking checkpoint state
  • Refactor and simplify the tests
  • Rewrite summary and test plan
  • New changes depend on D8673 to make the test plan easier to understand by simple examination of the command line arguments in each step.
deadalnix requested changes to this revision.Dec 15 2020, 02:31

This looks good overall, but you really should refactor the state machine/message processing a bit. Most of the logic can stay similar.

src/seeder/bitcoin.cpp
104 ↗(On Diff #26372)

There is clearly some duplication here. You main issue comes from the fact that message parsing/processing is intermingled with the logic of the seeder's state machine. This is just bound to cause problems. For instance, if the remote node send response out of order - which is allowed by the protocol - then this won't be good. I think some refactoring is in order.

155 ↗(On Diff #26372)

See previous comment. It is hard to justify doing anything with he address soft cap in the processing of received headers. While I understand that the seeder is a mess to begin with, making it worse should be avoided.

This revision now requires changes to proceed.Dec 15 2020, 02:31