Extensible Messages #48

braydonf · 2015-03-11T21:30:42Z

Messages are now built with a factory that can be customized with an options argument to Messages, for example:
```
var messages = new Messages({Block: Block, magicNumber: 0x0b120907});
var message = messages.GetData.forBlock(inv.hash);
peer.sendMessage(message);
```
Additionally, individual message constructors have been organized into separate files.
Peer and Pool now have options defined as an object to avoid needing to pass in "null, null, true" as arguments, for example:
```
var pool = new Pool({relay: false, dnsSeed: false, network: Networks.testnet});
```
Adds an Inventory constructor to be shared between several of the message types, separate from the Inventory message.
Changes to use BufferWriter instead of needing the "bufferput" dependency.
Adds a listen() method to Pool to enable remote peers to be added to the pool upon incoming connections, enables bitcore to bitcore node connectivity.
Will disconnect a peer on error (to avoid _connectedPeers having invalid peers).
Increases test coverage

- changed options in constructors - define block and transaction constructors for block and tx messages

- Renamed "Commands" to "builder" - "Messages.parseMessage" to "Messages.parseBuffer" - Changed to use private methods for "discardUntilNextMessage" and "buildFromBuffer" - Cleaned up tests

…essages

coveralls · 2015-03-16T15:47:37Z

Coverage increased (+5.9%) to 100.0% when pulling f17bbf5 on braydonf:feature/extensibility into ae93c14 on bitpay:master.

carnesen · 2015-03-16T16:54:41Z

lib/buffers.js

+};
+
+module.exports = Buffers;
+


This seems like a pretty useful function. Should we submit it to the global buffers module https://github.com/substack/node-buffers? I'd be happy to give it a go if that's the right thing to do.

Possibly, we may not need this dependency either though, since we may only be using push(). Thoughts?

Agree, I wanted to take out this dependency as well at some point, don't remember what changed my mind.

Spawned an issue: #52

can you unit-test the skip function please?

added test in: braydonf@7cfe6d1

coveralls · 2015-03-16T18:11:34Z

Coverage increased (+5.9%) to 100.0% when pulling 8f2d008 on braydonf:feature/extensibility into ae93c14 on bitpay:master.

carnesen · 2015-03-16T18:34:58Z

lib/peer.js

- * A Peer instance represents a remote bitcoin node and allows to communicate
- * with it using the standard messages of the bitcoin p2p protocol.
+ * A constructor to create Peer instances to send and recieve messages
+ * using the standard Bitcoin protocol.
 *


How about: "The Peer class implements the Bitcoin peer-to-peer (P2P) network protocol. A Peer instance represents one connection on the Bitcoin network. To create a new peer connection to a remote server, provide the host and port options in the class constructor and then invoke the connect method. Incoming peer connections are handled by the Pool class and its listen method."?

Using "constructor" I think is more descriptive. There is also the option of passing a "socket" in, and "host" and "port" are based on the connected socket.

You're right that constructor is better here as you had it. There's a typo though *receive. In retrospect my suggestion was really as an update to docs/peer.md class description.

Updated in: 664ceb2

eordano · 2015-03-16T18:46:55Z

Amazing work, great!

There are a few nits on the tests names but I'd approve this PR as is. I'll review it more in-deep this afternoon

braydonf · 2015-03-16T19:23:48Z

Two questions:

Should we remove all of the fromObject methods on the message command constructors, since they nolonger seem necesssary ?
Should the magicNumber for messages be stored as a buffer, so it's the same as Network. And writing this is nolonger needed: bitcore.Networks.defaultNetwork.networkMagic.readUInt32LE(0) ?

… for readme icon

eordano · 2015-03-16T21:53:05Z

I wouldn't, it may be useful at some point if we implement .toObject (dunno, maybe somebody interested in sending serialized messages in json?)
Either way it's fine by me. The only verbosity you'd save is the readUINt32LE, right? I'm slightly more concerned about every message doing magicNumber = bitcore.Networks....

braydonf · 2015-03-16T22:03:10Z

Yeah, except that new Message() can be used for those purposes is most (all currently) cases. For JSON it would be fromJSON() that would then call new Message(objectFromJSON).
Only a slight difference, it may be possible to configure Message when required (similar to how the commands are being required with options) so that it magicNumber would be defined at that level instead.

Edit: Took a look at implementing #2 And think the current solution is better.

coveralls · 2015-03-16T23:08:04Z

Coverage increased (+5.9%) to 100.0% when pulling c0e3bdb on braydonf:feature/extensibility into ae93c14 on bitpay:master.

maraoz · 2015-03-17T17:52:02Z

lib/messages/builder.js

+      getdata: 'GetData',
+      headers: 'Headers',
+      notfound: 'NotFound',
+      inv: 'Inventory',


Isn't the message now called InvMessage?

Yes, however it's mapped here with just Inventory. messages.Inventory rather than messages.InvMessage. Should they be the same?

eordano · 2015-03-17T17:57:10Z

LGTM, let's merge this

maraoz · 2015-03-17T17:58:46Z

lib/messages/commands/inv.js

+ */
+InvMessage.forFilteredBlock = function(hash) {
+  return new InvMessage([Inventory.forFilteredBlock(hash)]);
+};


I don't like that these 3 methods (forBlock, forFilteredBlock and forTransaction) are repeated in some classes. Why don't you use a generic method builder like: https://github.com/bitpay/bitcore-p2p/blob/master/lib/messages.js#L974 ?

