Improve re-insertion of TXs from disconnected blocks into mempool

[pinheadmz]

Closes #915
Should be ported to Handshake after review to close handshake-org/hsd#306

Current behavior

1. In a FullNode, when a block is removed from the chain, the chain event 'disconnect' triggers mempool._removeBlock(), so the mempool can reset its internal tip, and also add back the unconfirmed transactions to the mempool. I'm not sure why, but if the mempool if emtpy at this time, the transaction re-insertions is totally skipped:

bcoin/lib/mempool/mempool.js

Lines 246 to 250 in 3623d04

	async _removeBlock(block, txs) {
	if (this.map.size === 0) {
	this.tip = block.prevBlock;
	return;
	}

2. In a FullNode, when chain emits a 'reorganize' event, that triggers mempool._handleReorg() which should clear out all transactions in the mempool (and those re-inserted from disconnected blocks) that are no longer valid at the new chain height. Examples of TXs that might not be valid after a reorg include premature coinbase spends and premature sequence-lock spends. The current implementation is incomplete in that it does not check the context of any transactions in this process: if a TX simply has a coinbase spend or has a sequence lock, it is dropped.

Updated behavior

1. mempool._removeBlock() attempts to re-insert transactions from disconnected blocks no matter what the current state of the mempool is. Note that each TX goes through the mempool.InsertTX() function which verifies everything as if it were the first time we'd ever seen the TX.

2. mempool._handleReorg() now checks the context of each TX in the mempool, via the spentView. If the TX spends from a coinbase, the maturity is checked. If the TX is version > 1, the sequence locks are checked. Transactions that are OK are allowed to remain in the mempool.

Testing

For what it's worth, this code passes the Bitcoin Core testing framework BIP68 test which I've been playing with on another branch. I did not implement a CSV-by-time test since the logic would be the same as by-height, but could if we want that. I've also got an actual CSV-by-time test in an open PR #613

Recommendation

I didn't include it in this PR, but I think it's also important that we check how 'disconnect' and 'reorganize' events are used throughout the codebase. For example, rpc invalidateblock will NOT emit either event, leaving the mempool in a potentially invalid state. Both _removeBlock() and _handleReorg() should be called on the mempool every time a block is disconnected, for any reason. I have one such implementation here, but given that @braydonf is working on the chain-expansion branch with so much chain refactoring, I just think we need to make sure that the mempool is handled correctly when we get there.

[codecov-io]

Codecov Report

Merging #917 into master will increase coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #917      +/-   ##
==========================================
+ Coverage   61.93%   62.12%   +0.18%     
==========================================
  Files         156      156              
  Lines       26093    26091       -2     
==========================================
+ Hits        16161    16208      +47     
+ Misses       9932     9883      -49

Impacted Files	Coverage Δ
lib/mempool/mempool.js	70.31% <100%> (+4.88%)	⬆️
lib/blockchain/chain.js	69.01% <0%> (+0.53%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3623d04...0a5d105. Read the comment docs.

[pinheadmz]

Pushed update to verify blocks with VERIFY_BODY and ensure that mempool contents are always valid in the next block after each relevant test.