Details
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
- arcpatch-D5380
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 14408 Build 28787: Build Diff build-without-wallet · build-debug · build-diff · build-clang · build-clang-tidy · lint-circular-dependencies Build 28786: arc lint + arc unit
Event Timeline
| 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. |
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.
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.
| 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 ? |
MAX_HEADERS_PER_MESSAGE -> MAX_HEADERS_RESULTS. Restructured messaging sequence to request headers before addrs.
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. |
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.
- 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.
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 | 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 | 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. | |