feat(conductor)!: implement chain ID checks#1482
Conversation
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
Blocking this merge because this change exposes private types publicly
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
Don't expose implementation details in the public API of service crates.
| if let Some(SequencerChainIdError::MismatchedSequencerChainId { | ||
| expected, | ||
| actual, | ||
| }) = err.downcast_ref::<SequencerChainIdError>() |
There was a problem hiding this comment.
Don't test implementation details in blackbox tests.
It's fine to assert that it exits after receiving a bad chain ID.
| let mut source = e.source(); | ||
| while source.is_some() { | ||
| let err = source.unwrap(); | ||
| if let Some(CelestiaChainIdError::MismatchedCelestiaChainId { |
There was a problem hiding this comment.
As above, don't assert on implementation details in blackbox tests. Just check behaviour.
There was a problem hiding this comment.
With checking the error source text itself, I've managed to work it such that the tests still check the error result without exposing any underlying types. It feels like the tests should probably ensure that the correct thing is causing the error, as opposed to succeeding for any kind of early exit, but I understand this is fragile so I can change if needed.
| Mock as GrpcMock, | ||
| }; | ||
|
|
||
| // We have to create our own test conductor and perform mounts manually because `TestConductor` |
There was a problem hiding this comment.
Instead of doing all that lifting here, could you have made use of https://doc.rust-lang.org/std/mem/struct.ManuallyDrop.html to inhibit the custom Drop from being called?
There was a problem hiding this comment.
In attempting to do so, I'm not sure this method will work either. Calling await on the conductor contained by ManuallyDrop<TestConductor> errs because await takes ownership of the conductor. ManuallyDrop does have a method take() which would allow for this, but it is an unsafe function and as far as I know these can't be used in tests :/
There was a problem hiding this comment.
OK, let's leave this for now.
There was a problem hiding this comment.
Can you make a followup issue to clean this up?
…ub.com/astriaorg/astria into ENG-73/conductor_chain_id_verification
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
I think this is ready to get merged. :) Thank you for your work!
To bring this over the line I did two things:
- flatten the return value of the
conductor::Handlefuture by giving some extra context; - replaced the undocumented
eyre::ErrReportalias byeyre::Report.
| Ok(Err(err)) => Err(err.wrap_err("conductor failed failed")), | ||
| Err(err) => Err(eyre::ErrReport::from(err).wrap_err("conductor failed")), | ||
| Ok(Err(err)) => Err(err.wrap_err("conductor exited with an error")), | ||
| Err(err) => Err(eyre::ErrReport::from(err).wrap_err("conductor panicked")), |
There was a problem hiding this comment.
Please don't use undocumented type aliases like ErrReport.
* main: (34 commits) feat(proto): add bundle and optimistic block apis (#1519) feat(sequencer)!: make empty transactions invalid (#1609) chore(sequencer): simplify boolean expressions in `transaction container` (#1595) refactor(cli): merge argument parsing and command execution (#1568) feat(charts): astrotrek chart (#1513) chore(charts): genesis template to support latest changes (#1594) fix(ci): code freeze action fix (#1610) chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) (#1492) ci: code freeze through github actions (#1588) refactor(sequencer): use builder pattern for transaction container tests (#1592) feat(conductor)!: implement chain ID checks (#1482) chore(ci): upgrade audit-check (#1577) feat(sequencer)!: transaction categories on UnsignedTransaction (#1512) fix(charts): sequencer prometheus rules (#1598) chore(all): Migrate all instances of `#[allow]` to `#[expect]` (#1561) chore(charts,sequencer-faucet): asset precision (#1517) chore(docs): endpoints (#1543) fix(docker): use target binary build param as name of image entrypoint (#1573) fix(ci): ibc bridge test timeout (#1587) Feature: Add `graph-node` to charts. (#1489) ...
4 participants
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
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
ASTRIA_CONDUCTOR_SEQUENCER_CHAIN_IDandASTRIA_CONDUCTOR_CELESTIA_CHAIN_IDenvironment variables, without which conductor will not function.Related Issues
closes #986