cculianu / Fulcrum

A fast & nimble SPV Server for BCH, BTC, and LTC
Other
331 stars 77 forks source link

bchd support #28

Closed cculianu closed 4 years ago

cculianu commented 4 years ago

From issue #27, we are creating a new issue since this arose:


@infertux wrote:

Alright the EC certificate works fine now but I'm running into another issue. Have you tried to point Fulcrum at bchd instead of bitcoind? It looks like bchd is more strict about checking HTTP headers and refusing the request sent by Fulcrum because it's missing the Host header:

<BitcoinD.1> 400 (header): HTTP/1.1 400 Bad Request: missing required Host header

Indeed if I MITM with netcat I cannot see any Host header:

$ nc -vlp 8334
listening on [any] 8334 ...
connect to [127.0.0.1] from localhost [127.0.0.1] 42256
POST / HTTP/1.1
Content-Type: application/json-rpc
Authorization: Basic blah==
Content-Length: 38

{"id":5,"method":"ping","params":[]}

Sending the request with curl works just fine (notice the extra headers):

$ curl http://127.0.0.1:8334/ -vi -H "Authorization: Basic blah==" --data '{"id":5,"method":"ping","params":[]}'

> POST / HTTP/1.1
> Host: 127.0.0.1:8334
> User-Agent: curl/7.64.0
> Accept: */*
> Authorization: Basic blah==
> Content-Length: 36
> Content-Type: application/x-www-form-urlencoded
[...]
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Connection: close
Connection: close
< Content-Type: application/json
Content-Type: application/json

< 
{"jsonrpc":"1.0","result":null,"error":null,"id":5}

Originally posted by @infertux in https://github.com/cculianu/Fulcrum/issues/27#issuecomment-654497449

cculianu commented 4 years ago

I added the Host: header. It's not enough. They don't implement HTTP/1.1 "persistent connections".. that is, bchd closes the connection on you rather than keeping it persistent. I remember looking into this before -- having the connection get closed each time and having to reconnect for EACH RPC MESSAGE severely impacts performance and synch time. See RFC: https://tools.ietf.org/html/rfc2616#section-8.1.2.1

@infertux - Any chance you can make some noise over at bchd and get them to fix this? Now that we're discussing this -- I remember why I never bothered to implement bchd support. This non-persistent connection thing was a deal-breaker for me.

cculianu commented 4 years ago

Issue created here with bchd: https://github.com/gcash/bchd/issues/380

infertux commented 4 years ago

Moving the conversation from #27 over here:

It's not a bug, but a feature. None of the bitcoind's except for bchd require it. We aren't implementing the full RFC anyway -- this is just to get data to/from a very customized server. The host header is useless here -- but ok. I'll add it and can see how deep the rabbit hole goes.

I disagree about "it's a feature". Just because bitcoind doesn't enforce the RFC doesn't mean Fulcrum should be buggy too.

I added the Host: header. It's not enough. They don't implement HTTP/1.1 "persistent connections".. that is, bchd closes the connection on you rather than keeping it persistent. I remember looking into this before -- having the connection get closed each time and having to reconnect for EACH RPC MESSAGE severely impacts performance and synch time. See RFC: https://tools.ietf.org/html/rfc2616#section-8.1.2.1

@infertux - Any chance you can make some noise over at bchd and get them to fix this? Now that we're discussing this -- I remember why I never bothered to implement bchd support. This non-persistent connection thing was a deal-breaker for me.

Wait a second... The RFC you linked reads "An HTTP/1.1 server MAY assume that a HTTP/1.1 client intends to maintain a persistent connection [...]". So a MAY is a deal-breaker to you but not sending a MUST Host header is not?! You're not making sense to me. I'm not gonna make noise over at bchd about this because I reckon it would be a waste of their time. If you want performance, use the gRPC endpoint, that's what it was designed for (among other things). It's a shame there is no good C++ lib for gRPC like you said in #27 though. I totally understand why you don't want to support gRPC then but wasting time reinventing the wheel on the legacy RPC is not the way to go IMHO.

I rely on bchd's gRPC API for my apps so I can't switch to BCHN. This means I have to stick with ElectrumX for the time being, which sucks because the main dev moved to BSV. I had high hopes about Fulcrum but you shouldn't advertise it as a "drop-in replacement for ElectronX/ElectrumX" as it's clearly not true.

That said, I do appreciate your contributions to BCH even though we're having a disagreement here. Eventually a decent C++ lib for gRPC will pop up and this won't be an issue anymore. Peace :)

cculianu commented 4 years ago

Wait a second... The RFC you linked reads "An HTTP/1.1 server MAY assume that a HTTP/1.1 client intends to maintain a persistent connection [...]". So a MAY is a deal-breaker to

Yes, it's a deal breaker for me. I don't want to re-initiate the connection each time. The code is already written in such a way where that introduces needless delays. This server is optimized for speed. There's a reason why it synchs 5x faster than ElectrumX and serves clients 10x faster. This is one of them. I am already working with the bchd devs and they will address this issue since they agree that it's wasteful too to not support persistent connections.

if you want performance, use the gRPC endpoint

Negative. I audited the gRPC C++ implementation and it's terrible. I refuse to use it. Until Google redoes their entire GRPC engine, it's not going to happen. Also I would need an entire different code path to support gRPC. Everybody else uses JSON-RPC via HTTP.

I totally understand why you don't want to support gRPC then but wasting time reinventing the wheel on the legacy RPC is not the way to go IMHO.

Mark my words: gRPC won't ever replace JSON RPC. It's not as fast or as good as is touted. In fact it's pretty slow since it does tons of double-copies. It's possible to get similar performance from JSON. On top of that, the learning curve is significant for many devs, as are the dependencies for languages such as C++. I do not believe gRPC is the future -- I think in 10 years JSON will still be around and gRPC will be replaced by something better. (Maybe this, or something like it)

I rely on bchd's gRPC API for my apps so I can't switch to BCHN. This means I have to stick with ElectrumX for the time being, which sucks because the main dev moved to BSV.

I hear ya. Don't worry -- things may resolve soon. It's coming! Be patient! I'm working with Josh Ellithorpe on the issue now. It could be fixed this week. At that point I will support bchd.. and I will ping you here and you can try it out too.

infertux commented 4 years ago

serves clients 10x faster. This is one of them. I am already working with the bchd devs and they will address this issue since they agree that it's wasteful too to not support persistent connections.

This is great to hear! I'll hold on then because 10x faster is worth the wait :)

if you want performance, use the gRPC endpoint

Negative. I audited the gRPC C++ implementation and it's terrible. I refuse to use it. Until Google redoes their entire GRPC engine, it's not going to happen. Also I would need an entire different code path to support gRPC. Everybody else uses JSON-RPC via HTTP.

I haven't looked at their C++ implementation but their Go one is pretty nice to use. I'm not a fan of Google but gotta admit gRPC is a breath of fresh air, especially after countless painful integrations with quirky JSON APIs over the years. I enjoy having a declarative Protobuf interface and not having to mess with random HTTP headers like Host and whatnot ;)

Mark my words: gRPC won't ever replace JSON RPC. It's not as fast or as good as is touted. In fact it's pretty slow since it does tons of double-copies. It's possible to get similar performance from JSON. On top of that, the learning curve is significant for many devs, as are the dependencies for languages such as C++. I do not believe gRPC is the future -- I think in 10 years JSON will still be around and gRPC will be replaced by something better. (Maybe this, or something like it)

Oh I don't think gRPC will replace JSON RPC either. Good old JSON RPC has many valid usecases. My point was that gRPC is fast as it's based on HTTP2 which multiplexes multiple streams over the same connection. It would be interesting to do a proper benchmark though syncing Fulcrum from scratch with the JSON and gRPC interfaces to see how they compare.

I hear ya. Don't worry -- things may resolve soon. It's coming! Be patient! I'm working with Josh Ellithorpe on the issue now. It could be fixed this week. At that point I will support bchd.. and I will ping you here and you can try it out too.

Awesome! I'll keep an eye out. Thank you.

cculianu commented 4 years ago

Status update: I worked with josh and I got fulcrum and bchd talking and synching. I just pushed some commits to work around some quirks of bchd. Here is josh's branch that implements the changes required to bchd: https://github.com/gcash/bchd/pull/384

Let me know if you want me to build you that version of bchd and send you a built Fulcrum too so you can take it for a spin.

cculianu commented 4 years ago

@infertux Note I just released Fulcrum 1.2.7 which can talk to bchd if it has the above-linked patch. If you like I can send you a built version of the above bchd (or you can build it yourself agianst that branch). Chris Pacia said he's amenable to above-linked PR he just has to make time to review it for merging.

infertux commented 4 years ago

@cculianu That's awesome news! Thank you. I'm busy with other stuff these days but will try it out soon. (I can build bchd myself, no worries.)

cculianu commented 4 years ago

Yeah man I always wanted to support bchd but the activation energy was high there because it required their help. They were very cool about it tho.

I have been running it for a few days now and it’s purring along like a kitten.

So yeah — lmk how it goes whenever you get around to it.. great that you can also build it.

cculianu commented 4 years ago

bchd v0.16.5 now works with Fulcrum 100%. It's out! https://github.com/gcash/bchd/releases/tag/v0.16.5

I'm closing this issue now due to it being resolved... but @infertux -- whenever you want now you can run the two together! :D

infertux commented 4 years ago

@cculianu I'm afraid there is still an issue, I just updated both bchd and fulcrum to the latest releases and here's the output from fulcrum:

[...]
[2020-07-24 21:03:47.986] Service started, listening for connections on 127.0.0.1:8080            
[2020-07-24 21:03:48.086] <Controller> Block height 645358, downloading new blocks ...                                                                                                                            
[2020-07-24 21:03:48.106] <BitcoinD.1> Unsupported "Connection: close" header field in response
[2020-07-24 21:03:48.106] <BitcoinD.1> TCP BitcoinD.1 (id: 2) 127.0.0.1:8334: error 1 (The remote host closed the connection)                                                                                     
[2020-07-24 21:03:48.106] <BitcoinD.1> Lost connection to bitcoind, will retry every 5 seconds ...
[2020-07-24 21:03:48.106] <Task.DL 14316 -> 645358> 310: FAIL: connection lost                 
[2020-07-24 21:03:48.106] <Task.DL 14314 -> 645358> 319: FAIL: connection lost                 
[2020-07-24 21:03:48.106] <Task.DL 14312 -> 645358> 321: FAIL: connection lost                      
[2020-07-24 21:03:48.106] <Task.DL 14315 -> 645358> 312: FAIL: connection lost                 
[2020-07-24 21:03:48.106] <Controller> Task errored: Task.DL 14316 -> 645358, error: connection lost

You can see it does get the block height fine but it says Unsupported "Connection: close" header field in response right after that. If I send a RPC request to bchd with curl like this:

$ curl -i --user foo:bar http:/127.0.0.1:8334/ --data '{"id":42,"method":"getbestblock","params":[]}'
HTTP/1.1 200 OK
Date: Fri, 24 Jul 2020 21:07:30 GMT
Content-Type: application/json
Content-Length: 140
Connection: keep-alive

{"jsonrpc":"1.0","result":{"hash":"00000000000000000197b5ef4794558c95639136b7b405970c3b2f29670f2e40","height":645358},"error":null,"id":42}

The Connection: keep-alive header looks fine but fulcrum says it receives Connection: close for some reason. Any ideas? Do I need to set some config options in fulcrum.conf to make it compatible with bchd?

cculianu commented 4 years ago

Wtf. I’m on vacation now. Let me try latest bch and fulcrum when I get back (later today).

I actually haven’t tried absolute latest bchd yet (the one they released). I hope nothing broke.

Thanks for the detailed log. I’ll get back to you in this.

cculianu commented 4 years ago

Hey -- I just tried this both on macOS and on linux against bchd v0.16.5 (latest).

2020-07-26 06:30:06.310] <BitcoinD.3> (Debug) on_connected 4
[2020-07-26 06:30:06.311] <HttpSrv 0.0.0.0:8080> (Debug) started ok
[2020-07-26 06:30:06.315] <Controller> (Debug) Auth recvd from bicoind with id: 3, proceeding with processing ...
[2020-07-26 06:30:06.316] <BitcoinDMgr> (Debug) Refreshed version info from bitcoind, version: 0.16.5, subversion: /bchd:0.16.5(EB32.0; Calins_BCHD)/
[2020-07-26 06:30:06.316] <BitcoinD.1> (Debug) Changed pingtime_ms: 3333
[2020-07-26 06:30:06.316] <BitcoinD.3> (Debug) Changed pingtime_ms: 3333
[2020-07-26 06:30:06.316] <BitcoinD.2> (Debug) Changed pingtime_ms: 3333
[2020-07-26 06:30:06.317] <BitcoinDMgr> (Debug) Refreshed genesis hash from bitcoind: 000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943
[2020-07-26 06:30:06.414] <Controller> Chain: test
[2020-07-26 06:30:06.414] <Storage> (Debug) Wrote new metadata to db
[2020-07-26 06:30:06.414] <Controller> Block height 1397559, downloading new blocks ...
[2020-07-26 06:30:06.414] <Controller> (Debug) Task.DL 0 -> 1397559 starting thread
[2020-07-26 06:30:06.414] <Controller> (Debug) Task.DL 1 -> 1397559 starting thread
[2020-07-26 06:30:06.414] <Controller> (Debug) Task.DL 2 -> 1397559 starting thread
[2020-07-26 06:30:06.414] <Controller> (Debug) Task.DL 3 -> 1397559 starting thread
[2020-07-26 06:30:06.414] <Controller> (Debug) Task.DL 4 -> 1397559 starting thread
[2020-07-26 06:30:06.414] <Controller> (Debug) Task.DL 5 -> 1397559 starting thread
[2020-07-26 06:30:06.415] <Controller> (Debug) Task.DL 6 -> 1397559 starting thread
[2020-07-26 06:30:06.626] <Controller> Processed height: 1000, 0.1%, 4721.7 blocks/sec, 9160.4 txs/sec, 19075.5 addrs/sec
[2020-07-26 06:30:06.715] <Controller> Processed height: 2000, 0.1%, 11236.0 blocks/sec, 11236.0 txs/sec, 11236.0 addrs/sec
[2020-07-26 06:30:06.887] <Controller> Processed height: 3000, 0.2%, 5814.0 blocks/sec, 5814.0 txs/sec, 5814.0 addrs/sec
[2020-07-26 06:30:07.135] <Controller> Processed height: 4000, 0.3%, 4016.1 blocks/sec, 4016.1 txs/sec, 4016.1 addrs/sec
[2020-07-26 06:30:07.337] <Controller> Processed height: 5000, 0.4%, 4975.1 blocks/sec, 54378.1 txs/sec, 85935.3 addrs/sec
[2020-07-26 06:30:07.432] <Controller> Processed height: 6000, 0.4%, 10526.3 blocks/sec, 10526.3 txs/sec, 10526.3 addrs/sec
[2020-07-26 06:30:07.611] <Controller> Processed height: 7000, 0.5%, 5586.6 blocks/sec, 5586.6 txs/sec, 5586.6 addrs/sec
[2020-07-26 06:30:07.712] <Controller> Processed height: 8000, 0.6%, 9901.0 blocks/sec, 9901.0 txs/sec, 9901.0 addrs/sec
[2020-07-26 06:30:07.889] <Controller> Processed height: 9000, 0.6%, 5618.0 blocks/sec, 5629.2 txs/sec, 5646.1 addrs/sec
[2020-07-26 06:30:07.979] <Controller> Processed height: 10000, 0.7%, 11236.0 blocks/sec, 11236.0 txs/sec, 11236.0 addrs/sec
[2020-07-26 06:30:08.154] <Controller> Processed height: 11000, 0.8%, 5714.3 blocks/sec, 5714.3 txs/sec, 5714.3 addrs/sec
[2020-07-26 06:30:08.268] <Controller> Processed height: 12000, 0.9%, 8771.9 blocks/sec, 8771.9 txs/sec, 8771.9 addrs/sec
[2020-07-26 06:30:08.372] <Controller> Processed height: 13000, 0.9%, 9523.8 blocks/sec, 9523.8 txs/sec, 9523.8 addrs/sec
[2020-07-26 06:30:08.515] <Controller> Processed height: 14000, 1.0%, 7042.3 blocks/sec, 7042.3 txs/sec, 7042.3 addrs/sec
[2020-07-26 06:30:08.620] <Controller> Processed height: 15000, 1.1%, 9523.8 blocks/sec, 9523.8 txs/sec, 9523.8 addrs/sec
[2020-07-26 06:30:08.742] <Controller> Processed height: 16000, 1.1%, 8196.7 blocks/sec, 8196.7 txs/sec, 8196.7 addrs/sec
[2020-07-26 06:30:08.847] <Controller> Processed height: 17000, 1.2%, 9523.8 blocks/sec, 12161.9 txs/sec, 23409.5 addrs/sec
[2020-07-26 06:30:08.935] <Controller> Processed height: 18000, 1.3%, 11363.6 blocks/sec, 11363.6 txs/sec, 11363.6 addrs/sec
[2020-07-26 06:30:09.107] <Controller> Processed height: 19000, 1.4%, 5814.0 blocks/sec, 5831.4 txs/sec, 5843.0 addrs/sec
[2020-07-26 06:30:09.199] <Controller> Processed height: 20000, 1.4%, 10869.6 blocks/sec, 12097.8 txs/sec, 13684.8 addrs/sec
[2020-07-26 06:30:09.283] <Controller> Processed height: 21000, 1.5%, 11904.8 blocks/sec, 13511.9 txs/sec, 16345.2 addrs

The above is the Linux (Ubuntu 20) -- It's working without a hitch... so -- I can't explain why you saw it fail with the close header. Also bchd should not be sending the Connection: close header now .. this makes me wonder if you built the wrong bchd?

I can't explain it otherwise....

cculianu commented 4 years ago

Also @infertux try running Fulcrum with the -d option to perhaps generate more debug output -- or even the -d -d option (twice) to see a network message trace...

infertux commented 4 years ago

@cculianu I finally figured it out. It was due to my weird setup. Sorry about the false alarm. I'll write down the details tomorrow but I need to sleep now :tired_face:

cculianu commented 4 years ago

Ah, ok. I'm curious what the setup was... a proxy perhaps?

infertux commented 4 years ago

@cculianu Indeed! I'm running bchd with TLS enabled for the gRPC API and that enables TLS for the RPC API as well. Fulcrum expect the RPC API to be plain HTTP so I was using nginx proxy_pass https://127.0.0.1:8334/ to strip the TLS encryption. However nginx adds a new header automatically - Connection: keep-alive - that you cannot hide even with proxy_hide_header. So I tried patching Fulcrum to ignore it, which mitigated the issue. However nginx would still send a Connection: close header after a while. At which point I said fuck nginx then tried with socat which worked perfectly!

So if anyone is running bchd with TLS enabled and wants to point Fulcrum to it, you can use socat TCP-LISTEN:1337,fork,reuseaddr ssl:127.0.0.1:8334,verify=0 then set bitcoind = 127.0.0.1:1337 in fulcrum.conf.

cculianu commented 4 years ago

Oh wow thanks for that. I'm going to pin this comment as a separate "issue" for people to quickly find it in case they run into the same thing.

cculianu commented 4 years ago

Ok, I cherry-picked your commit. There's no sense in warning on keep-alive, so this commit makes it not warn if it sees that header, which is what we want.

infertux commented 4 years ago

Sounds good to me. Thanks for your help. Glad to finally be running Fulcrum! :partying_face:

cculianu commented 4 years ago

@infertux I just pushed this commit ce4b969 to master. It allows Fulcrum to just contact bchd via HTTPS. Specify the --bitcoind-tls option on the CLI or bitcoind-tls = true in the conf file and it will "just work" with bchd https.

So now you don't have to use socat! 👍

infertux commented 4 years ago

@cculianu This is great. I assumed it would be harder to implement. Thanks Qt I guess ;)