Add guide on how to connect local nodes #107

OrfeasLitos · 2018-08-22T23:52:53Z

Demo of two regtest nodes connecting. Resolves #106.

pinheadmz

Thank you so much for this guide! I think it's a great demonstration, it's super simple and I learned a lot in just these few lines of code. I was thinking (see my specific comments) that maybe instead of asserting specific outputs from the peer list -- could we actually show how the SPV / Fullnode relationship works? Like, add an address to the SPV bloom filter and then demonstrate how the Fullnode alerts the SPV. Perhaps the easiest way would be to hard-code an address for the SPV node to watch:

spvNode.pool.watchAddress('RCFhpyWXkz5GxskL96q4KtceRXuAMnWUQo')

...and then generate a block to that address from Fullnode:

fullNode.rpc.generateToAddress([1, 'RCFhpyWXkz5GxskL96q4KtceRXuAMnWUQo')

You could also generate a block to a different address just to indicate that Fullnode would not relay that to SPV node.

Again, great work and thanks! Let me know what you think of these changes. I'm excited to add this to bcoin.io!

pinheadmz · 2018-08-23T16:23:14Z

guides-markdown/connect-local-nodes.md

+
+  // no peers for the spvNode yet :(
+  console.log('spvNode\'s peers before connection:', spvNode.pool.peers.head());
+  assert.equal(spvNode.pool.peers.head(), null);


When I ran this script, I actually already had a peer:
spvNode's peers before connection: <Peer: handshake=false host=18.191.95.172:48445 outbound=true ping=-1>

...and so the assert failed. (That's the IP address of my own server). The "before connection" printout is interesting, but I don't know if we need the assert there... unless there is some other way to definitively clear all peers before starting.

pinheadmz · 2018-08-23T16:30:42Z

guides-markdown/connect-local-nodes.md

+  spvNode.pool.peers.add(peer);
+
+  // allow some time to establish connection
+  await delay(3000);


I actually had to change this delay to 10 seconds delay(10000) on my machine or the second assert would fail. I wonder if we could use an await/async method here instead of gambling on how long the connection will take? I haven't tested this yet but maybe peer.open()? https://github.com/bcoin-org/bcoin/blob/master/lib/net/peer.js#L347-L361

pinheadmz · 2018-08-23T16:34:51Z

guides-markdown/connect-local-nodes.md

+  // nodes are now connected!
+  console.log('spvNode\'s peers after connection:', spvNode.pool.peers.head());
+  assert.equal(spvNode.pool.peers.head().inspect(),
+    '<Peer: handshake=true host=127.0.0.1:48445 outbound=true ping=-1>');


Once I adjusted the timeout above and commented out the first assert, this assert succeeded. But still, again, I think it is a long string to assert equality. See my main review comment for my idea on how to wrap this up nicely.

OrfeasLitos · 2018-08-25T00:46:11Z

Thanks for the constructive suggestions. These would make the example much more robust and informative. Avoiding timeouts and asserts is the ideal case. I managed to avoid the second timeout with a less-than-ideal trick, but I'm not sure how to avoid the first timeout. Specifically, the spvNode tries to connect to a default port (48444) and if I try to explicitly disconnect it from this port there is an error. I couldn't find some way to ensure that the peer list is empty (apart from the obviously ugly busy-wait). Do you have any suggestions?

OrfeasLitos · 2018-08-25T01:47:00Z

Another question. It seems that the example you gave with the generateToAddress() does not work. On the wire I can just see a get_block inv packet, not a get_tx. The other solution would be to actually create wallets just for the demo, but first I find this an overkill and second there is currently a bug in bcoin where if an spv node creates a walletdb, it overwrites its spv filter with the tx filter and the spv node can't connect anymore (bcoin-org/bcoin#578). So the guide would not work until the next release. Any alternative ideas for the demo? Or a workaround to the get_block inv?

pinheadmz · 2018-08-27T22:22:17Z

Hi @OrfeasLitos ! Sorry took me a few days to play with this and get back.

Here's where I ended up with the script: https://gist.github.com/pinheadmz/66f1ff9ae44ef594f0857987b075da77

You can take or leave whatever you want from there, see what you think. But here's kinda the gist of that... gist:

Took out all asserts, just replaced with console messages. We're not testing code here, just demonstrating a few methods.
Removed the close()'s at the end -- in my testing these would close the nodes sometimes before the block and tx messages went through.
Added event listeners for block and generate 2 blocks: one that pays out to the SPV node's watch address and one that doesn't. In the console when those blocks come through, you can see that one has a Map{} object, which informs the SPV node that its filter matches a transaction in the block. In this case, it's the coinbase transaction. The user can see that one block matched the bloom filter and the other did not.
Added event listeners for tx and send 2 transactions: one that pays the SPV watch address and one that does not. Interestingly, this event never fires. I'm discussing with the devs about this right now, might actually be an overall issue with SPV nodes emitting the tx event. Too bad! It would be nice to demonstrate how transactions get filtered as well.
I left the delays in there, I tried a few things but couldn't figure out a cleaner way to "wait" for a connection to be established before moving on in the process.

I look forward to your update! Thanks again and let me know if you have any other questions or run into any other weirdness :-)

OrfeasLitos · 2018-08-29T01:41:02Z

I like your approach, with the logging and maxOutbound: 1. I removed the assert()s (and stopped require()ing the library) and fixed a couple of typos. I realized that createOutbound() actually does all the connection plumbing, except for actually adding the peer to the PeerList (what spvNode.pool.peers.add(peer) does, c.f. https://github.com/bcoin-org/bcoin/blob/1bde15b9a24c2d1c71db33864b326b58ae5d2e60/lib/net/pool.js#L2984-L2986, https://github.com/bcoin-org/bcoin/blob/1bde15b9a24c2d1c71db33864b326b58ae5d2e60/lib/net/pool.js#L1054-L1066). Thus createOutbound() should go after delay(800) and together with peers.add(). In practice, this doesn't change the output, but let's change it for the sake of clarity.

I used the tool p-event to await for the emission of a 'peer connect' event from spvNode.pool and the 'block' event from spvNode. This way we avoid the long, arbitrary delay of 10s (at the cost of continuing before the handshake is complete) and we can also cleanly close the nodes and allow the script to exit.

In slightly older commits, the SPV node used to correctly receive only one transaction! The regression happens here: bcoin-org/bcoin@a0ac953 . If it is fixed, we can simply send the two txs and avoid the blocks. This solution seems better because the difference between the two blocks as printed is too small.

pinheadmz

Dude thanks again so much for this contribution! I think it's almost ready to go, checking on the bug fix right now... I am super-duper amazed you caught that!

OK - @tuxcanfly found the bug and is working on a fix. SO I'd say, in addition to the small comments below, let's go with your plan: remove all the block stuff and make it about the transactions instead.

Maybe just add a comment either in the code or in the text description about why one TX goes to the SPV and the other doesn't (because of the bloom filter)

pinheadmz · 2018-08-30T17:48:43Z

guides-markdown/connect-local-nodes.md

+  logConsole: false,
+  logLevel: 'spam',
+
+  // reduce log spam on SPV node (won't warn on Full node)


sorry this is my typo, it should say won't work on full node -- the full node will still try to connect to that 1 outbound peer and the log will fill up anyway. maxOutbound: 0 returns an error, I tried it :-)

Ah, now I see what you meant. Maybe we can achieve the intended behaviour by making the full node selfish. I'll try it and add it if it reduces logging but still broadcasts stuff (I suppose it should).

I will add a comment explaining why selfish and why users shouldn't do it in general :)

I thought selfish prevented all blocks and txs from going out...? My intent with the maxOutbound was just to reduce the attempt to connect to peer stuff in the log... it's not that crucial either way :-)

Apparently selfish mode doesn't work. First of all, selfish nodes do not connect to spv nodes. Second, even if I try to undo some of the damage done by the selfish flag, apparently the handshake fails with the (selfish) full node never sending back the verack packet... Probably what I tried to do is unsupported, but still it's weird behavior. Anyway, let's forget the selfish trick.

pinheadmz · 2018-08-30T17:52:15Z

guides-markdown/connect-local-nodes.md

+gracefully closed.
+
+The script should work out of the box, the only requirement is having `bcoin`
+installed.


I had to install p-event on my system as well :-)
Can you add a little instruction for that? Maybe a one-line description of the package as well?

Oops, I forgot to change the prose as well.

OrfeasLitos · 2018-08-31T10:57:53Z

I will try to see if the example works with txs on the latest npm release. If it works (which is what I expect) I will remove the block creation and just send 2 txs. I assume that guides should work on the latest version, not on the latest commit, so it's fine if it doesn't work until bcoin-org/bcoin#598 is merged.

bucko13

Nice! Just a couple small suggestions but this looks great!

bucko13 · 2018-08-31T22:42:25Z

guides-markdown/connect-local-nodes.md

+Start and manually connect two regtest nodes in a single script.
+```
+
+When we start two nodes in the regtest (loopback network, 127.0.0.1) with


"on the regtest network" instead of "in the regtest"

bucko13 · 2018-08-31T22:43:43Z

guides-markdown/connect-local-nodes.md

+When we start two nodes in the regtest (loopback network, 127.0.0.1) with
+manually set ports, they don't get connected automatically because they don't
+know each other's port. We thus have to connect them manually. In this example
+we will fire up one SPV and one Full node and initiate a connection from the


I think either "Full Node" or "full node". SPV is capitalized because it's an acronym, but "Full" isn't a proper noun on its own

bucko13 · 2018-08-31T22:43:51Z

guides-markdown/connect-local-nodes.md

+manually set ports, they don't get connected automatically because they don't
+know each other's port. We thus have to connect them manually. In this example
+we will fire up one SPV and one Full node and initiate a connection from the
+SPV side. The SPV node is given the address of the Full node and attempts to


bucko13 · 2018-08-31T22:46:27Z

guides-markdown/connect-local-nodes.md

+know each other's port. We thus have to connect them manually. In this example
+we will fire up one SPV and one Full node and initiate a connection from the
+SPV side. The SPV node is given the address of the Full node and attempts to
+connect. If everything goes well, the two nodes connect. The Full node then


Same here. Also, let's split the paragraph here when you start talking about transactions (easier to read smaller paragraphs and this is kind of another section). So "Next, we'll have the full node broadcast two transactions."

👍 I also rephrased the first sentence of the newly split paragraph to "To demonstrate that the connection was successful, we'll have full node..." to make clear why on earth we deal with txs.

bucko13 · 2018-08-31T22:47:33Z

guides-markdown/connect-local-nodes.md

+by the SPV node and the second isn't. Finally the two nodes are gracefully
+closed.
+
+We use the library `p-event` which promisifies events in order to `await` for


bucko13 · 2018-08-31T22:49:59Z

guides-markdown/connect-local-nodes.md

+  httpPort: 48449, // avoid clash of ports
+
+  // write log file and chain data to specific directory
+  prefix: '~/connect-test/SPV',


small nit, but it might be nice to make ~/connect-test a constant since it's used in multiple places. This is nice in case someone forgets to change both and ends up with two different project directories. Also if you use node's os.homedir() method instead of ~ it can work across platforms since I don't think Windows recognizes ~.

Doesn't windows use backslashes instead of slashes as well? I don't have a windows box to test upon, but I think something in the lines of https://gist.github.com/domenic/2790533 should be used. More info: https://nodejs.org/docs/latest/api/path.html. I think I got it correctly; At least my box is happy. If anyone can test it on a windows box, please tell me if it works for them.

Well, the beauty if using Node's os module is that it will take care of environment specific conditions for you so you don't even have to worry about it.

i.e.

const prefix = `${require('os').homedir()}/connect-test/SPV`

if you look at the bcfg library which manages this in bcoin, it makes heavy use of this convention, even explicitly replacing instances of ~ passed by the user.

Thanks for pointing me there, I found path.join() which accomplishes what I wanted more simply than path.format(). Let me know if you'd like any more changes.

bucko13

Looks great! Thanks for the contribution.

Add guide on how to connect local nodes

5212c46

OrfeasLitos mentioned this pull request Aug 22, 2018

Write example of two regtest nodes connecting bcoin-org/bcoin#582

Closed

pinheadmz self-assigned this Aug 23, 2018

pinheadmz requested changes Aug 23, 2018

View reviewed changes

OrfeasLitos added 8 commits August 29, 2018 02:30

Remove assert()s

42c6fbe

Add logging

e9670d8

Allow only one outbound peer

2d749e8

Add p-event

cc9fa60

Remove "success!" console.log()

abdd243

Connect to peer after initial delay

39281ee

Watch address, start sync, set up listeners

db4ece8

Generate and wait for blocks and txs

3b9f6bb

pinheadmz requested changes Aug 30, 2018

View reviewed changes

pinheadmz mentioned this pull request Aug 30, 2018

Fixes pool.getBroadcasted() error item.hash is not a function bcoin-org/bcoin#598

Merged

OrfeasLitos added 4 commits August 31, 2018 21:34

Update guide description and requirements

def25da

Rephrase comment

4a70d95

Remove blocks stuff

e76f604

Add back tx stuff

1d3e507

pinheadmz approved these changes Aug 31, 2018

View reviewed changes

bucko13 suggested changes Aug 31, 2018

View reviewed changes

OrfeasLitos added 3 commits September 4, 2018 00:09

Change prose for clarity

240f1d3

Add directory portability

871b015

Use path.join() instead of path.format()

efb1d5a

bucko13 approved these changes Sep 6, 2018

View reviewed changes

bucko13 merged commit 5066b6a into bcoin-org:staging Sep 6, 2018

OrfeasLitos deleted the connect-nodes branch September 6, 2018 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add guide on how to connect local nodes #107

Add guide on how to connect local nodes #107

OrfeasLitos commented Aug 22, 2018

pinheadmz left a comment •

edited

Loading

pinheadmz Aug 23, 2018

pinheadmz Aug 23, 2018

pinheadmz Aug 23, 2018 •

edited

Loading

OrfeasLitos commented Aug 25, 2018

OrfeasLitos commented Aug 25, 2018

pinheadmz commented Aug 27, 2018

OrfeasLitos commented Aug 29, 2018

pinheadmz left a comment •

edited

Loading

pinheadmz Aug 30, 2018

OrfeasLitos Aug 31, 2018

OrfeasLitos Aug 31, 2018

pinheadmz Aug 31, 2018

OrfeasLitos Aug 31, 2018

pinheadmz Aug 30, 2018

OrfeasLitos Aug 31, 2018

OrfeasLitos commented Aug 31, 2018

bucko13 left a comment

bucko13 Aug 31, 2018

bucko13 Aug 31, 2018

bucko13 Aug 31, 2018

bucko13 Aug 31, 2018

OrfeasLitos Sep 3, 2018 •

edited

Loading

bucko13 Aug 31, 2018

bucko13 Aug 31, 2018

OrfeasLitos Sep 3, 2018

bucko13 Sep 6, 2018

bucko13 Sep 6, 2018 •

edited

Loading

OrfeasLitos Sep 6, 2018

bucko13 left a comment

Add guide on how to connect local nodes #107

Add guide on how to connect local nodes #107

Conversation

OrfeasLitos commented Aug 22, 2018

pinheadmz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

OrfeasLitos commented Aug 25, 2018

OrfeasLitos commented Aug 25, 2018

pinheadmz commented Aug 27, 2018

OrfeasLitos commented Aug 29, 2018

pinheadmz left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrfeasLitos commented Aug 31, 2018

bucko13 left a 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

OrfeasLitos Sep 3, 2018 • edited Loading

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

bucko13 Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bucko13 left a comment

Choose a reason for hiding this comment

pinheadmz left a comment •

edited

Loading

pinheadmz Aug 23, 2018 •

edited

Loading

pinheadmz left a comment •

edited

Loading

OrfeasLitos Sep 3, 2018 •

edited

Loading

bucko13 Sep 6, 2018 •

edited

Loading