This could be possible. I left them there so that these constructors could be used standalone (i.e. without the Messages factory), though that isn't a prominent use case.

Updated in: braydonf@34c3846 moved the helpers to builder.

maraoz · 2015-03-17T17:59:36Z

Great work in general, left some small comments. Please fix/reply and I'll be happy to merge this :)

coveralls · 2015-03-17T20:17:17Z

Coverage increased (+5.9%) to 100.0% when pulling 34c3846 on braydonf:feature/extensibility into ae93c14 on bitpay:master.

Extensible Messages

braydonf changed the title ~~WIP: Feature/extensibility~~ WIP: Extensible Messages Mar 11, 2015

Braydon Fuller added 5 commits March 11, 2015 23:57

Extensibility

9a813ba

- changed options in constructors - define block and transaction constructors for block and tx messages

Changed options for Pool

0b20724

Moved commands, message and messages to directory

e8f0725

Improved API:

11bee8b

- Renamed "Commands" to "builder" - "Messages.parseMessage" to "Messages.parseBuffer" - Changed to use private methods for "discardUntilNextMessage" and "buildFromBuffer" - Cleaned up tests

Removed bufferput dependency

32a28b1

braydonf force-pushed the feature/extensibility branch from b066b7e to 32a28b1 Compare March 12, 2015 03:57

Braydon Fuller and others added 23 commits March 12, 2015 00:05

removed "buildFromObject" alias

42288e8

added missing precondition and moved buffer.skip to new file

7e29af4

added test for inventory fromBufferReader

d63c117

added test for pool.sendMessage

1dfe709

added test coverage for peer

608e41d

Moved block and version messages and shared utils into seperate files.

6ca2f69

moved ping and verack to seperate files

3c2afed

pass options directly into command messages

feda6e9

moved the rest of the commands into seperate files

e460bf9

organized commands to build from an array

2eb7712

whitespace cleanup

091893b

updated bitcoind integration test

faf2bb1

listen for incoming connections

7f61fc6

make listening optional and fixed peer test

6461748

expose message construtors by name

641443f

updated bitconid integration test to use exposed message constructors

4ecb34e

added default options for all command messages, and added tests

a8b8c59

added tests for command edge cases

8c9babc

added tests for pool.listen and improved arguments for tx and block m…

3b53593

…essages

added tests for pool _addConnectedPeer

97f39db

updated to use mapped constructers and removed build method

6007dc6

cleanup builder options

e31f28e

update documentation for messages

cfbee2d

added jsdocs for message commands

42985a7

updated docs for message commands

f1aa4d3

braydonf changed the title ~~WIP: Extensible Messages~~ Extensible Messages Mar 16, 2015

carnesen reviewed Mar 16, 2015
View reviewed changes

braydonf mentioned this pull request Mar 16, 2015

Peer arguments overloading #46

Closed

Braydon Fuller added 2 commits March 16, 2015 12:59

fix jsdocs for message commands

39d1ae9

updated readme and fixed image scaling issue

8f2d008

carnesen reviewed Mar 16, 2015
View reviewed changes

Braydon Fuller added 3 commits March 16, 2015 15:44

improved peer jsdoc, fixed docs for messages, and switched to use svg…

664ceb2

… for readme icon

fixed spelling

16aa989

upgrade bloom-filter to 0.2.0

f6e9c43

removed fromObject method that is nolonger needed

c0e3bdb

maraoz reviewed Mar 17, 2015
View reviewed changes

Braydon Fuller added 2 commits March 17, 2015 15:11

added unit tests for buffers.skip

7cfe6d1

moved inventory helper functions to builder

34c3846

maraoz added a commit that referenced this pull request Mar 17, 2015

Merge pull request #48 from braydonf/feature/extensibility

487b5f0

Extensible Messages

maraoz merged commit 487b5f0 into bitpay:master Mar 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible Messages #48

Extensible Messages #48

braydonf commented Mar 11, 2015

coveralls commented Mar 16, 2015

carnesen Mar 16, 2015

braydonf Mar 16, 2015

eordano Mar 16, 2015

braydonf Mar 16, 2015

maraoz Mar 17, 2015

braydonf Mar 17, 2015

coveralls commented Mar 16, 2015

carnesen Mar 16, 2015

braydonf Mar 16, 2015

carnesen Mar 16, 2015

braydonf Mar 16, 2015

eordano commented Mar 16, 2015

braydonf commented Mar 16, 2015

eordano commented Mar 16, 2015

braydonf commented Mar 16, 2015

coveralls commented Mar 16, 2015

maraoz Mar 17, 2015

braydonf Mar 17, 2015

eordano commented Mar 17, 2015

maraoz Mar 17, 2015

braydonf Mar 17, 2015

braydonf Mar 17, 2015

maraoz commented Mar 17, 2015

coveralls commented Mar 17, 2015

		};

		module.exports = Buffers;

Extensible Messages #48

Extensible Messages #48

Conversation

braydonf commented Mar 11, 2015

coveralls commented Mar 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eordano commented Mar 16, 2015

braydonf commented Mar 16, 2015

eordano commented Mar 16, 2015

braydonf commented Mar 16, 2015

coveralls commented Mar 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eordano commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraoz commented Mar 17, 2015

coveralls commented Mar 17, 2015