Peer/Host restructuring #315

sangaman · 2018-08-09T21:17:31Z

Fixes #289. Fixes #271. Fixes #269. Closes #155. Implements parts of #185 and #162. Edit: Closes #177. Closes #305.

This is a structural change to the p2p layer with breaking database changes.

The term to represent a known xud instance that we are not necessarily connected to is changed from "Host" to "Node" and all references have been renamed.
The unique identifier for a node used throughout the codebase is their nodePubKey rather than their socket address.
External addresses for incoming connections are configurable and are shared during handshake.
On receiving NODES packets, connections are only attempted to nodes that we were not previously aware of.

I'm opening this PR a bit early for purposes of getting input. Todo before merging:

~~Hardcode the nodePubKeys of the three xud seed nodes.~~ Edit: Done.
~~Update README.~~ Edit: Done.
Testing between two running xud instances. Edit: Basic testing done.

2018-8-10 09:14:22 [P2P] debug: Connected to peer 127.0.0.1:59443
2018-8-10 09:14:22 [P2P] debug: Received data (127.0.0.1:59443): {"header":{"id":"4c0529a0-9c9f-11e8-8b78-dfb49cdc6ab6","hash":"9S+96qeN0grAs834/Y6ESg==","type":"HELLO"},"body":{"pairs":[],"version":"1.0","nodePubKey":"029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345","addresses":[]}}

2018-8-10 09:14:22 [P2P] debug: Received data (029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345): {"header":{"id":"4c07c1b0-9c9f-11e8-8b78-dfb49cdc6ab6","type":"GET_ORDERS"}}
{"header":{"id":"4c07c1b1-9c9f-11e8-8b78-dfb49cdc6ab6","type":"GET_NODES"}}

Executing (default): INSERT INTO `nodes` (`id`,`nodePubKey`,`addressesText`,`createdAt`,`updatedAt`) VALUES (DEFAULT,'029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345','[]','2018-08-10 13:14:22','2018-08-10 13:14:22');
2018-8-10 09:14:22 [P2P] debug: Received data (029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345): {"header":{"reqId":"4c07c1b0-9c9f-11e8-a3cf-41f24daf02b0","id":"4c0cf1d0-9c9f-11e8-8b78-dfb49cdc6ab6","hash":"J/XyycqWKe0kLqZ0sVdlDw==","type":"NODES"},"body":[{"nodePubKey":"1","addresses":[{"host":"xud1.test.exchangeunion.com","port":8885}]},{"nodePubKey":"2","addresses":[{"host":"xud2.test.exchangeunion.com","port":8885}]},{"nodePubKey":"3","addresses":[{"host":"xud3.test.exchangeunion.com","port":8885}]}]}

Executing (default): SELECT `id`, `baseCurrency`, `quoteCurrency`, `swapProtocol` FROM `pairs` AS `Pair`;
2018-8-10 09:14:22 [P2P] debug: Received data (029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345): {"header":{"reqId":"4c079aa0-9c9f-11e8-a3cf-41f24daf02b0","id":"4c130c50-9c9f-11e8-8b78-dfb49cdc6ab6","hash":"11FxOYiYfpMxmANj4kGJzg==","type":"ORDERS"},"body":[]}

2018-8-10 09:14:52 [P2P] debug: Received data (029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345): {"header":{"id":"5de8c870-9c9f-11e8-8b78-dfb49cdc6ab6","type":"PING"}}

2018-8-10 09:14:52 [P2P] debug: Received data (029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345): {"header":{"reqId":"5de8a160-9c9f-11e8-a3cf-41f24daf02b0","id":"5de91690-9c9f-11e8-8b78-dfb49cdc6ab6","type":"PONG"}}

sangaman · 2018-08-09T21:19:01Z

Travis is getting DB errors that I wasn't getting locally, so I'll have to look into those. It compiles, runs, and passes tests for me locally in its current state although I did need to drop all my xud databases.

