vanilla vigilante codebase #1
Conversation
|
@SebastianElvis this is fantastic, but a little overwhelming to review a PR that is almost 8,000 lines long. Do you think you could break it up into things that are I assume unchanged BTC wallet code, and then another PR that adds Babylon specific things to it? Are there any alternative reuse patterns other than copying the whole codebase? |
|
For example would it be possible to fork it, or use it as a library? Here's a description of how to create a private fork of a public repo: https://gist.github.com/0xjac/85097472043b697ab57ba1b1c7530274 |
Yeah I agree. The reason of such a big PR is that many packages depend on each other and a large portion of code is boilerplate code. For reviewing, I think you can skip |
Wow didn't know this private fork trick before. Perhaps for simplicity using btcclient as a library is a better way. Will try! |
|
Thanks for the review Akosh! I have resolved the comments, and now this PR:
Now the program output looks like
Please feel free to review again. |
| Copyright (c) 2022-2022 The Babylon developers | ||
| Copyright (c) 2013-2017 The btcsuite developers | ||
| Copyright (c) 2015-2016 The Decred developers |
| log.Info("Stopping BTC client...") | ||
| btcClient.Stop() | ||
| log.Info("BTC client shutdown") | ||
| }) |
There was a problem hiding this comment.
Seem no, as I did ctrl+c and all the goroutines will shut down gracefully, including metrics server
There was a problem hiding this comment.
Read the code again and found that you are right we need to stop the reporter/submitter here by reporter.Stop(), which also shuts btcClient. Not sure how to shut down metrics gracefully but I will look into it 👍
There was a problem hiding this comment.
Fixed the graceful shutdown of submitter/reporter in the last PR. Do you think we can merge the metrics server into the RPC server here?
There was a problem hiding this comment.
I don't know if it should be merged. You mean if it should be served at, say, localhost:26001 or localhost:8081/metrics? I don't have any opinion on this, because in practice you can put the whole thing behind nginx and use it as a reverse proxy to do this. For sure Prometheus can usually be disabled by a config, but that can be done both ways.
| log.Info("Stopping BTC client...") | ||
| btcClient.Stop() | ||
| log.Info("BTC client shutdown") | ||
| }) |
There was a problem hiding this comment.
Do you need to stop metrics and submitter?
| verString = "0.0.1" | ||
| verMajor = 0 | ||
| verMinor = 0 | ||
| verPatch = 1 |
There was a problem hiding this comment.
Maybe it would be good if at some point this came from git, for example like this https://gist.github.com/awalterschulze/d0f217db28e17b2ee56e8e8a4e68c28e
If you use git tags like v0.1.0 then it's possible to get a description from git which is like this version plus the latest commit hash, if it's not tagged directly, for example git describe --tags --always --long
That could be injected into this module and the service could parse it into a version and a commit hash part, depending on which one is present.
At some point Go will provide this on its own, see golang/go#37475
There was a problem hiding this comment.
This actually was just a placeholder RPC, not sure if we want this eventually 🤣
I'm fine with this approach (Didn't know this trick before 👍), but this seems to require the RPC API version to be the same as the software version, which is not always the case. For example, Bitcoin has its versions while its API versions are something like 70001.
There was a problem hiding this comment.
Oh I see, it wasn't obvious to me what version this was. As you know the gRPC versioning introduced by Google usually appears in the package name, so you have v1, then v2beta1 etc, with the intention that a service could run multiple of these to gracefully sunset older versions, but keep older clients going by letting them hit the URLs they can consume.
API versioning is a topic that I never saw getting anywhere, but I did not expect patch versions to appear in it. I thought it's more useful to expose the semantic version and commit hash of the application itself, so if something goes wrong you can ask what exact version somebody is running. Since this is a debug API in the first place of a CLI daemon, the API version is not that interesting since the only consumer is going to be the operator.
There was a problem hiding this comment.
Agreed. Yeah you remind me of how gRPC handles versions, where each set of APIs is already prefixed by stuff like v1beta1. Thus this version API is redundant. Perhaps let's keep it for now as a reference and I will remove it in the next PR that adds some real APIs.
There was a problem hiding this comment.
I would think an API version should only contain the major version in the URL because:
- a patch version is just a bug fix, not a new feature, and I don't suppose the API should keep supporting the bug, or break URLs by making everyone have to switch to a new version
- a minor version is a new, backwards compatible feature; users who don't consume it can just ignore it, they don't have to keep asking for an API without that feature
- a major version is a breaking change, so you have to let clients tell you they want the old version
There was a problem hiding this comment.
https://cloud.google.com/blog/products/api-management/common-misconceptions-about-api-versioning is an interesting read about why not to put the version in the URL :)
There was a problem hiding this comment.
Thanks for the pointer, nice article on API management! So this article suggests to use content negotiation (possibly at the HTTP layer) rather than API versioning. This makes sense to me and is supposed to be doable (gRPC server uses HTTP 2.0 which also supports negotiation). Let's stick to this then and will add it in subsequent PRs 👍
There was a problem hiding this comment.
I think it's totally worth keeping this endpoint, but use it to show the application version, rather than the interface version. Or both, just a general info stuff if you are interested in what kind of server you are talking to.
| endpoints: | ||
| - localhost:8080 | ||
| onetimetlskey: true | ||
| rpccert: /<your-user-directory>/Babylon-vigilante/rpc.cert |
There was a problem hiding this comment.
It should be possible, but sometimes the app directory is not under ~/ either. For example, my Macbook puts it under ~/Library/Application Support/Babylon-vigilante/ by default.
There was a problem hiding this comment.
What I see most of the time is that you can configure a single "data-dir", which is used as the root for all these other stuff, but the application can pick reasonable defaults if it's not set, which can be platform specific. Like it could be ~/.babylon/vigilante on Linux and whatever makes sense on MacOS, but overridden in one place (via command line flag or config field) if necessary.
There was a problem hiding this comment.
Actually this PR already follows this approach, where the default directory will be used if config file or command line arguments does not specify any, by using the defaultConfigFile variable in config/config.go. This yaml file is just a template.
There was a problem hiding this comment.
Ah okay, I thought you have to fill this or else it bombs or creates a directory called "<your-user-director>" in / root.
Co-authored-by: Filippos Malandrakis <philmln@protonmail.com>
Fixes BM-103
This PR introduces the vanilla vigilante. The major components are adapted from https://github.com/btcsuite/btcwallet.
Implemented functionalities and tech stack
btcclient/)cmd/)config/)netparams/)rpcserver/)Usage
To start a Bitcoin node
To start a vigilante (e.g., reporter)
such that the vigilante will create a WebSocket connection to the BTC node, and start a GRPC server.
Below are outputs from vigilant reporter and Bitcoin, respectively.
To query the GRPC server,
and we will get
Future work other than Jira tickets
netparams/)