[peer] Refactor peer code into its own package #445

tuxcanfly · 2015-06-03T13:43:54Z

This pull request introduces package peer which contains peer related features
refactored from peer.go.

The following is an overview of the features the package provides:

Provides a basic concurrent safe bitcoin peer for handling bitcoin
communications via the peer-to-peer protocol
Full duplex reading and writing of bitcoin protocol messages
Automatic handling of the initial handshake process including protocol
version negotiation
Automatic periodic keep-alive pinging and pong responses
Asynchronous message queuing of outbound messages with optional
channel for notification when the message is actually sent
Inventory message batching and send trickling with known inventory
detection and avoidance
Ability to wait for shutdown/disconnect
Flexible peer configuration
- Caller is responsible for creating outgoing connections and listening
  for incoming connections so they have flexibility to establish
  connections as they see fit (proxies, etc.)
- User agent name and version
- Bitcoin network
- Service support signalling (full nodes, bloom filters, etc.)
- Maximum supported protocol version
- Ability to register callbacks for handling bitcoin protocol messages
Proper handling of bloom filter related commands when the caller does
not specify the related flag to signal support
- Disconnects the peer when the protocol version is high enough
- Does not invoke the related callbacks for older protocol versions
Snapshottable peer statistics such as the total number of bytes read
and written, the remote address, user agent, and negotiated protocol
version
Helper functions for pushing addresses, getblocks, getheaders, and
reject messages
- These could all be sent manually via the standard message output
  function, but the helpers provide additional nice functionality such
  as duplicate filtering and address randomization
Full documentation with example usage
Test coverage

In addition to the addition of the new package, btcd has been refactored
to make use of the new package by extending the basic peer it provides to
work with the blockmanager and server to act as a full node. The
following is a broad overview of the changes to integrate the package:

The server is responsible for all connection management including
persistent peers and banning
Callbacks for all messages that are required to implement a full node
are registered
Logic necessary to serve data and behave as a full node is now in the
callback registered with the peer

Finally, the following peer-related things have been improved as a part
of this refactor:

Don't log or send reject message due to peer disconnects
Remove trace logs that aren't particularly helpful
Finish an old TODO to switch the queue WaitGroup over to a channel
Improve various comments and fix some code consistency cases
Improve a few logging bits
Implement a most-recently-used nonce tracking for detecting self
connections and generate a unique nonce for each peer

tuxcanfly · 2015-06-12T14:52:41Z

Update: Moved all message handlers into respective listeners. Listeners can be added and removed using a key (string) and all listeners are invoked when a message is received.

Looking into a couple of issues which cropped up while testing:

hang on btcctl peer commands
no pong replies to ping messages
hang on shutdown on outbound peer

davecgh · 2015-06-15T15:29:56Z

I'll be giving this a thorough review over the coming days, but a few points from a very quick review are:

I think there is too much state related to the block manager left in the peer (requested transactions, requested blocks, etc). Keep in mind the point of having this peer code separated is common code for full nodes, SPV nodes, proxies, etc. State related to the full node's block manager needs to be in the block manager, not the peer.
The semantic version bits don't belong in the peer code. That is something defined by the caller, not the peer.
The callback registration bits should be defined at the file level as functions rather than function pointers to closures.
All of the callback functions that are being registered need comments
The peer/log.go is completely wrong. It should be like blockchain/log.go, not have all of the btcd subsystems.

davecgh · 2015-06-15T18:06:08Z