Edit: Looks like the issue is Travis is using mysql 5.5, which doesn't support the JSON data type I'm using for storing node addresses. I can fix the Travis build, I just want to make sure it's not unreasonable to require 5.7 mysql. It was released almost 3 years ago so I think it's fine.

Second Edit: After a bit more research, Debian stretch by default uses MariaDB 10.1 for it's MySQL variant, which doesn't support the JSON data type. Rather than have to worry about installing special versions of software on popular variants, I'll figure out another approach. For testing purposes this should still work fine on MySQL 5.7+ and MariaDB 10.2.7+.

sangaman · 2018-08-10T14:04:34Z

I fixed the DB dependency issue that was causing travis to fail by changing the column to a TEXT type named addressesText which stores the JSON in string format. The code then has a VIRTUAL column named addresses that doesn't exist on the table itself, but has a getter and setter that parses and stringifies the addressesText value.

~~Only thing missing is the nodePubKey values for the seed nodes~~, and more testing would be nice as I've only tested between two local nodes on my machine so far.

michael1011 · 2018-08-13T08:08:48Z

instructions-for-devs.md

@@ -142,12 +144,15 @@ host = "localhost"

 [p2p]
 listen = true
-port = 8885
-#make sure this port is reachable from the internet
+port = 8885 # Make sure this port is reachable from the internet


# Make sure this port is reachable from the internet Maybe we can phrase this differently and refer to firewalls and does that port have to be reachable when you just want to have outbound connections?

Another comment about this subject:
The listen port may not be the port which is open on the firewall. for example, xud may listen on 8885 but the firewall may open 8888 and have port forwarding role from 8888 to 8885 on the xud machine. This is a very common situation (just consider 2 servers behind the FW running xud).

As such we must have a listen port and an external port which we publish to the peers.

@offerm I think this is already handled in this PR by p2p.addresses. There you can specify the port that is sent in the handshake for each address.

A string array of reachable socket addresses for this node, port defaults to p2p.port if unspecified
addresses = [ "4.8.15.16:8885" ]

Well done @sangaman , that's exactly how it should be! This will allow even people in heavily censored countries to propagate p2p.addresses with port e.g. 443

@kilrau The port has to be between 1024 and 65535. But there are still plenty options to choose from.

And when you are in a heavily censored country changing the port won't help you much. Maybe we can provide a tutorial for setting up XUD with Tor in the future.

michael1011 · 2018-08-13T08:09:15Z

instructions-for-devs.md

-port = 8885
-#make sure this port is reachable from the internet
+port = 8885 # Make sure this port is reachable from the internet
+# A string array of reachable socket addresses for this node, port defaults to 8885 if unspecified


Shouldn't the default port be the config value p2p.port?

That would be good, yes.

moshababo

Great job, looks good in the overall.

Making the pubkey as the main identifier definitely make things simpler. We just need to make sure it doesn't put unnecessary constrains.

moshababo · 2018-08-13T07:15:17Z

lib/p2p/Pool.ts

