bcoin
bcoin copied to clipboard
Add boolean option `http` to SPVNode
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).
Codecov Report
Merging #927 into master will decrease coverage by
<.01%
. The diff coverage is33.33%
.
@@ 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.
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
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>
.
@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 toFullNode
- Options typically have default values. Without a default
true
in this case, thehttp
module would be implicitlyfalse
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 theNode
level -- whether or not to start thehttp
server) - My opinion is that this should be called
http-listen
so it doesn't get confused with any otherhttp
-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.
@martindale any interest in finishing up this PR? If not we may just take it over to formalize it and stage for merge.