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

Self hosting backend service instruction #91

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

hiepmai-babylonchain
Copy link
Contributor

No description provided.

@jrwbabylonlab jrwbabylonlab requested a review from gitferry July 4, 2024 08:52
Copy link
Contributor

@filippos47 filippos47 left a comment

Choose a reason for hiding this comment

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

Big effort, very nice work!
Suggest to add the following sections to all infra / services:

  • Monitoring section. For the services, let's explain that they expose Prom metrics and they can be scraped. For the infra, we can suggest a Prom exporter to use. (refs: 1, 2)
  • Hardware and Network requirements: We are often asked by teams how many resources does a server require, how should it be deployed etc. Suggest to add generic guidelines.

docs/user-guides/phase-1/global-params.md Outdated Show resolved Hide resolved
docs/user-guides/phase-1/images/phase-1-overview.png Outdated Show resolved Hide resolved
sidebars.js Show resolved Hide resolved
sidebars.js Outdated Show resolved Hide resolved
sidebars.js Outdated Show resolved Hide resolved
docs/user-guides/phase-1/infra/rabbitmq.md Outdated Show resolved Hide resolved
docs/user-guides/phase-1/services/overview.md Outdated Show resolved Hide resolved
docs/user-guides/phase-1/services/staking-api.md Outdated Show resolved Hide resolved
docs/user-guides/phase-1/infra/rabbitmq.md Outdated Show resolved Hide resolved
docs/user-guides/phase-1/services/staking-api.md Outdated Show resolved Hide resolved
Comment on lines 6 to 10
# Global System Configuration

## Staking Parameters

The Global Configuration includes a series of versioned governance parameters that greatly affect the behaviour of the system. Detailed information can be found [here](https://github.com/babylonchain/networks/tree/main/bbn-test-4/parameters).
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we can copy the content here other than a reference?

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Recommend that on markdown we have 80 character lines.

@hiepmai-babylonchain
Copy link
Contributor Author

Thanks for the recommendation @vitsalis.
As I will need to prepare more documents in the future. Would you be able to explain the 80 character lines a bit more?
For example, I have the following sentence:
The Global Configuration includes a series of versioned governance parameters that greatly affect the behaviour of the system.
Would it be clearer if it were written as:

The Global Configuration includes a series of versioned governance parameters
that greatly affect the behaviour of the system.

Is this format easier for you to follow?

Copy link
Contributor

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

LGTM!

@vitsalis
Copy link
Member

Thanks for the recommendation @vitsalis. As I will need to prepare more documents in the future. Would you be able to explain the 80 character lines a bit more? For example, I have the following sentence: The Global Configuration includes a series of versioned governance parameters that greatly affect the behaviour of the system. Would it be clearer if it were written as:

The Global Configuration includes a series of versioned governance parameters
that greatly affect the behaviour of the system.

Is this format easier for you to follow?

@hiepmai-babylonchain yes, that's perfect. This is a good practice for markdown files as it is easier to review and edit.

Copy link
Contributor

@filippos47 filippos47 left a comment

Choose a reason for hiding this comment

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

Great job!
Small nits.

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

A big set of comments. Up to you which ones you want to include.


### 6.1 Create systemd service definition

Run the following command, replacing `your_username` with your actual username:
Copy link
Member

Choose a reason for hiding this comment

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

What does username refer to in this context? The user of the system?

Also, doesn't this only work with Linux? We should not that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's stated as Optional on the heading, should we just remove this section as it only works it Linux?

### 1.2 Install the binary by running

```bash
cd staking-expiry-checker
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we ask the user to decide on what version/release to use? Same for all other services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it in the first place but didn't include the version.
The reason is if we put specific version here, we will need to update the docs frequently. Otherwise, the version is not up-to-dated.
Alternatively, we could ask users to check the release page for updates. If we go this route, we'll need to indicate in the release notes whether a particular version is recommended.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, although this is counter-intuitive for the user though. How can the user know which versions are compatible with each other?


### 4.1 Create systemd service definition

Run the following command, replacing `your_username` with your actual username:
Copy link
Member

Choose a reason for hiding this comment

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

Linux only.

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

An additional round of comments after a quick look. Most major comment is that we refer to the system as the "Bitcoin Staking system" which might confuse users, as this is only the first phase, which includes a lock-only system. Additionally, it seems that some introductions and explanations of some terms are warranted.

To not delay the merge of this, no need to review again. Once my comments are resolved you can merge, and you can create some issues to track the improvements of the guide so that we can improve it in a more fine-grained manner instead of repeatedly reviewing a big PR.


![Overview of Bitcoin Staking backend deployment](images/phase-1-overview.png)

The Babylon Bitcoin Staking system comprises of the following components:
Copy link
Member

Choose a reason for hiding this comment

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

Let's not call this the "Bitcoin Staking system" as this might confuse a user that this system is all there is to Bitcoin Staking or a potential Babylon blockchain. This is the phase-1 lock only system. You can get an idea of how we typically describe the phase-1 lock-only system in this PR. We should also note that this system connects to a Bitcoin network.

### 1.2 Install the binary by running

```bash
cd staking-expiry-checker
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, although this is counter-intuitive for the user though. How can the user know which versions are compatible with each other?

The staking indexer is a tool that extracts BTC staking relevant data
from the Bitcoin blockchain, ensures that it follows the pre-requisites
for a valid staking transaction, and determines whether
the transaction should be active or not.
Copy link
Member

Choose a reason for hiding this comment

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

What's an active transaction? Have we defined this somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it from the staking-indexer README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to update to active stake to match with the glossary

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.

6 participants