blockmanager.go

 	// Don't accept more transactions if we're shutting down.
 	if atomic.LoadInt32(&b.shutdown) != 0 {
-		p.txProcessed <- struct{}{}
+		p.TxProcessed <- struct{}{}


I believe we're going to want a better mechanism for this. It is mixing peer and block manager state and won't fit in well with other callers.

davecgh · 2015-06-15T19:15:08Z

Also, before this is ready for merge it will need the standard fare of package documentation and a full suite of test coverage.

doc.go
README.md
example_test.go
peer_test.go

tuxcanfly · 2015-06-16T04:09:31Z

Thanks @davecgh will add those.

tuxcanfly · 2015-07-01T18:34:04Z

Refactored to remove fields owned by blockmanager i.e requestedTxns, requestedBlocks from peer.Peer. Updated blockmanager to maintain a map of requested objects to peers. Now testing to make sure this works as expected.

As for pending tasks, added a first version of README.md. doc.go. Need to add example.go and tests for package peer.

tuxcanfly · 2015-07-03T18:44:33Z

Added example to showcase usage of package peer. Also fixed the bugs around btcctl peer commands.

Working on testing and writing peer tests.

tuxcanfly · 2015-07-07T15:29:02Z

Added both inbound and outbound peer examples. Working on ironing out a couple of minor issues and adding tests.

tuxcanfly · 2015-07-17T13:56:48Z

Added tests for package peer, with coverage at ~47% now. Working on clearing up and improving the coverage.

tuxcanfly · 2015-07-20T19:15:31Z

Debugging an crash in handleInvMsg which occurs due to nil IV.

tuxcanfly · 2015-07-20T19:55:24Z

Pushed a fix for the inv message crash issue.

davecgh · 2015-07-21T01:10:12Z

blockmanager.go

-	for k := range p.requestedTxns {
-		delete(b.requestedTxns, k)
+	for k, txPeer := range b.requestedTxns {
+		if txPeer == p { // peer matches


I believe we have unique IDs for peers already. We should probably use that instead of relying on pointer equality. I realize that as long as the code is super careful they should match, but it wouldn't be very difficult to have two different pointers to the same peer which would cause a check like this fail.

davecgh · 2015-10-21T05:56:41Z

Please squash all commits and update the initial commit message to reflect reality.

davecgh · 2015-10-21T06:42:59Z

As a point of reference, all previous comments up until this point have been taken care of.

tuxcanfly · 2015-10-21T07:39:30Z

Updated commit message and added a TODO regarding ourNA.

jrick · 2015-10-21T14:02:04Z

peer/mrunoncemap_test.go

+
+		// Ensure the limited number of most recent entries in the list
+		// exist.
+		for j := numNonces - 1; j >= numNonces-test.limit; j-- {


Can this and the loop below iterate with increasing j's? For this one it should start at the limit and iterate up to numNonces, and the one below would iterate from 0 up to the limit. imo that would be more readable than iterating them backwards.

OK, updated to iterate in increasing order.

jrick · 2015-10-21T14:13:37Z

// MessageListeners defines callback function pointers to invoke with message 
// listeners.  Since all of the functions are nil by default, all listeners are 
// effectively ignored until their handlers are set to a concrete callback.

I think this should be reworded as it could be interpreted to mean that these callbacks can be modified through this struct at runtime after a peer is created (it won't race because it creates a copy, but it won't work as intended either).

It also MUST explain how these callbacks are executed. Per a single peer, do these callbacks execute concurrently with multiple messages or does one callback block the execution of the next?

jrick · 2015-10-21T14:25:46Z

peer/peer_test.go

+		return
+	}
+
+	// TODO: actually test the following methods


davecgh · 2015-10-21T16:56:37Z

For reference, here is an overview of the major testing points I've done:

Ensure general communications work as before including all supported p2p messages are handled properly (version negotiation, transactions, blocks, inventory, data retrieval, addresses, mempool, bloom filters)
Manually verifying properly generated version messages via tracnig
Maximum number of peers is respected
Eight outbound peers are maintained
Manual testing against a couple of SPV wallets connecting to it
Check output of peer data in various RPC commands (results below)
Adding permanent and temp peers via node and addnode commands
Disconnecting peers via node command

Connection count from getinfo:

$ btcctl getinfo | grep connections
"connections": 15,

Outbound and inbound peers shown properly in getpeerinfo:

$ btcctl getpeerinfo | grep ""inbound": false" | wc -l
8
$ btcctl getpeerinfo | grep ""inbound": true" | wc -l
7

Total bytes read and written from getnettotals:

$ btcctl getnettotals
{
"totalbytesrecv": 402747331,
"totalbytessent": 3840105502,
...
}

davecgh · 2015-10-21T21:18:02Z

Once @jrick's comments are addressed, this is ready for master.

tuxcanfly · 2015-10-22T21:01:10Z

@davecgh Addressed comments by @jrick, rebased and squashed.

davecgh · 2015-10-22T21:28:14Z

Everything has been addressed for me.

OK

jrick · 2015-10-22T21:34:59Z

ok

This commit introduces package peer which contains peer related features refactored from peer.go. The following is an overview of the features the package provides: - Provides a basic concurrent safe bitcoin peer for handling bitcoin communications via the peer-to-peer protocol - Full duplex reading and writing of bitcoin protocol messages - Automatic handling of the initial handshake process including protocol version negotiation - Automatic periodic keep-alive pinging and pong responses - Asynchronous message queueing of outbound messages with optional channel for notification when the message is actually sent - Inventory message batching and send trickling with known inventory detection and avoidance - Ability to wait for shutdown/disconnect - Flexible peer configuration - Caller is responsible for creating outgoing connections and listening for incoming connections so they have flexibility to establish connections as they see fit (proxies, etc.) - User agent name and version - Bitcoin network - Service support signalling (full nodes, bloom filters, etc.) - Maximum supported protocol version - Ability to register callbacks for handling bitcoin protocol messages - Proper handling of bloom filter related commands when the caller does not specify the related flag to signal support - Disconnects the peer when the protocol version is high enough - Does not invoke the related callbacks for older protocol versions - Snapshottable peer statistics such as the total number of bytes read and written, the remote address, user agent, and negotiated protocol version - Helper functions for pushing addresses, getblocks, getheaders, and reject messages - These could all be sent manually via the standard message output function, but the helpers provide additional nice functionality such as duplicate filtering and address randomization - Full documentation with example usage - Test coverage In addition to the addition of the new package, btcd has been refactored to make use of the new package by extending the basic peer it provides to work with the blockmanager and server to act as a full node. The following is a broad overview of the changes to integrate the package: - The server is responsible for all connection management including persistent peers and banning - Callbacks for all messages that are required to implement a full node are registered - Logic necessary to serve data and behave as a full node is now in the callback registered with the peer Finally, the following peer-related things have been improved as a part of this refactor: - Don't log or send reject message due to peer disconnects - Remove trace logs that aren't particularly helpful - Finish an old TODO to switch the queue WaitGroup over to a channel - Improve various comments and fix some code consistency cases - Improve a few logging bits - Implement a most-recently-used nonce tracking for detecting self connections and generate a unique nonce for each peer

tuxcanfly force-pushed the peer-refactor branch from 497207b to 9dd273d Compare June 4, 2015 15:35

davecgh reviewed Jun 15, 2015
View reviewed changes

tuxcanfly force-pushed the peer-refactor branch 2 times, most recently from 6150e4f to b39b230 Compare June 19, 2015 17:48

tuxcanfly force-pushed the peer-refactor branch 4 times, most recently from d722b47 to ca816a7 Compare July 1, 2015 13:15

tuxcanfly force-pushed the peer-refactor branch 2 times, most recently from 32b5760 to 2c230a2 Compare July 3, 2015 18:41

tuxcanfly force-pushed the peer-refactor branch from 1bddd5f to 07a16d4 Compare July 16, 2015 12:07

tuxcanfly force-pushed the peer-refactor branch 2 times, most recently from 9287b25 to 2186828 Compare July 20, 2015 13:56

tuxcanfly force-pushed the peer-refactor branch from bc5c8fd to c734521 Compare July 20, 2015 19:55

davecgh reviewed Jul 21, 2015
View reviewed changes

tuxcanfly force-pushed the peer-refactor branch 3 times, most recently from ffd6da1 to c3cc837 Compare October 21, 2015 06:40

tuxcanfly force-pushed the peer-refactor branch from c3cc837 to 53347ea Compare October 21, 2015 07:34

jrick reviewed Oct 21, 2015
View reviewed changes

peer/peer_test.go

return

}

// TODO: actually test the following methods

Copy link

Member

jrick Oct 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

davecgh mentioned this pull request Oct 21, 2015

peer: Don't log write errs due to disconnect. #524

Merged

tuxcanfly force-pushed the peer-refactor branch 6 times, most recently from 00ece4b to 631aa31 Compare October 22, 2015 20:53

tuxcanfly force-pushed the peer-refactor branch from 631aa31 to 00bddf7 Compare October 23, 2015 00:51

conformal-deploy merged commit 00bddf7 into btcsuite:master Oct 23, 2015

This was referenced Oct 23, 2015

blockchain: Rework to use new db interface. #491

Merged

[peer] Refactor peer code into its own package #439

Closed

tuxcanfly deleted the peer-refactor branch October 30, 2015 02:56

jrick added a commit to jrick/btcd that referenced this pull request Nov 9, 2016

Update dcrrpcclient for dcrctl. (btcsuite#445)

18049cb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[peer] Refactor peer code into its own package #445

[peer] Refactor peer code into its own package #445

tuxcanfly commented Jun 3, 2015

tuxcanfly commented Jun 12, 2015

davecgh commented Jun 15, 2015

davecgh Jun 15, 2015

davecgh commented Jun 15, 2015

tuxcanfly commented Jun 16, 2015

tuxcanfly commented Jul 1, 2015

tuxcanfly commented Jul 3, 2015

tuxcanfly commented Jul 7, 2015

tuxcanfly commented Jul 17, 2015

tuxcanfly commented Jul 20, 2015

tuxcanfly commented Jul 20, 2015

davecgh Jul 21, 2015

davecgh commented Oct 21, 2015

davecgh commented Oct 21, 2015

tuxcanfly commented Oct 21, 2015

jrick Oct 21, 2015

tuxcanfly Oct 22, 2015

jrick commented Oct 21, 2015

jrick Oct 21, 2015

davecgh commented Oct 21, 2015

davecgh commented Oct 21, 2015

tuxcanfly commented Oct 22, 2015

davecgh commented Oct 22, 2015

jrick commented Oct 22, 2015

[peer] Refactor peer code into its own package #445

[peer] Refactor peer code into its own package #445

Conversation

tuxcanfly commented Jun 3, 2015

tuxcanfly commented Jun 12, 2015

davecgh commented Jun 15, 2015

davecgh Jun 15, 2015

Choose a reason for hiding this comment

davecgh commented Jun 15, 2015

tuxcanfly commented Jun 16, 2015

tuxcanfly commented Jul 1, 2015

tuxcanfly commented Jul 3, 2015

tuxcanfly commented Jul 7, 2015

tuxcanfly commented Jul 17, 2015

tuxcanfly commented Jul 20, 2015

tuxcanfly commented Jul 20, 2015

davecgh Jul 21, 2015

Choose a reason for hiding this comment

davecgh commented Oct 21, 2015

davecgh commented Oct 21, 2015

tuxcanfly commented Oct 21, 2015

jrick Oct 21, 2015

Choose a reason for hiding this comment

tuxcanfly Oct 22, 2015

Choose a reason for hiding this comment

jrick commented Oct 21, 2015

jrick Oct 21, 2015

Choose a reason for hiding this comment

davecgh commented Oct 21, 2015

davecgh commented Oct 21, 2015

tuxcanfly commented Oct 22, 2015

davecgh commented Oct 22, 2015

jrick commented Oct 22, 2015