Page MenuHomePhabricator

Dedup net message writing code in the seeder
ClosedPublic

Authored by jasonbcox on Jun 3 2020, 22:56.

Details

Reviewers
deadalnix
nakihito
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC2a8d9ce51932: Dedup net message writing code in the seeder
Summary

The message writing logic is duplicated in the seeder. Using the same code as the node
makes it easier to test, maintain, and add new message types for the seeder to support
(ie. header messages so that seeders can apply checkpoints).

Depends on D6355 and D8622

Test Plan
ninja
ninja check-bitcoin-seeder

Edit chainparams.cpp to replace all seeds in TestNet with a single entry: vSeeds.emplace_back("localhost");

src/bitcoind -testnet -whitelist=127.0.0.1 -listen=1 -connect=0 -debug=net

Wait for the node to finish startng up

src/seeder/bitcoin-seeder -host=localhost -ns=blah.bitframe.org -port=8888 -mbox=info@bitframe.org -wipeban -wipeignore -testnet

From the seeder, the output should start as:

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

And eventually become something like:

0/503 available (1 tried in 1s, 470 new, 32 active), 0 banned; 0 DNS requests, 0 db queries

Output from the node should be similar to the following:

Leaving InitialBlockDownload (latching to false)
Added connection peer=0
connection from 127.0.0.1:58994 accepted
received: version (111 bytes) peer=0
sending version (114 bytes) peer=0
send version message: version 70015, blocks=1383542, us=[::]:0, peer=0
Cleared nodestate for peer=0
sending verack (0 bytes) peer=0
receive version message: [127.0.0.1:58994] /bitcoin-cash-seeder:0.15/: version 70015, blocks=1378461, us=127.0.0.1:18333, peer=0
added time data, samples 2, offset +0 (+0 minutes)
received: verack (0 bytes) peer=0
sending sendheaders (0 bytes) peer=0
sending sendcmpct (9 bytes) peer=0
sending ping (8 bytes) peer=0
sending feefilter (8 bytes) peer=0
received: getaddr (0 bytes) peer=0
sending addr (30003 bytes) peer=0
socket closed
disconnecting peer=0
Cleared nodestate for peer=0

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jun 3 2020, 22:56
nakihito requested review of this revision.Jun 3 2020, 22:56
nakihito planned changes to this revision.

Fixed copyrights and removed some debug code.

nakihito requested review of this revision.Jun 3 2020, 23:18
deadalnix requested changes to this revision.Jun 4 2020, 09:00
deadalnix added a subscriber: deadalnix.

There are still no explanation as to what the problem is with the previous version of this code. The only significant way this is changing things is by keeping more copies of the messages in memory instead of streaming them, which does not seem to be a good move.

src/seeder/bitcoin.h
34 ↗(On Diff #20858)

I'm fairly confident that this is the wrong move.

This revision now requires changes to proceed.Jun 4 2020, 09:00

Reverted vSend to CDataStream and adjusted unit tests to reflect this.

Added additionally test case.

nakihito requested review of this revision.Jun 6 2020, 00:40

Updated summary to remove mention of previous seeder implementation being unable to send multiple messages. This was actually the result of a different bug that made it appear as if it was not possible.

Removed change to seeder/bitcoin.h copyright.

jasonbcox added a reviewer: nakihito.
  • Rebase ontop of linker fixes: D8622
  • Use netmagic from config instead of hardcoding the values in multiple places. (More cleanup can be done here, but out of scope of this patch)
  • Use TransportSerializer.prepareForTransport to de-dup header serialization code.
jasonbcox edited the test plan for this revision. (Show Details)
jasonbcox retitled this revision from Refactor seeder to use CNetMsgMaker to write its messages to Dedup net message writing code in the seeder.
jasonbcox edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Dec 8 2020, 00:38

More of a question that a change request, but please double check this.

src/seeder/messagewriter.h
34 ↗(On Diff #26255)

You want to move that vector to avoid a copy. But even then, is that really the way it is done in the rest of th codebase? That seems like it can create a lot of copies and be quite slow.

This revision now requires changes to proceed.Dec 8 2020, 00:38

This was a good question. There's multiple copies going on here and they aren't necessary.
While the handling of the outgoing data stream is not identical between the seeder and the
node software, the new WriteMessage function can easily write directly to the stream in
question instead of copying into an intermediate which then is copied into the outgoing stream.

I also cleaned up the tests a bit, since version was being passed around for no good reason.

This revision is now accepted and ready to land.Dec 9 2020, 23:48
This revision was automatically updated to reflect the committed changes.