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

docker: Include vigilantes and explorer in the localnet deployment #123

Merged
merged 9 commits into from
Sep 10, 2022

Conversation

vitsalis
Copy link
Member

@vitsalis vitsalis commented Sep 8, 2022

This PR makes some modifications to the docker-compose methodology to setup a simnet. More specifically:

  1. Removes bitcoinsim from localnet-start. The reasoning for this is that localnet-start should only be about testing the Cosmos codebase itself and not the entire Babylon stack.
  2. For testing the entire babylon stack, the make simnet-start command is introduced. It builds the bitcoinsim machine, a localnet of 4 babylon nodes, a vigilante reporter, and a vigilante submitter. The vigilante inclusion is possible due to the recent update on the Dockerfiles for the vigilantes.
  3. A vigilante.yaml file is introduced. This file is useful for providing the configuration for the vigilante nodes. The aim is to ultimately remove this file by having the vigilante implement a command similar to babylond testnet to build a configuration file.
  4. The bitcoinsim image is updated so that it stores the rpc.cert and rpc.key files in a directory that can be accessed by the vigilante nodes. The vigilantes need the certificates to communicate with BTC properly.
  5. The shared volumes for the vigilante, BTC, and Babylon are stored under .testnets. This means that the .testnets directory now contains gentxs, node{i}, bitcoin, and vigilante directories.

Currently, the vigilante reporter on the above setup fail with the following error:

vigilante-reporter    | time="2022-09-08T04:25:16Z" level=info msg="Successfully loaded config file at /vigilante/vigilante.yaml" module=config
vigilante-reporter    | time="2022-09-08T04:25:16Z" level=info msg="Successfully created the BTC client and connected to the BTC server" module=btcclient
vigilante-reporter    | time="2022-09-08T04:25:16Z" level=info msg="Successfully subscribed to newly connected/disconnected blocks from BTC" module=btcclient
vigilante-reporter    | time="2022-09-08T04:25:16Z" level=debug msg="Babylon key directory: /babylonconfig" module=babylonclient
vigilante-reporter    | time="2022-09-08T04:25:16Z" level=debug msg="All Babylon addresses: map[]" module=babylonclient
vigilante-reporter    | time="2022-09-08T04:25:16Z" level=info msg="Successfully created the Babylon client" module=babylonclient
vigilante-reporter    | time="2022-09-08T04:25:16Z" level=info msg="Successfully generated TLS certificates for the RPC server" module=rpcserver
vigilante-reporter    | panic: rpc error: code = Unknown desc = unknown query path: unknown request
vigilante-reporter    |
vigilante-reporter    | goroutine 1 [running]:
vigilante-reporter    | github.com/babylonchain/vigilante/reporter.(*Reporter).Init(0x400014e780)
vigilante-reporter    |         /work/reporter/bootstrapping.go:23 +0x400
vigilante-reporter    | github.com/babylonchain/vigilante/cmd/reporter.cmdFunc(0x4000efe000?, {0x142caab?, 0x4?, 0x4?})
vigilante-reporter    |         /work/cmd/reporter/reporter.go:90 +0x24c
vigilante-reporter    | github.com/spf13/cobra.(*Command).execute(0x4000efe000, {0x4000e82880, 0x4, 0x4})
vigilante-reporter    |         /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:876 +0x4d0
vigilante-reporter    | github.com/spf13/cobra.(*Command).ExecuteC(0x400029db80)
vigilante-reporter    |         /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:990 +0x360
vigilante-reporter    | github.com/spf13/cobra.(*Command).Execute(...)
vigilante-reporter    |         /go/pkg/mod/github.com/spf13/cobra@v1.5.0/command.go:918
vigilante-reporter    | main.main()
vigilante-reporter    |         /work/cmd/main.go:22 +0x114

Still investigating the source of this, but I figured you people could take a look at the rest of the implementation while I do that or provide ideas as to why it might fail.

I would suggest that you try the make simnet-start command yourselves to check the stack. A future step would be to add the explorer to the stack as well.

@aakoshh
Copy link
Contributor

aakoshh commented Sep 8, 2022

The reasoning for this is that localnet-start should only be about testing the Cosmos codebase itself and not the entire Babylon stack.

It's difficult to say what localnet-start should be about. I thought that to test the Cosmos codebase you will need a BTC nodes to accept the checkpoints and the submitter/reporter to work as well, so a "local net" will need all of them. But if you found otherwise, for sure do whatever makes sense.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Looks nice! Have you found out the error?

README.md Outdated Show resolved Hide resolved
contrib/images/bitcoinsim/Dockerfile Show resolved Hide resolved
localnet:
ipv4_address: 192.168.10.6
volumes:
- ./.testnets/bitcoin:/bitcoinconf:Z
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is :Z required?

Copy link
Member Author

Choose a reason for hiding this comment

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

The :Z flag denotes that the volume is shared between containers (see here`

@vitsalis
Copy link
Member Author

vitsalis commented Sep 9, 2022

@aakoshh Pushed a few new updates:

  1. Remove the simnet-start command and do that in the localnet-start based on your comments. I agree that someone who wants to run a localnet, doesn't want to just run the Babylon nodes (which will do nothing by themselves) but wants to run the entire stack.
  2. Added the explorer to the deployment.
  3. Updated instructions to include the explorer.

The bug hasn't been resolved yet, will look for that right now.

@vitsalis vitsalis changed the title docker: Add simnet command with vigilante docker: Include vigilantes and explorer in the localnet deployment Sep 9, 2022
@vitsalis
Copy link
Member Author

vitsalis commented Sep 9, 2022

Turns out the bug was related to the vigilante not using the latest version of Babylon. #114 changed the endpoints of the btccheckpoint module so the latest version was needed. Fixed that at the vigilante repo.

vigilante.yaml Outdated Show resolved Hide resolved
restart: on-failure:10
vigilante-submitter:
container_name: vigilante-submitter
image: babylonchain/vigilante-submitter
Copy link
Contributor

Choose a reason for hiding this comment

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

The vigilante-submitter and vigilante-reporter are the same repo, same executables, no? Maybe one image with different command would be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would, but I believe it is better if we have those separate, since in the future they might be split up or work using different parameters etc.

Makefile Outdated
Comment on lines 458 to 459
build-bitcoinsim:
$(MAKE) -C contrib/images bitcoinsim
Copy link
Contributor

Choose a reason for hiding this comment

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

It's built automatically by docker-compose. Do we need a separate top level target?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary, but it can be useful imo.

docker-compose.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Great! Must have been quite an effort to figure out all these cert files and align all the config 🙇

@vitsalis vitsalis merged commit 7454857 into main Sep 10, 2022
@vitsalis vitsalis deleted the vigilante-deployment branch September 10, 2022 03:37
@KonradStaniec
Copy link
Collaborator

@vitsalis

  1. tbh It would be nice to leave localnet-start without vigiliante and submitter just for the sake on integration tests. I planned to start without them.
  2. For the future I would avoid merging pr-s with failing CI. If something is not easy fix and is really needed, I would prefer to disable failing check, and create separate task to fix it. Failing CI just breaks the flow for other people.

@vitsalis
Copy link
Member Author

Oh I'm sorry about that, for some reason I was under the impression that the build was already failing. Added a fix on #125

@KonradStaniec
Copy link
Collaborator

Yea we have some flaky test for now i created issue for it: #124

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.

3 participants