net: fixes for SPV

[rithvikvibhu]

Related (at least partially) to #705.

This fixes 2 things:

1. Mempool

• Nodes always expect a response when they ask for mempool
• If mempool is empty, another peer will not send any response, so the requesting node thinks the peer is stalling and disconnects after timeout (30 secs).
• To fix: removed mempool packet from addTimeout so the node doesn't wait for a response
• Also changed requestMempool to true on all networks (was only for regtest before). This makes SPV nodes request mempool on connecting to loader peers.

2. SendGetBlocks

• Once a chain is reset, it tells the pool to force resync.
• But, sendGetBlocks has a check to see if it's a repeat request (if the tip and stop match the last time it had sent getBlocks to that peer)
• If it matches, then getBlocks is not sent and we don't get the blocks, chain doesn't get blocks, walletdb doesn't get txs, no block connect events are emitted, etc.
• To fix: The peer's lastTip and lastStop are reset so that sendGetBlocks will not see it as a repeat request.

---

Added a test to net-spv-test.js since that seemed the most relevant (will separate it into a different file if needed).
All tests pass, but this PR probably needs more review/testing since it changes net code.

[coveralls]

Coverage increased (+0.009%) to 66.993% when pulling 9ca87f1 on rithvikvibhu:spv-net-fixes into dfa0741 on handshake-org:master.

[pinheadmz]

Instead of passing the force argument through a series of functions, why not add this to pool.resync() ?

diff --git a/lib/net/pool.js b/lib/net/pool.js
index eef0be3e8..311e27dab 100644
--- a/lib/net/pool.js
+++ b/lib/net/pool.js
@@ -783,6 +783,11 @@ class Pool extends EventEmitter {
       if (!force && peer.syncing)
         continue;
 
+      if (force) {
+        peer.lastTip = consensus.ZERO_HASH;
+        peer.lastStop = consensus.ZERO_HASH;
+      }
+
       this.sendLocator(locator, peer, force);
     }
   }

[pinheadmz]

Instead of using the completed flag in all those "spv on connect" tests, maybe they can all be replaced with test/util.forValue():

await forValue(spv.chain, height, 10))

I'm gonna take another look at this though because the existing test had more than one event listener and didnt throw the multiple resolves error, so maybe theres an even simpler solution...

[pinheadmz]

OK i think this is cleaner:

diff --git a/test/net-spv-test.js b/test/net-spv-test.js
index 6d6cead23..a8fd91270 100644
--- a/test/net-spv-test.js
+++ b/test/net-spv-test.js
@@ -11,6 +11,7 @@ const NameState = require('../lib/covenants/namestate');
 const {Resource} = require('../lib/dns/resource');
 const {types: packetTypes} = require('../lib/net/packets');
 const {types: urkelTypes} = require('urkel').Proof;
+const {forValue} = require('./util/common');
 
 const network = Network.get('regtest');
 const {
@@ -95,17 +96,8 @@ describe('SPV', function() {
 
     it('should generate blocks', async () => {
       addr = await wallet.receiveAddress(0);
-      const waiter = new Promise((res, rej) => {
-        let completed = false;
-        spv.on('connect', (entry) => {
-          if (!completed && entry.height === 10) {
-            completed = true;
-            res();
-          }
-        });
-      });
       await mineBlocks(10);
-      await waiter;
+      await forValue(spv.chain, 'height', 10);
       assert.strictEqual(full.chain.height, spv.chain.height);
     });
 
@@ -213,17 +205,8 @@ describe('SPV', function() {
       assert.strictEqual(proofType, urkelTypes.TYPE_EXISTS);
 
       // Restore
-      const waiter2 = new Promise((res, rej) => {
-        let completed = false;
-        spv.on('connect', (entry) => {
-          if (!completed && entry.height === full.chain.height) {
-            completed = true;
-            res();
-          }
-        });
-      });
       await spv.chain.removeInvalid(entry.hash);
-      await waiter2;
+      await forValue(spv.chain, 'height', full.chain.height);
       assert.strictEqual(full.chain.height, spv.chain.height);
     });
 
@@ -231,37 +214,17 @@ describe('SPV', function() {
       const resetHeight = 1;
       const fullNodeHeight = full.chain.height;
 
-      // Returns a promise that waits for spv wdb to catch up to full node
-      // `completed` ensures that the promise will not resolve multiple times
-      const waiter = () => new Promise((resolve) => {
-        let completed = false;
-        spv.on('connect', (entry) => {
-          if (!completed && entry.height === fullNodeHeight) {
-            completed = true;
-            resolve();
-          }
-        });
-      });
-
-      // If rescans don't complete in time, throw error
-      const timer = setTimeout(() => {
-        throw new Error('SPV rescan stuck.');
-      }, 1000);
-
       // Rescan once
       await spv.chain.reset(resetHeight);
       assert(spv.chain.height === resetHeight);
-      await waiter();
+      await forValue(spv.chain, 'height', fullNodeHeight);
       assert(spv.chain.height === fullNodeHeight);
 
       // Rescan again
       await spv.chain.reset(resetHeight);
       assert(spv.chain.height === resetHeight);
-      await waiter();
+      await forValue(spv.chain, 'height', fullNodeHeight);
       assert(spv.chain.height === fullNodeHeight);
-
-      // Rescan worked, cancel timer
-      clearTimeout(timer);
     });
 
     it('should request name data with unknown tree root', async () => {

[pinheadmz]

we maybe want to remove these as well:
assert(spv.chain.height === resetHeight);

Because i can see that being a race condition: like, if the spv node is already on its way back up the chain when that assertion is checked, it will be false

[pinheadmz]

I confirmed this is an issue with bcoin too by connecting to Bitcoin Core on regtest