The 'fRelay' param on Version requests is optional.

[throughnothing]

I'v seen RangeError: index out of range crashes from nodes that don't send it, when bitcore-p2p tries to readUInt8() the last bit. You can see the same error by keeping my test, and removing the code change.

This field is optional as per the spec, and when it is not present, should essentially default to 1/true: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki

[coveralls]

Coverage increased (+0.03%) to 91.95% when pulling cc369b31777c164b25d484236ec334db5978c3f7 on throughnothing:frelay-optional into e19734593e1df6c4edc6d73603666eaaf9a9ba98 on bitpay:master.

[coveralls]

Coverage increased (+0.03%) to 91.95% when pulling 4ad4a38aadddc0fd676b79f763d5955097e0c7a9 on throughnothing:frelay-optional into e19734593e1df6c4edc6d73603666eaaf9a9ba98 on bitpay:master.

[braydonf]

Sending a fRelay as true should also be possible in getPayload and needed for filtering.

[throughnothing]

Yup, I was thinking that, too. Will add here.

[throughnothing]

I realized consumers would want to be able to set relay on the peer, since consumers don't control the initial Version message that gets sent. This means Peer had to have a relay also, that _sendVersion() respects.

[braydonf]

LGTM

[coveralls]

Coverage increased (+0.07%) to 91.99% when pulling 962137619b7a11d26b85f576bfcae58dcc04693d on throughnothing:frelay-optional into e19734593e1df6c4edc6d73603666eaaf9a9ba98 on bitpay:master.

[throughnothing]

Just changed the commit message to have it rebuild, didn't change any code.

[throughnothing]

Hmmm, I think technically relay should be settable up to the pool level, since if you're using a pool, you don't really instantiate peer objects yourself. Thus, relay would need to be passed through by the pool to it's peers.

Seems like both pool and peers could be a little bit nicer with a Peer(options) { ... } style interface, so you could do

var peer = new Peer({ host: 'localhost', relay: false });

instead of

var peer = new Peer('localhost', null, null, false);

I know that's a bigger change, but just thinking out loud.

[braydonf]

I agree, passing an object with options is better, except in this case:

var peer = new Peer({host: '127.0.0.1'});

instead of

var peer = new Peer('127.0.0.1');

So I think as soon as more than one to three options are introduced it should be possible to use an object instead.

[throughnothing]

Currently, Pool(network) only accepts network. Seems odd to pass Pool(network,relay), but I can do that...

Edit: For Pool, since it only takes 1 arg currently, it could be good to make the switch now for an object, that way its simpler/cleaner to detect Network/object for 1 arg and act/interpret accordingly. Peer is trickier.

[braydonf]

Pending pull request: https://github.com/bitpay/bitcore-p2p/pull/37/files adds options to Pool as the second parameter, which could be used.

[throughnothing]

Ah, cool, that could work.

[maraoz]

LGTM, waiting for https://github.com/bitpay/bitcore-p2p/pull/37 to be fixed and merged so you can use options object instead of a long list of arguments. btw, thanks a lot for this awesome contribution! :)

[maraoz]

https://github.com/bitpay/bitcore-p2p/pull/37 was merged. @throughnothing can you update this?

[throughnothing]

@maraoz definitely. What do you think I should do about the Pool() interface, as I think relay should be added at that level as well. I don't like Pool(network,relay) but we can start there and move to options on Pool() later. What do you think?

[throughnothing]

Oh, I see #37 added options to Pool, not Peer. I can work with that for now.

[throughnothing]

@maraoz Pushed an update. I ended up doing a few other things that maybe shouldn't be in this PR to make testing easier, like adding options.size ability to the Pool to easier control how many peers it'll connect to. This is probably something that's needed, but could be broken into a separate PR if you think it's necessary. I also refactored the options stuff a little bit to make it cleaner in the Pool() constructor, but let me know if that's not satisfactory.

I kept those new changes in a separate commit for isolated review.

[throughnothing]

Huh, it looks like my new Pool test can flap. Will investigate.

[throughnothing]

I think using dns in my test was causing it to timeout, so I added dnsSeed: false to the options I pass in, and that seems to have resolved it. I think the reason is that in the Pool.connect() call, it'll try to add addresses from DNS seeds, which takes extra time. It's not needed since I add an address in my test.

[coveralls]

Coverage increased (+0.04%) to 92.31% when pulling 63ad4e543828ddbed5872c5ddbca684f709397cf on throughnothing:frelay-optional into 872dc52c63404f5c64138047dd2d8c265585a3d9 on bitpay:master.

[braydonf]

Adding size as an option may not be needed (since Pool.MaxConnectedPeers can be modified), other than that changes look good.

[throughnothing]

@braydonf overwriting Pool.MaxConnectedPeers is pretty non-ideal, IMO. It's currently what I'm doing in my project, but what if I wanted to run 2 pools? It should be specific to a pool, not all pools globally.

Happy to revert that part and leave that discussion for another PR if that's the best way forward with this one.

[braydonf]

That's a decent point with multiple pools.

[braydonf]

Needs to be added to the docs as well.

[throughnothing]

@braydonf where is the best place to do that. docs/pool.md, or in the code-docs in lib/pool.js or both? Is there somewhere else? I don't see Pool.MaxConnectedPeers in docs/ any where currently.

[braydonf]

jsdocs should be good, added here: https://github.com/throughnothing/bitcore-p2p/pull/2

[throughnothing]

Thanks @braydonf !

[coveralls]

Coverage decreased (-0.13%) to 92.14% when pulling fe08f5a4de016ea9847ac6f3aa1b35d9921310ce on throughnothing:frelay-optional into 872dc52c63404f5c64138047dd2d8c265585a3d9 on bitpay:master.

[coveralls]

Coverage decreased (-0.13%) to 92.14% when pulling d751e583b49e98894414deabcd64c39e56f997c2 on throughnothing:frelay-optional into 872dc52c63404f5c64138047dd2d8c265585a3d9 on bitpay:master.

[braydonf]

It appears that this line is no longer traveled: https://coveralls.io/builds/1953190/source?filename=lib%2Fpool.js#L154 and decreasing coverage. I'll submit a PR with more test coverage, since everything appears to be covered that this has changed.

[throughnothing]

Huh, that's strange.

[braydonf]

In any regard, more test coverage: https://github.com/throughnothing/bitcore-p2p/pull/3

[throughnothing]

Thanks again! It doesn't look like we have test coverage for verifying that maxSize is respected (i.e that it won't connect to 2 nodes if maxSize is 1). Should we add that?

[coveralls]

Coverage increased (+1.04%) to 93.31% when pulling 06f285fc9fa279c337a03f0e8c88e16d2ebabbdb on throughnothing:frelay-optional into 872dc52c63404f5c64138047dd2d8c265585a3d9 on bitpay:master.

[braydonf]

We can add that in another PR, such as when adding: https://github.com/bitpay/bitcore-p2p/issues/41

[throughnothing]

Sounds good to me, thanks for all your help on this one.

[braydonf]

Tested locally against a Bitcoin Core v0.10 node with/without relay set and it is working. Inventory messages are announced for "block" but not "tx" when relay is set to false, and "tx" are relayed when it's true/default.

ACK

[throughnothing]

@braydonf awesome, thanks for that. I've also tested it against nodes on the bitcoin network.

[coveralls]

Coverage increased (+1.04%) to 93.31% when pulling e1f2fbb7ed6a76dfd933e56ea897edce05a48106 on throughnothing:frelay-optional into 872dc52c63404f5c64138047dd2d8c265585a3d9 on bitpay:master.

[maraoz]

LGTM