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

validate communicating with correct sequencer chain #986

Closed
joroshiba opened this issue Apr 19, 2024 · 0 comments · Fixed by #1482
Closed

validate communicating with correct sequencer chain #986

joroshiba opened this issue Apr 19, 2024 · 0 comments · Fixed by #1482
Assignees

Comments

@joroshiba
Copy link
Member

joroshiba commented Apr 19, 2024

We should add a check to conductor, composer, and sequencer relayer to validate that the sequencer RPC they communicate with is of the expected Astria sequencer network.

It is possible that RPCs change, or are confusingly named. On startup the services should validate this. There should be an env var specifying the expected chain id and then it can be verified from genesis file.

### Tasks
- [x] composer verify chain id on startup
- [x] sequencer-relayer verify sequencer chain id on startup
- [x] conductor verify sequencer chain id on startup
github-merge-queue bot pushed a commit that referenced this issue Jun 6, 2024
… config env vars (#1063)

## Summary
Added two new configuration options specifying the expected chain IDs of
the sequencer and Celestia networks.

## Background
This is a guard rail to help avoid an operator accidentally connecting
the relayer to the wrong network.

## Changes
The configured values are used in the initialization phase, where the
relevant clients query the appropriate servers for their chain IDs.

The Celestia client was already retrieving this info prior to this PR.
Now it returns an error if the chain ID doesn't match, and this
particular error variant causes the endless retry loop (and ultimately
the process) to exit.

The sequencer client was not previously requesting this info, so it now
does this in a somewhat similar manner to the Celestia client. Likewise,
a chain ID mismatch there causes the process to exit.

## Testing
A blackbox test for a mismatch in each of the chain IDs was added.

## Breaking Changelist
- added `ASTRIA_SEQUENCER_RELAYER_SEQUENCER_CHAIN_ID`
- added `ASTRIA_SEQUENCER_RELAYER_CELESTIA_CHAIN_ID`

## Related Issues

Part of #986.
Closes #978.
github-merge-queue bot pushed a commit that referenced this issue Jul 22, 2024
## Summary
Added check to ensure the executor's configured chain_id and the
sequencer client's actual chain_id match.

## Background
This check ensures that the composer is communicating with the sequencer
RPC on the correct network.

## Changes
Added chain_id check to the composer executor builder.

Made executor builder async to accommodate for async communication with
sequencer client.

Moved nonce guard declaration from mock startup to individual tests in
executor tests.

Added function to mount a status response with a given chain_id to the
mock. This is so the executor builder can gather the status response
without throwing an HTTP 404 error.

## Testing
Added executor test to validate the failure of an executor build with a
chain_id mismatch.

## Related Issues
Part of #986

---------

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
Co-authored-by: Ethan Oroshiba <ethan@astria.org>
bharath-123 pushed a commit that referenced this issue Jul 25, 2024
## Summary
Added check to ensure the executor's configured chain_id and the
sequencer client's actual chain_id match.

## Background
This check ensures that the composer is communicating with the sequencer
RPC on the correct network.

## Changes
Added chain_id check to the composer executor builder.

Made executor builder async to accommodate for async communication with
sequencer client.

Moved nonce guard declaration from mock startup to individual tests in
executor tests.

Added function to mount a status response with a given chain_id to the
mock. This is so the executor builder can gather the status response
without throwing an HTTP 404 error.

## Testing
Added executor test to validate the failure of an executor build with a
chain_id mismatch.

## Related Issues
Part of #986

---------

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
Co-authored-by: Ethan Oroshiba <ethan@astria.org>
github-merge-queue bot pushed a commit that referenced this issue Oct 1, 2024
## Summary
Implemented chain ID validation for both the sequencer and Celestia
readers in Conductor.

## Background
This is to safeguard against connecting to an incorrect network.

This change was previously blocking on a switch to solely using gRPC for
communication with sequencer, but after offline discussion it was
decided to implement the chain ID check anyways using the HTTP client.

## Changes
- Added sequencer and Celestia chain ID environment variables.
- Added chain ID checks to the initialization of both the sequencer and
Celestia readers.
- Changed conductor `run_until_stopped()` to return with an error so
that it will not fail silently.

## Testing
Added 2 blackbox tests to ensure the conductor exits with the proper
error in the event of both a Celestia and sequencer chain ID mismatch.

## Breaking Changelist
- Added `ASTRIA_CONDUCTOR_SEQUENCER_CHAIN_ID` and
`ASTRIA_CONDUCTOR_CELESTIA_CHAIN_ID` environment variables, without
which conductor will not function.

## Related Issues
closes #986

---------

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
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 a pull request may close this issue.

2 participants