Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Neutrino Client #1163

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

manavdesai27
Copy link
Contributor

This pr implements neutrino client mode. This has the commit history cleaned for better reviewing.

  1. Added Neutrino node which will communicate with the network.

  2. Added

    • sendGetCFilters - Gets compact filters from peer.
    • sendGetCFHeaders - Gets Compact filter headers from peer.
    • sendGetCFCheckpt - Gets Filters using checkpoints
  3. Added saveFilter and saveFilterHeader methods in filterIndexer. While saving filterHeader I am keeping the filter field empty.

  4. Added NeutrinoState which helps to keep track of the filterHeaderHeight and the filterHeight

  5. Added

    • Headers-sync: This skips downloading the blocks and instead saves the block headers and once filter headers are synced, emits event called headersFull, which indicates that we can start fetching filter headers
    • Filter Headers Sync: Syncs filter headers and saves them after certain assertions. After all cfheaders are synced we emit cfheaders event, which indicates that we can start fetching filters.
    • Filters Sync: Syncs filters and saves them after certain assertions. After saving the filter, we emit cfilter event, which sends the filter to the wallet and marks beginning of filter matching for the particular filter.
  6. Added checkFilter: Checks the filter against all address hashes in the wallet db. Address hashes are first converted to output scripts and that is then matched against the filter.

  7. Added getBlockFromNode and getBlockPeer: These methods help the client to download a particular block when any filter matches on the wallet side.

  8. Added RPC methods:

    • getfiltercount: Gets the filter height in NeutrinoState
    • getfilterheadercount: Gets the filter header height in NeutrinoState
    • getblockfilterheader: Retrieves filter header for a particular block

@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch coverage: 76.47% and project coverage change: +0.49 🎉

Comparison is base (a4bf281) 70.26% compared to head (8e0bec0) 70.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
+ Coverage   70.26%   70.75%   +0.49%     
==========================================
  Files         174      175       +1     
  Lines       27367    27833     +466     
==========================================
+ Hits        19229    19693     +464     
- Misses       8138     8140       +2     
Impacted Files Coverage Δ
lib/bcoin-browser.js 0.00% <0.00%> (ø)
lib/bcoin.js 0.00% <0.00%> (ø)
lib/blockchain/layout.js 100.00% <ø> (ø)
lib/client/node.js 25.86% <0.00%> (-1.42%) ⬇️
lib/node/fullnode.js 85.42% <ø> (+0.88%) ⬆️
lib/protocol/networks.js 100.00% <ø> (ø)
lib/wallet/client.js 0.00% <0.00%> (ø)
lib/wallet/nullclient.js 87.50% <ø> (ø)
lib/node/http.js 58.20% <12.50%> (-1.74%) ⬇️
lib/node/rpc.js 42.04% <52.00%> (+0.17%) ⬆️
... and 10 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@manavdesai27 manavdesai27 marked this pull request as ready for review July 11, 2023 00:26
bin/bcoin Outdated Show resolved Hide resolved
bin/neutrino Outdated Show resolved Hide resolved
lib/node/neutrino.js Outdated Show resolved Hide resolved
@@ -50,6 +50,8 @@ class Indexer extends EventEmitter {
this.blocks = this.options.blocks;
this.chain = this.options.chain;

this.neutrino = this.options.neutrino;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably just live in FilterIndexer and leave the base class alone

Comment on lines +297 to +301
if (this.neutrino) {
if (!this.batch)
this.start();
return true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of doing this here, I think what we may need to do is in FilterIndexer.indexBlock() just check if neutrino, return false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I implement it this way, the logStatus at line 458 in indexer.js gives an error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also, the indexBlock is expected to return a value. Return false gives out an error

lib/blockchain/chaindb.js Outdated Show resolved Hide resolved
@@ -1001,7 +1008,7 @@ class ChainDB {
*/

async getRawBlock(block) {
if (this.options.spv)
if (this.options.spv && !this.options.neutrino)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm what's wrong with this line? We can't get raw blocks in neutrino mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need this in order to check if a requested block by wallet already exists in the database or not, in order to skip fetching it if it exists.

@@ -1478,7 +1493,7 @@ class ChainDB {
async saveBlock(entry, block, view) {
const hash = block.hash();

if (this.options.spv)
if (this.options.spv && !this.neutrinoSave)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here - SPV mode does just fine without calling saveBlock() Neutrino should be the same. Don't we save ChainEntry (headers) outside this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we get a pruned block on request by wallet, without the neutrinoSave flag, the chain skips saving the block.

lib/node/fullnode.js Outdated Show resolved Hide resolved
lib/node/fullnode.js Outdated Show resolved Hide resolved
lib/wallet/wallet.js Outdated Show resolved Hide resolved
test/wallet-neutrino-test.js Outdated Show resolved Hide resolved
@@ -73,58 +72,6 @@ describe('P2P', function () {
await node2.close();
});

describe('BIP157', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that this change triggered a bunch of code coverage warnings, will have to investigate further.

bin/neutrino Outdated Show resolved Hide resolved
lib/blockchain/chain.js Outdated Show resolved Hide resolved
lib/blockchain/chain.js Outdated Show resolved Hide resolved
lib/blockchain/chain.js Outdated Show resolved Hide resolved
lib/net/peer.js Outdated Show resolved Hide resolved
lib/node/http.js Show resolved Hide resolved
lib/node/neutrino.js Outdated Show resolved Hide resolved
lib/node/rpc.js Show resolved Hide resolved
test/neutrino-test.js Show resolved Hide resolved
test/p2p-test.js Show resolved Hide resolved
@manavdesai27 manavdesai27 force-pushed the neutrino-client branch 3 times, most recently from 059de6e to 7d2635c Compare July 17, 2023 05:11
@manavdesai27 manavdesai27 force-pushed the neutrino-client branch 2 times, most recently from a8bb572 to 934bdcc Compare July 17, 2023 06:24
@manavdesai27 manavdesai27 force-pushed the neutrino-client branch 2 times, most recently from 1dc89dc to bffb60a Compare July 20, 2023 06:10
@pinheadmz pinheadmz removed their request for review July 1, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants