bcoin icon indicating copy to clipboard operation
bcoin copied to clipboard

Add boolean option `http` to SPVNode

Open martindale opened this issue 5 years ago • 5 comments

Sharing my quick patch to disable HTTP listeners (re: #568). Comments and feedback desired, as I'm not sure of the side effects or current conventions (esp. re: this.options / this.config). Tests seem to pass (surprisingly).

martindale avatar Jan 15 '20 00:01 martindale

Codecov Report

Merging #927 into master will decrease coverage by <.01%. The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #927      +/-   ##
=========================================
- Coverage   62.01%     62%   -0.01%     
=========================================
  Files         156     156              
  Lines       26099   26100       +1     
=========================================
- Hits        16184   16183       -1     
- Misses       9915    9917       +2
Impacted Files Coverage Δ
lib/node/spvnode.js 31.52% <33.33%> (-1.45%) :arrow_down:

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 f2425d2...d104e72. Read the comment docs.

codecov-io avatar Jan 15 '20 00:01 codecov-io

Tests seem to pass (surprisingly)

Most http based tests run against the full node, not the spv node. I agree that not enabling the http servers could be useful, maybe an option no-http would be better as there are options like no-wallet and no-auth

tynes avatar Jan 15 '20 00:01 tynes

I think the option should be also the same for FullNode. Since the HTTP and RPC APIs share the same listener/port, it'll be useful to enable/disable each respectively or both. The config options http-api: <boolean>, and rpc-api: <boolean> could work well, or http: <boolean> and rpc: <boolean>.

braydonf avatar Jan 15 '20 01:01 braydonf

@martindale concept ACK! I made a similar commit here to illustrate some of the bcoin conventions we'd need from this PR:

  • Options to SPVNode should be copied to FullNode
  • Options typically have default values. Without a default true in this case, the http module would be implicitly false by default (undefined).
  • This option isn't really an http option, so it doesn't need to be passed to that module (it's an option at the Node level -- whether or not to start the http server)
  • My opinion is that this should be called http-listen so it doesn't get confused with any other http-thing.

However, this approach has a drawback which we discussed on IRC: socket events will not fire if the http module is not opened. I sort of understood that you didn't want any open http listeners in your application, but you were still counting on your SPV node to emit block events. If that's the case, then some deeper logic will be needed in the http module, and perhaps even its parent class Server from bweb.

The socket events require authentication for subscription, and that would mean that the http server has to at least be listening for that before socket events can begin.

pinheadmz avatar Jan 15 '20 14:01 pinheadmz

@martindale any interest in finishing up this PR? If not we may just take it over to formalize it and stage for merge.

pinheadmz avatar Feb 20 '20 13:02 pinheadmz