Separate mempool indexer #650

nodech · 2018-12-10T19:20:20Z

Related Issues in bcoin: #594 and #499.

Summary:
This separate coin/tx indexer from the mempool and moves to its separate file.

Separate mempool indexer from the mempool.
Fix mempool indexer issues on reorg
Fix mempool indexer issues when recovering from persistent storage.
Add view to the add entry event.

Other PRs:

use before and after for setting up.

remove unnecessary methods

When restoring mempool from db, it returns transactions sorted by txid. But indexer logic depends on transactions being in dependency orders, so it does not have to deal with the orphan logic. This will change load method to use proper orphan handling and emit events in order even on load.

codecov-io · 2018-12-10T19:22:35Z

Codecov Report

Merging #650 into master will increase coverage by 1.21%.
The diff coverage is 86.32%.

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
+ Coverage   53.33%   54.55%   +1.21%     
==========================================
  Files         104      105       +1     
  Lines       27676    27725      +49     
  Branches     4738     4744       +6     
==========================================
+ Hits        14762    15126     +364     
+ Misses      12914    12599     -315

Impacted Files	Coverage Δ
lib/node/fullnode.js	`53.48% <25%> (-1.45%)`	⬇️
lib/mempool/mempool.js	`64.21% <68.18%> (+20.78%)`	⬆️
lib/mempool/indexer.js	`92.69% <92.69%> (ø)`
lib/coins/compress.js	`55.06% <0%> (-1.27%)`	⬇️
lib/primitives/block.js	`66.33% <0%> (+0.98%)`	⬆️
lib/blockchain/chain.js	`62.59% <0%> (+2.03%)`	⬆️
lib/mempool/mempoolentry.js	`85.36% <0%> (+20.12%)`	⬆️

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 0b70a94...811fc0d. Read the comment docs.

braydonf · 2019-03-26T22:49:51Z

In regards to the fix for #499 and #594 here is the scenario that would cause the issue:

Tip block has tx A and outputs: A[0], A[1].
Mempool tx B spends A[0].
Tip block becomes unconfirmed and all txs are added to the mempool including tx A, this would call indexEntry (and addEntry in the refactor) that would add the coins back to the mempool.
Mempool tx B has spent those outputs, so the coins should not be added back to the mempool.
The fix is checking that the outputs are spent, and removing them in the unconfirmed function for MempoolIndexer. This is tested in the block for "should make unconfirmed coins available".

Thanks @nodar-chkuaselidze for explanation of the above scenario.

braydonf · 2019-03-27T16:15:44Z

Taking a look at the persistence issue for the mempool, and it's also related to the coin index. As described in the commit 66042d8 the coin index depends on the order, and thus there is an issue when restoring, if the order is not correct. I've applied to same test to the current master branch 01a7156 to demonstrate the issue, see braydonf@73fab1a. The test will fail at assertions of the coin index size, the test will pass without these assertions (lines 664 and 644), and will pass with the assertions for the tx address index.

tynes · 2019-04-16T21:06:26Z

lib/mempool/indexer.js

+  /**
+   * Find all transactions pertaining to a certain address.
+   * Note: this does not accept multiple addresses.
+   * @param {Address} addrs


Why does this function accept addrs instead of addr and then place the single address into an array to iterate over if it only accepts a single address?

tynes · 2019-04-16T21:17:43Z

This could be exported in lib/bcoin{-browser}.js like https://github.com/bcoin-org/bcoin/pull/758/files#diff-303d6ea5bde0409e0d148f538471662aR58

tynes · 2019-04-16T21:20:03Z

lib/mempool/indexer.js

+ * @alias module:mempool.MempoolIndexerOptions
+ */
+
+class MempoolIndexerOptions {


These options don't seem necessary since there is really only 1 option that it sets

tynes · 2019-04-16T21:21:00Z

lib/mempool/indexer.js

+    for (const addr of addrs) {
+      const items = this.index.get(addr);
+
+      assert(items);


These assert statements could use messages

tynes · 2019-04-16T21:26:08Z

lib/node/fullnode.js

@@ -404,8 +417,12 @@ class FullNode extends Node {
   */

  async getCoinsByAddress(addrs) {
-    const mempool = this.mempool.getCoinsByAddress(addrs);
+    if (!this.mempoolIndex)


Why return early here when there is no mempoolIndex? Can't there be coins that are in the chain db still?

braydonf · 2019-04-16T22:03:06Z

These updates are not necessary with #758 and have included other fixes and updates well. It may be nice to separate the indexer and combine with lib/indexers and some point, but it's fairly small so I think can stay in lib/mempool.

braydonf · 2019-04-17T03:51:03Z

I've incorporated tests from this PR into #758

nodech added 8 commits December 10, 2018 18:43

mempool: add hasCoin method

11543f4

mempool-indexer: add separate indexer

073d499

mempool-test: add more test.

ca686ad

use before and after for setting up.

mempool-indexer: accept arrays

8f5b130

remove unnecessary methods

mempool-test: implement indexer checks

45b3ffa

fullnode: use mempool/indexer

f99d36f

fullnode: check if we have mempool indexer.

811fc0d

This was referenced Dec 12, 2018

Separate Mempool indexer bcoin-org/bcash#117

Closed

mempool: separate mempool indexer handshake-org/hsd#81

Closed

pinheadmz added enhancement Improving a current feature tests Has tests for code change is just an addtional test labels Jan 23, 2019

braydonf added indexer mempool Related to the mempool labels Mar 26, 2019

braydonf added the ready for review Ready to be reviewed label Mar 27, 2019

tynes reviewed Apr 16, 2019

View reviewed changes

tynes added the waiting for rebase The PR needs to be updated before it can be merged label Apr 16, 2019

tynes reviewed Apr 16, 2019

View reviewed changes

braydonf removed the ready for review Ready to be reviewed label Apr 16, 2019

braydonf removed the waiting for rebase The PR needs to be updated before it can be merged label Apr 16, 2019

nodech closed this Apr 18, 2019

nodech deleted the mempool/index branch June 28, 2019 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate mempool indexer #650

Separate mempool indexer #650

nodech commented Dec 10, 2018 •

edited

Loading

codecov-io commented Dec 10, 2018

braydonf commented Mar 26, 2019

braydonf commented Mar 27, 2019 •

edited

Loading

tynes Apr 16, 2019

tynes commented Apr 16, 2019

tynes Apr 16, 2019

tynes Apr 16, 2019

tynes Apr 16, 2019

braydonf commented Apr 16, 2019

braydonf commented Apr 17, 2019

Separate mempool indexer #650

Separate mempool indexer #650

Conversation

nodech commented Dec 10, 2018 • edited Loading

codecov-io commented Dec 10, 2018

Codecov Report

braydonf commented Mar 26, 2019

braydonf commented Mar 27, 2019 • edited Loading

tynes Apr 16, 2019

Choose a reason for hiding this comment

tynes commented Apr 16, 2019

tynes Apr 16, 2019

Choose a reason for hiding this comment

tynes Apr 16, 2019

Choose a reason for hiding this comment

tynes Apr 16, 2019

Choose a reason for hiding this comment

braydonf commented Apr 16, 2019

braydonf commented Apr 17, 2019

nodech commented Dec 10, 2018 •

edited

Loading

braydonf commented Mar 27, 2019 •

edited

Loading