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

Run a tendermint node in CI to test the RPC & light client #120

Closed
ebuchman opened this issue Dec 31, 2019 · 14 comments
Closed

Run a tendermint node in CI to test the RPC & light client #120

ebuchman opened this issue Dec 31, 2019 · 14 comments
Assignees
Labels
light-client Issues/features which involve the light client
Milestone

Comments

@ebuchman
Copy link
Member

As per some RPC incompatibilities being discovered, namely #88 and #4, we should have the CI spin up a tendermint node, eg. using the latest docker image, and run the currently ignored integration tests against it (via cargo test -- --ignored) - see #88 (comment)

@liamsi
Copy link
Member

liamsi commented Mar 6, 2020

Likewise for the light node.

Basically these 2 things need a tendermint instance to run / test against:

  1. Light node:
  1. Currently ignored integration tests:
    https://github.com/interchainio/tendermint-rs/blob/master/tendermint/tests/integration.rs

Ref: #144, informalsystems/hermes#22, #95

Maybe make it optional for the (ignored) tests to pass initially. So we can see the error output and fix the rpc issues and the (unknown) light node bugs.

@liamsi liamsi changed the title Run a tendermint node in CI to test the RPC client Run a tendermint node in CI to test the RPC & light client Mar 10, 2020
@greg-szabo
Copy link
Member

I also opened an issue in the tendermint-go repository to update the tendermint docker image so we can use it in GitHub Actions. This would make the conifguration simpler and we could get away from the custom tendermint docker image.

@greg-szabo
Copy link
Member

Opened PR to update the tendermint-go Dockerfile.

@greg-szabo
Copy link
Member

The new Dockerfile was merged to master in the tendermint repository. This means that changes will be available in the next 0.33 release.
I propose we keep the current custom docker image for 0.32 and by the time we upgrade our code to 0.33, hopefully there will be a new 0.33 release with the changes - and we can move over.

@greg-szabo
Copy link
Member

I've been trying to run the light_node acceptance tests but I get bisect errors just starting the light_node. I'm going to park working on this while the code (or me) gets fixed.
Running a full node on the dev machine is slightly easier now because you can run a full node on your machine with one command, exactly how the CI does it:
docker run -it --rm -p 26657:26657 interchainio/tendermint. (Note: this will change when we upgrade to tendermint 0.33)
I'll reach out to @liamsi for further steps.

@greg-szabo
Copy link
Member

Light client CI blocks on #186 . @liamsi is aware, we'll troubleshoot further as time permits.

@zmanian
Copy link
Contributor

zmanian commented Mar 21, 2020

This to me looks a lot like the implicit clock sync issues we found in the IBC relayer light client? You are starting a new chain and new light client in different VMs and their clocks will likely be slightly out of sync. Introducing a delay of 5 -10 seconds before starting the light client helped in relayer development.

@liamsi
Copy link
Member

liamsi commented Mar 21, 2020

I think what Greg found is a much simpler and more substantial bug (but I haven't verify it yet). But we might run into the problem you described (when this bug is fixed).

@ebuchman
Copy link
Member Author

We should just need the genesis time for the chain we're using to be recent enough. We can also make the default trusting period longer. In any case doesn't seem like there's a bug, just a constraint on how we set up the test environment.

Note if we wanted the test to work against an existing public chain (eg hub-3), the client wouldn't be able to start from the first block. We'd have to provide a more recent height to initialize from (this couldn't be static, we'd want to do something like check a full node for a recent height and validator hash and use that to initialize the light client ...)

@romac romac added the light-client Issues/features which involve the light client label May 25, 2020
@liamsi
Copy link
Member

liamsi commented May 27, 2020

I have a suggestion on how to further improve the integration test situation:

Currently, we always run against the lates (go) tendermint docker image. On one hand this is really cool because we get early warnings for things that will break compatibility with the next release. On the other hand, we can't guarantee compatibility against a current release (e.g. we only want compatibility with 0.33.X or even only with 0.33.4 because that's what the hub is using or goz or whatever reasons there might be to want compatibility to a particular version).

I think tests would be even more useful if:

  1. they were run against a fixed and chosen tendermint version and were made mandatory (required on gh) for this version
  2. additionally integration tests for the latest image run regularly too (but much less frequently; e.g. nightly/weekly)

Regarding 1): we currently do not merge PRs unless integration tests pass anyways. The tools could better reflect this already established process and only allow merging if intgration tests pass too. Also, sometimes integration tests pass on the branch but fail on master (#249 #233 and many more). This happens because there was a new docker image released between last check on branch and merge to master. This would be mitigated if we made sure, we commit to compatibility of one tendermint version.

And 2) would help us to be learn about upcoming changes on a regular basis.

@ebuchman ebuchman added this to the Light Node milestone Jun 2, 2020
@liamsi
Copy link
Member

liamsi commented Jun 9, 2020

related:
#304 (comment)

@brapse
Copy link
Contributor

brapse commented Jun 15, 2020

This was resolved by closing #304

@brapse brapse closed this as completed Jun 15, 2020
@liamsi
Copy link
Member

liamsi commented Jun 15, 2020

Did I miss that we now spin up a light node (or an simple light client) against a (go) tendermint node? If not, I would reopen this issue. Or maybe it is better to open a new more focused one and link in your meta test-issue #320.

@ebuchman
Copy link
Member Author

As far as I can tell we do not do that. Opened #373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
light-client Issues/features which involve the light client
Projects
None yet
Development

No branches or pull requests

6 participants