bcoin icon indicating copy to clipboard operation
bcoin copied to clipboard

Add support for BIP158 filter types.

Open iamayushanand opened this issue 2 years ago • 10 comments

  • Adds support for filter types referenced in BIP158 & BIP157
  • Fixes the getblockfilter RPC message. Previously the RPC message implemented had the form getblockfilter "hash" however as per Bitcoin core docs, the RPC message is supposed to be getblockfilter "hash" ( "type" ) where the type by default is BASIC.

iamayushanand avatar May 22 '22 15:05 iamayushanand

Codecov Report

Merging #1057 (c528d5e) into master (96ac820) will increase coverage by 0.22%. The diff coverage is 75.64%.

:exclamation: Current head c528d5e differs from pull request most recent head 7c8a3fd. Consider uploading reports for the commit 7c8a3fd to get more accurate results

@@            Coverage Diff             @@
##           master    #1057      +/-   ##
==========================================
+ Coverage   67.35%   67.57%   +0.22%     
==========================================
  Files         157      157              
  Lines       26073    26111      +38     
==========================================
+ Hits        17561    17645      +84     
+ Misses       8512     8466      -46     
Impacted Files Coverage Δ
lib/indexer/index.js 0.00% <0.00%> (ø)
lib/blockstore/file.js 95.94% <37.50%> (+0.03%) :arrow_up:
lib/blockstore/level.js 78.84% <50.00%> (+9.61%) :arrow_up:
lib/golomb/golomb.js 93.60% <61.53%> (-1.14%) :arrow_down:
lib/node/rpc.js 40.88% <71.42%> (+0.65%) :arrow_up:
lib/primitives/block.js 75.36% <75.00%> (-1.13%) :arrow_down:
lib/node/fullnode.js 84.05% <93.75%> (+4.35%) :arrow_up:
lib/indexer/filterindexer.js 94.59% <94.44%> (+73.16%) :arrow_up:
lib/blockchain/chaindb.js 76.92% <100.00%> (ø)
lib/blockstore/common.js 100.00% <100.00%> (ø)
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar May 22 '22 15:05 codecov-commenter

Another big thing we should look at is, instead of modifying filterindexer -- we should use that as a base class and instantiate multiple indexers for different filter types. That's what Bitcoin Core does: Screen Shot 2022-05-31 at 8 55 57 PM

pinheadmz avatar Jun 01 '22 00:06 pinheadmz

It also might be good to add a new test file like filter-test.js which instantiates a full node with multiple filter indexers and tests them both for consistency.

It might be annoying though because the test would have to stub a lot of stuff, like blockstore/common.types and block.toFilter() so we should brainstorm and discuss

pinheadmz avatar Jun 15 '22 01:06 pinheadmz

We can also add filter indexer here!

https://github.com/bcoin-org/bcoin/blob/f8118d0d9aef493b1ec93ccf026eaf863c9e7056/lib/indexer/index.js#L13-L15

pinheadmz avatar Jun 15 '22 01:06 pinheadmz

this might need to be options.location??

https://github.com/bcoin-org/bcoin/blob/f8118d0d9aef493b1ec93ccf026eaf863c9e7056/lib/indexer/indexer.js#L155

--> bcoin --prefix ~/Desktop/filter-test --index-filter
[info] (blockstore) Opening FileBlockStore...
[info] (chain) Chain is loading.
[info] (chain) Checkpoints are enabled.
[info] (chaindb) Opening ChainDB...
[info] (chaindb) Writing genesis block to ChainDB.
[info] (chaindb) ChainDB successfully initialized.
[info] (chaindb) Chain State: hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f tx=1 coin=0 value=0.0.
[info] (chain) Chain Height: 0
[debug] (chain) Memory: rss=68mb, js-heap=11/28mb native-heap=40mb
[debug] (chain) Memory: rss=68mb, js-heap=11/28mb native-heap=40mb
[info] (mempool) Mempool loaded (maxsize=97656.25kb).
[info] (miner) Miner loaded (flags=mined by bcoin).
[warning] (miner) No reward address is set for miner!
[info] (net) Pool loaded (maxpeers=8).
[info] (net) Initialized header chain to height 0 (checkpoint=0000000069e244f73d78e8fd29ba2fd2ed618bd6fa2ee92559f542fdb26e7c1d).
[info] (filter/basicindexer) Indexer is loading.
Error: NotFound: /Users/matthewzipkin/Desktop/filter-test/index/filter/basic/LOCK: No such file or directory

pinheadmz avatar Jun 15 '22 01:06 pinheadmz

TODO: MIGRATION

pinheadmz avatar Jun 22 '22 14:06 pinheadmz

todo: await this.db.verify(layout.V.encode(), 'index', 1);

pinheadmz avatar Jul 06 '22 14:07 pinheadmz

✅ Tested _index method by deleting index folders and restarting bcoin

🤔 Tested migration script by synching 100000 blocks with bcoin v2.2.0 then upgrading, some issues:

  1. We must enforce version number:
diff --git a/lib/indexer/indexer.js b/lib/indexer/indexer.js
index 2aa2e80bd..697fe8e24 100644
--- a/lib/indexer/indexer.js
+++ b/lib/indexer/indexer.js
@@ -110,7 +110,7 @@ class Indexer extends EventEmitter {
     this.closing = false;
     await this.ensure();
     await this.db.open();
-    await this.db.verify(layout.V.encode(), 'index', 0);
+    await this.db.verify(layout.V.encode(), 'index', 1);
     await this.verifyNetwork();
 
     // Initialize the indexed height.

This gives us an error on boot, which is a GOOD THING:

[info] (txindexer) Indexer is loading.
Error: Database version mismatch for database: "index". Please run a data migration before opening.
    at DB.verify (/Users/matthewzipkin/Desktop/work/bcoin/node_modules/bdb/lib/db.js:568:13)
    at async TXIndexer.open (/Users/matthewzipkin/Desktop/work/bcoin/lib/indexer/indexer.js:113:5)
    at async FullNode.open (/Users/matthewzipkin/Desktop/work/bcoin/lib/node/fullnode.js:296:7)
    at async /Users/matthewzipkin/Desktop/work/bcoin/bin/node:53:3
  1. the indexer update should be included (I think) in migrate/latest although we should double check that works

  2. I think the usage for the migration script should be node migrate/indexerdb0to1.js ~/.bcoin/index

  • the main loop of the migrate script (starting on line 38 with (async () => { ... should iterate through the subdirectories present in ~/.bcoin/index and then pass each subdir to the updateVersion() function, which checks versions and skips if db is already upgraded
  • when it gets to the filter subdir I think it should clone FIRST, and then update the version of the CLONE db in the new correct folder, then DELETE the original. This should cover any scenario where the process crashes before it is completed.

pinheadmz avatar Jul 07 '22 15:07 pinheadmz

Ok I went through everything again and it so SO good. Really great work and alot of hard work here! SO thankyou and congratulations, we're almost there. I think there's just a few style nits, and then we gotta make that migration clean. All the internal indexer stuff is really really solid.

pinheadmz avatar Jul 07 '22 15:07 pinheadmz

We also need the hashes of the filters to be saved in the db so that we can use them in the getCFHeaders message.

masterchief164 avatar Jul 14 '22 15:07 masterchief164