-  public addOutbound = async (socketAddress: SocketAddress): Promise<Peer> => {
-    if (this.peers.has(socketAddress)) {
-      const err = errors.ADDRESS_ALREADY_CONNECTED(socketAddress.address);
+  public addOutbound = async (address: Address, nodePubKey: string): Promise<Peer> => {


why do I must know the nodePubKey in order to connect to a node? it obviously helps to have it as the pk, but do we want to put the constrain of identifying a node by ip+port+pubkey, and not just ip+port?

I don't think it's completely necessary, and actually in my first approach I didn't add the pk to the connect gRPC method, but it seemed worth it to me. The advantage is we know who we're connecting to, meaning that we can tell before even attempting the connection if we are already connected, and that we know which pk to expect from the handshake. In cases where we already know the pk, either from saved nodes or nodes we hear about from other peers, there's no downside. For the gRPC method it makes the command a bit more cumbersome particularly from the cli, but I still thought it made sense for purposes of knowing right away if we are already connected. This is also the approach lnd uses, connecting requires specifying the node pub key up front.

I see, so let’s just wait for @kilrau to approve this (in case he didn’t already)

moshababo · 2018-08-13T07:32:46Z

lib/db/models/Node.ts

+        return JSON.parse(this.addressesText);
+      },
+      set(this: db.NodeInstance, value: Address[]) {
+        this.setDataValue('addressesText', JSON.stringify(value));


JSON column type would have been good, but if it's not supported, why not to normalize it into a separate table?
The downside of the current approach is mainly that it's not possible to query a node via address, because you must deserialize everything first.

I hadn't thought of that but it does make querying for node via address awkward (I think it could still be possible with SELECT * WHERE addresses LIKE '%host:'1.2.3.4',port:8885%, but not the nicest query).

The advantage as I see it is that addresses can be updated with a single, simple UPDATE query on handshake and it makes loading nodes from the db a little simpler because it's just querying rows from a single table. But if querying node via address is something we'll do regularly, it should probably be changed. I can't think of a real need to query node by address off the top of my head, can you?

As an aside, updating the addresses on handshake is actually not something that's implemented in this PR. So I will either take care of that here or in a separate PR after this is merged.

Main scenario for querying by address would be to block an inbound peer, prior to handshake, based on the node entity (see my other comment). We might end up having other usages as well.

Squashing a many-to-one relationship in a row is a known problem in RDBMS, forcing you to normalize. This is why MongoDB became popular, followed by the JSON columns (PostgreSQL and then MySQL).
Concatenating the array is usually a fine for a dumb column, one that you wouldn't do anything with except projecting. Yes, querying would be possible, but pretty awful.

At the moment, even if we'll want to query by address, we'll do it in-memory (after deserializing), so no DB support is needed. So I guess you can leave this situation as-is for now, and change it if we'll need to.

moshababo · 2018-08-13T09:44:20Z

lib/p2p/Pool.ts

-      peer.setHost(host);
+  private handleOpen = async (peer: Peer): Promise<void> => {
+    if (this.nodes.isBanned(peer.nodePubKey!)) {
+      // TODO: Ban IP address for this session if banned peer attempts repeated connections.


Do you plan to ban IP addresses prior to handshake, or here?

I think in both places, or rather right after the handshake. The if statement here is supposed to catch peers who are banned who connect to us anyway. The Peer class doesn't know about which nodes are banned which is why the logic is here. But I think if the handshake is bad or invalid, we can also put logic to ban before emitting the open event.

We were talking earlier about blocking inbound peers prior to handshake, based on IP address. Just wanted to know if we plan to support that, because if we do, we'll need to query nodes by IP address. If we don't, we're more vulnerable to DDoS.

This doesn't apply for banning within session, if we'll just collect banned IP addresses.
But we might want to apply this to blacklist/whitelist modes, which should persist, and might refer to a node entity - not strictly an IP address.

But anyway, we'll deal with that when we'll get to it.

moshababo · 2018-08-13T09:56:46Z

lib/p2p/SocketAddress.ts

+    return {
+      host: this.host,
+      port: this.port,
+    };


why using both SocketAddress & Address, if you already renamed some of the codebase to Address?

It's perhaps a bit confusing, I tried to explain in comments but maybe I need to make it more clear. SocketAddress is a class with a handy toString method. Address is just a type representing the host+port tuple. Maybe we can drop SocketAddress though and just use Address plus a utils method to convert an Address to the host:port string we use in several places though.

moshababo · 2018-08-13T10:52:01Z

lib/p2p/Peer.ts

@@ -416,17 +414,23 @@ class Peer extends EventEmitter {

    this.socket.once('close', (hadError) => {
      // emitted once the socket is fully closed
-      if (hadError) {
-        this.logger.warn(`Socket closed due to error (${this.id})`);
+      if (this.nodePubKey === undefined) {


as discussed in #327, I think it would be good to log pre-handshake socket close only if connection was already established.

Sounds good, but keep the verbose log if we're connected but pre-handshake?

I think it should be on info, same as the other non-error disconnection

moshababo · 2018-08-13T11:00:48Z

lib/p2p/NodeList.ts

+  /**
+   * Return a [[NodeInstance]] array for nodes that haven't been banned.
+   */
+  public toArray = (): NodeInstance[] => {


not in usage

moshababo · 2018-08-13T11:03:06Z

lib/p2p/NodeList.ts

+  /**
+   * Return a [[NodeFactory]] array for nodes that haven't been banned and have known addresses.
+   */
+  public toFactoryArray = (): NodeFactory[] => {


I think using the Factory term here is a bit confusing, because it's from the DB row context. Maybe create a different type, with the same properties.

That's true, so basically create an alias for the type? I'll look into it.

Yep, something like this.

moshababo · 2018-08-13T11:04:34Z

lib/p2p/Pool.ts

+    await this.nodes.load();
+    this.nodes.forEach((node) => {
+      this.connectNode(node);
+    });


connecting to banned nodes as well?

Good catch, I'll fix this. I'll probably put the logic in one place since similar logic is also being used on receiving a NODES packet.

moshababo · 2018-08-13T11:22:29Z

lib/p2p/Pool.ts

+  private connectNode = async ({ addresses, nodePubKey }: NodeFactory) => {
+    for (let n = 0; n < addresses.length; n += 1) {
+      try {
+        await this.addOutbound(addresses[n], nodePubKey);


why connecting to all the node's addresses? if one succeed, isn't it sufficient? specially if they are all using the same pubkey.

Good catch as well, I thought I'd put a break statement here but apparently not.

sangaman · 2018-08-13T16:04:28Z

Conflicts resolved and suggested changes addressed, there are still a few open questions to @moshababo. I created a type alias called NodeConnectionInfo for NodeFactory, let me know if you think it's good to keep it that way.

michael1011

Just a few more minor details.

michael1011 · 2018-08-13T18:31:09Z

instructions-for-devs.md

+  --rpc.port, -r      gRPC service port                                 [number]
+  --webproxy.disable  Disable web proxy server                         [boolean]
+  --webproxy.port     Port for web proxy server                         [number]
+>>>>>>> 789fccd... update readme


You overlooked some conflicts here.

Thanks, I'll fix it.

michael1011 · 2018-08-13T18:44:55Z

lib/db/models/Node.ts

+
+export default (sequelize: Sequelize.Sequelize, DataTypes: Sequelize.DataTypes) => {
+  const attributes: db.SequelizeAttributes<db.NodeAttributes> = {
+    id: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true },


Is there a reason the nodePubKey is not the primaryKey?

Eventually we'll have a table to track node reputation events - and perhaps more tables to track successful trade history or other information related to nodes - and being able to use an integer id as a foreign key will be better for those tables.

michael1011 · 2018-08-13T19:04:41Z

lib/p2p/Peer.ts

@@ -122,13 +108,19 @@ class Peer extends EventEmitter {

  public getStatus = (): string => {
    if (this.connected) {
-      return `Connected to peer (${this.id})`;
+      return `Connected to peer ${this.socketAddress.toString()}`;


The public key of the node is used as identifier everywhere else. Even in the log messages. Maybe you can add it here too.

At this point we might not know the public key of the node (at least if it's an incoming connection), this is being called before the handshake from the initConnection method. But you're right that it's a little unusual. Maybe it should say Connected to peer [nodePubKey] if it has a valid handshakeState and Connected pre-handshake to peer [socketAddress] otherwise?

No it is fine to leave it this way. I didn't think of inbound connections.

michael1011 · 2018-08-13T19:13:46Z

lib/p2p/Peer.ts

+        if (this.nodePubKey === undefined) {
+          this.logger.verbose(`Socket closed prior to handshake (${this.socketAddress.toString()})`);
+        } else if (hadError) {
+          this.logger.warn(`Peer ${this.nodePubKey} socket closed due to error (${this.socketAddress.toString()})`);


Is this.socketAddress.toString() returning the error in this case or does it get printed in a different place?

Yeah the error would get logged elsewhere, but this message is a little unclear. Maybe it should just not print the socket address if we have a nodePubKey value to print instead.

sangaman · 2018-08-14T05:12:19Z

I removed the SocketAddress class and replaced it with addressUtils helper methods. Let me know what you think.

moshababo

Code looks good to me.

moshababo · 2018-08-14T10:06:17Z

lib/utils/addressUtils.ts

+import assert from 'assert';
+
+/** Helper methods for interacting with the [[Address]] type. */
+export const addressUtils = {


This should be exported as the default.

Besides that, I preferred the class implementation, as it was before, specially because toString is called many times. In my earlier comment I was just wandering about the naming, but it wasn't an issue.

Both solutions are fine.

The reason I wanted a type was for the NodeFactory type definition plus a handful of other places where we were using host+port tuples. Using SocketAddress wouldn't have worked I believe because it was a class with a public toString method. I agree the current toString usage is a bit awkward, but if it's ok with you then we can leave it.

moshababo · 2018-08-14T10:16:24Z

lib/p2p/Peer.ts

+      if (this.connected) {
+        // don't log anything on close if we've never connected as the connection failure is logged elsewhere
+        if (this.nodePubKey === undefined) {
+          this.logger.verbose(`Socket closed prior to handshake (${addressUtils.toString(this.socketAddress)})`);


can be changed to info

moshababo · 2018-08-14T10:49:14Z

lib/p2p/Pool.ts

+      // check that this node is not ourselves, that it has listening addresses,
+      // and that either we haven't heard of it, or we're not ignoring known nodes and it's not banned
+      if (node.nodePubKey !== this.handshakeData.nodePubKey && node.addresses.length > 0 &&
+        (!this.nodes.has(node.nodePubKey) || (!ignoreKnown && !this.nodes.isBanned(node.nodePubKey)))) {


Suggestion: all conditions here are negative (including ignoreKnown). I think that making them positive will make it easier to follow. So basically checking whether to drop the node, instead of whether to connect to it. ignoreKnown can be turned to allowKnown,

What would that look like? I can't use a continue statement here unless I change it from a foreach. And fwiw if we are checking whether to skip a node, ignoreKnown would make more sense I figure.

I know it's a pretty complex if statement, that's why I added the comments above to try to make it more clear.

return should be the same for that manner.

But you can leave it as is if it doesn't work for you.

Oh right return would work, I didn't think of that. But I think I'll leave it as is if that's OK. One of the checks does happen to be positive (node.addresses.length > 0).

moshababo · 2018-08-14T10:53:17Z

lib/p2p/Pool.ts

+   * Attempt to create an outbound connection to a node using its known listening addresses.
+   */
+  private connectNode = async ({ addresses, nodePubKey }: NodeConnectionInfo) => {
+    for (let n = 0; n < addresses.length; n += 1) {


forEach instead? parallel to connectNodes

The reason I used a regular for loop is to be able to use the break command after the first successful connection, which I don't believe works with .forEach().

kilrau

add nodePubKey to xucli getinfo output
make [nodePubKey] optional in xucli connect
kilrau@ubuntu:~/xud/bin$ ./xucli connect 03b46799265e53fd8255915f4f62929728f42c6144e89d82626a64ca1f09ea792e 35.236.118.88 8885
Error: 3 INVALID_ARGUMENT: nodePubKey must be specified

sangaman · 2018-08-15T19:54:13Z

OK, Resolved conflicts again and addressed some of @moshababo's comments.

@kilrau Thanks for catching that bug with the cli, it's fixed now. I had to regenerate the type declarations for xudrpc and I used the new script I posted in #338. Give it a quick test again if you can.

sangaman · 2018-08-15T21:01:33Z

Had to make some changes to the sanity tests including a new test case, also fixed what I believe is a bug in that both nodes were using the same port. Travis tests pass but looks like the node isn't shutting down properly, so I'll look into that. Still good for testing and I think just needs this sanity test/shutdown thing straightened out.

Sanity tests don't work for me at all when I do them locally, whether on master or this branch. I get this every time.

       "before all" hook:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

kilrau

CLI connect still has issues @sangaman 😞
~/xud/bin$ ./xucli connect 03b46799265e53fd8255915f4f62929728f42c6144e89d82626a64ca1f09ea792e 35.236.118.88 8885
{
"result": "Not connected"
}
2018-8-15 16:56:05 [P2P] warn: error while opening connection to peer 03b46799265e53fd8255915f4f62929728f42c6144e89d82626a64ca1f09ea792e: Connection timed out.

Interestingy also now it accepts to not specify nodePubKey OR port and still fails as timeout. Did you make one of them optional finally?

sangaman · 2018-08-16T00:12:08Z

Can you provide an example of xucli connect not requiring nodePubKey? Port should not be required, but if I try to pass just host I get:

xucli connect 35.236.118.88
xucli connect <node_pub_key> <host> [port]

connect to an xu node

Options:
  --help          Show help                                            [boolean]
  --version       Show version number                                  [boolean]
  --rpc.port, -p  RPC service port                      [number] [default: 8886]
  --rpc.host, -h  RPC service hostname           [string] [default: "localhost"]
  --node_pub_key  nodePubKey of peer to connect                         [string]
  --host          hostname or IP address                                [string]
  --port          listening port number               [number] [default: "8885"]

Not enough non-option arguments: got 1, need at least 2

kilrau · 2018-08-16T02:22:21Z

Above connection issue is fixed now. Opened separate issue to catch error states caused by corrupted/unexpected data from the db: #349

Question regarding #162 : why does banned in nodes expect an integer? Is it NULL for notBanned and 1 for banned?

mysql> SELECT * FROM nodes;
+----+--------------------------------------------------------------------+------------------------------------------------------+--------+---------------------+---------------------+
| id | nodePubKey                                                         | addressesText                                        | banned | createdAt           | updatedAt           |
+----+--------------------------------------------------------------------+------------------------------------------------------+--------+---------------------+---------------------+
|  1 | 02b66438730d1fcdf4a4ae5d3d73e847a272f160fee2938e132b52cab0a0d9cfc6 | [{"host":"xud1.test.exchangeunion.com","port":8885}] |   NULL | 2018-08-16 01:37:23 | 2018-08-16 01:37:23 |
|  2 | 028599d05b18c0c3f8028915a17d603416f7276c822b6b2d20e71a3502bd0f9e0a | [{"host":"xud2.test.exchangeunion.com","port":8885}] |   NULL | 2018-08-16 01:37:23 | 2018-08-16 01:37:23 |
|  3 | 03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0 | [{"host":"xud3.test.exchangeunion.com","port":8885}] |   NULL | 2018-08-16 01:37:23 | 2018-08-16 01:37:23 |
|  4 | 029a96c975d301c1c8787fcb4647b5be65a3b8d8a70153ff72e3eac73759e5e345 | []                                                   |   NULL | 2018-08-16 01:48:39 | 2018-08-16 01:48:39 |
+----+--------------------------------------------------------------------+------------------------------------------------------+--------+---------------------+---------------------+
4 rows in set (0.00 sec)
mysql> INSERT INTO nodes VALUES ('5','abc','[{"host":"127.0.0.1","port":8885}]','NULL','date','date');
ERROR 1366 (HY000): Incorrect integer value: 'NULL' for column 'banned' at row 1

Also: why don't you query nodes for banned before connecting? I just manually added this banned host, assuming 1 means banned:
| 5 | abc | [{"host":"127.0.0.1","port":8885}] | 1 | 2018-08-16 01:37:23 | 2018-08-16 01:37:23 |
+----+--------------------------------------------------------------------+---------

and it did go through:

kilrau@ubuntu:~/xud/bin$ ./xucli connect abc 127.0.0.1 8885
{
  "result": "Not connected"
}
2018-8-15 19:16:22 [P2P] warn: error while opening connection to peer abc: Node at 127.0.0.1:8885 sent pub key 03d2fc7499fa9ee64290acdd12cd653e2c63963d660642422903c3c66c5dc98931, expected abc

Same with #177:

kilrau@ubuntu:~/xud/bin$ ./xucli connect 03d2fc7499fa9ee64290acdd12cd653e2c63963d660642422903c3c66c5dc98931 127.0.0.1 8885
{
  "result": "Connected to peer 03d2fc7499fa9ee64290acdd12cd653e2c63963d660642422903c3c66c5dc98931"
}

To be tested: order match, #271

sangaman · 2018-08-16T04:53:36Z

banned in the db is a tinyint(1), it's just what sequelize uses for booleans. It looks like the banned peer you tried connecting to resulted in Not Connected, right? I think we can take a closer look at banned peer behavior in a future issue/PR though.

I haven't tested explicitly trying to connect to oneself, in my tests it looks like it connects but fails the handshake. I can put in a check up front, but I'd rather do it in a separate PR to keep this one from getting bigger than it already is. It should be low priority, you have to intentionally be trying to mess things up by commanding xud to connect to yourself. What's prevented here is connecting to yourself when other peers tell you about yourself.

I fixed the builds and added more p2p sanity test cases, but I did detect a weird edge case bug I'll open an issue for later, it existed before this PR. I also fixed a bug with initialization, it was possible for the web proxy initialization to prevent xud from starting if it failed.

michael1011 · 2018-08-16T08:06:18Z

lib/p2p/Peer.ts

    assert(!this.opened);
    this.opened = true;

+    if (this.closed) {
+      throw 'wtf';


Good error message 🤣

Haha, I put this in as a sanity test for the sanity tests when I was trying to see why they'd never terminate. I'll probably change this to something a little more formal - probably just an assert like above - but it's really something that should never happen.

kilrau · 2018-08-16T16:50:43Z

Agree, put the upfront check in a separte issue: #352

sangaman · 2018-08-16T16:58:47Z

Ok last set of changes I hope, but I made it so that attempting to connect to yourself throws an error upfront before the handshake, and added a test case for that. @kilrau doing a few quick tests then this can be merged.

This is a structural change to the p2p layer with breaking database changes. - The term to represent a known xud instance that we are not necessarily connected to is changed from "Host" to "Node" and all references have been renamed. - The unique identifier for a node used throughout the codebase is their `nodePubKey` rather than their socket address. - External addresses for incoming connections are configurable and are shared during handshake. These are persisted to the `nodes` table as a JSON string under the `addressesText` column. - On receiving NODES packets, connections are only attempted to nodes that we were not previously aware of. This also makes minor fixes and adds new test cases to the p2p sanity tests.

Removes the `SocketAddress` class to avoid confusion with the `Address` type and moves the functionality to `addressUtils` helper classes.

Fixes a bug with the xud initialization that would hang when the web proxy server fails to start listening, which happens if the rpc server is disabled. Also initializes components which don't have dependencies on each other in parallel to improve start start time.

sangaman requested review from itsbalamurali, michael1011, offerm, moshababo and kilrau August 9, 2018 21:17

ghost assigned sangaman Aug 9, 2018

ghost added the in progress label Aug 9, 2018

sangaman mentioned this pull request Aug 10, 2018

Discover node's external IP #317

Merged

sangaman force-pushed the peer_host_restructuring branch from a8e5522 to 180ae97 Compare August 10, 2018 13:33

sangaman force-pushed the peer_host_restructuring branch from 180ae97 to bcbda53 Compare August 10, 2018 14:33

sangaman changed the title ~~Peer/Host restructuring - WIP~~ Peer/Host restructuring Aug 10, 2018

sangaman added the p2p Peer to peer networking label Aug 10, 2018

sangaman force-pushed the peer_host_restructuring branch from bcbda53 to 789fccd Compare August 11, 2018 04:39

This was referenced Aug 11, 2018

Error: 14 UNAVAILABLE: Connect Failed #320

Closed

log 'socket closed' msg only if already connected #327

Closed

michael1011 requested changes Aug 13, 2018

View reviewed changes

moshababo reviewed Aug 13, 2018

View reviewed changes

sangaman force-pushed the peer_host_restructuring branch from 789fccd to 2571e58 Compare August 13, 2018 16:03

sangaman requested a review from michael1011 August 13, 2018 16:04

michael1011 requested changes Aug 13, 2018

View reviewed changes

sangaman mentioned this pull request Aug 14, 2018

hosts table (db) should include only valid entries #305

Closed

sangaman force-pushed the peer_host_restructuring branch from 2571e58 to fe8c5ef Compare August 14, 2018 05:10

sangaman requested review from michael1011 and moshababo August 14, 2018 05:12

moshababo approved these changes Aug 14, 2018

View reviewed changes

kilrau suggested changes Aug 15, 2018

View reviewed changes

sangaman force-pushed the peer_host_restructuring branch from fe8c5ef to ffbe61f Compare August 15, 2018 19:49

sangaman requested a review from kilrau August 15, 2018 19:54

sangaman force-pushed the peer_host_restructuring branch 4 times, most recently from ebdcf8e to d10b05a Compare August 15, 2018 20:48

sangaman force-pushed the peer_host_restructuring branch from d10b05a to 523932c Compare August 15, 2018 21:35

kilrau suggested changes Aug 16, 2018

View reviewed changes

sangaman force-pushed the peer_host_restructuring branch from 523932c to 541bd42 Compare August 16, 2018 04:41

sangaman mentioned this pull request Aug 16, 2018

Catch db 'NULL's & other weirdo data from db #349

Closed

michael1011 approved these changes Aug 16, 2018

View reviewed changes

kilrau mentioned this pull request Aug 16, 2018

Check banned, self and others BEFORE connection #352

Closed

sangaman force-pushed the peer_host_restructuring branch from 541bd42 to 14955bf Compare August 16, 2018 16:53

sangaman added 4 commits August 16, 2018 13:15

update readme

ff8bfe2

replace SocketAddress w/ addressUtils

ba3baeb

Removes the `SocketAddress` class to avoid confusion with the `Address` type and moves the functionality to `addressUtils` helper classes.

sangaman force-pushed the peer_host_restructuring branch from 14955bf to 5bd11a1 Compare August 16, 2018 17:15

kilrau approved these changes Aug 16, 2018

View reviewed changes

kilrau mentioned this pull request Aug 16, 2018

Replace init task with seed values #351

Merged

sangaman merged commit db3d8cc into master Aug 16, 2018

ghost removed the in progress label Aug 16, 2018

michael1011 deleted the peer_host_restructuring branch August 20, 2018 18:13

Peer/Host restructuring #315

Peer/Host restructuring #315

Conversation

sangaman commented Aug 9, 2018 • edited Loading

sangaman commented Aug 9, 2018 • edited Loading

sangaman commented Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kilrau Aug 13, 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

moshababo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangaman Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

moshababo Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshababo Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

moshababo Aug 13, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangaman commented Aug 13, 2018

michael1011 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangaman Aug 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangaman commented Aug 14, 2018

moshababo 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

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

kilrau left a comment • edited Loading

Choose a reason for hiding this comment

sangaman commented Aug 15, 2018

sangaman commented Aug 15, 2018

kilrau left a comment

Choose a reason for hiding this comment

sangaman commented Aug 16, 2018

kilrau commented Aug 16, 2018 • edited Loading

sangaman commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kilrau commented Aug 16, 2018

sangaman commented Aug 16, 2018

sangaman commented Aug 9, 2018 •

edited

Loading

sangaman commented Aug 9, 2018 •

edited

Loading

sangaman commented Aug 10, 2018 •

edited

Loading

kilrau Aug 13, 2018 •

edited

Loading

sangaman Aug 13, 2018 •

edited

Loading

moshababo Aug 14, 2018 •

edited

Loading

moshababo Aug 14, 2018 •

edited

Loading

moshababo Aug 13, 2018 •

edited

Loading

sangaman Aug 13, 2018 •

edited

Loading

kilrau left a comment •

edited

Loading

kilrau commented Aug 16, 2018 •

edited

Loading