feat(sequencer-relayer)!: provide filter for rollups #1001
Conversation
github-actions
bot
added
conductor
There was a problem hiding this comment.
I agree with the direction of this PR.
The only thing I am worried about is the relatively complex use of filter.is_empty() || filter.contains(&rollup_id) to test if a block should be permitted. Could we make this into a dedicated type with a single filter -> bool method instead?
This feels like the classic thing that is unclear a few months down (plus this gives us the chance to make all these clones cheaper).
crates/astria-sequencer-relayer/src/relayer/write/conversion.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks great, thank you for the PR.
I am not sure about the state of the blackbox since #963 removed a ton of that functionality.
Since #1008 is a follow-up PR that is imminent I would ask for having a special test specifically for using this filtering functionality.
Other that I only have some comments here and there and suggestions for maybe more explicit naming, none of which is blocking.
|
|
||
| const EXAMPLE_ENV: &str = include_str!("../local.env.example"); | ||
|
|
||
| #[test] | ||
| fn example_env_config_is_up_to_date() { | ||
| config::tests::example_env_config_is_up_to_date::<Config>(EXAMPLE_ENV); | ||
| } | ||
|
|
||
| #[test] | ||
| fn should_create_filter() { |
There was a problem hiding this comment.
There is a pattern to create oft-repeated tests that I learned from this blog post: https://matklad.github.io/2021/05/31/how-to-test.html
I believe the tests might be reexpressed like so:
#[track_caller]
fn assert_rollups_are_correctly_parsed(input: &str, expected: HashSet<RollupId>) {
let actual = IncludeRollup::new(&input).unwrap().0;
assert_eq!(actual, expected);
}I am not sure if this is useful here, but it reminded me of it.
There was a problem hiding this comment.
Yes, I generally favour that style too. But in this case, most of the assertions are one-liners, so I didn't think it was worthwhile.
| fn should_create_filter() { | ||
| let rollup_ids: HashSet<_> = (0..10).map(|i| RollupId::new([i; 32])).collect(); | ||
|
|
||
| // Normal form: "aaa,bbb,ccc". |
There was a problem hiding this comment.
All these comments are very useful! Would they also make sense in the assert_eq! message? i.e.:
assert_eq!(
*IncludeRollup::new(&input).unwrap().0,
rollup_ids,
"input of the form `aaa,bbb,ccc,...` should pass"
);There was a problem hiding this comment.
I'm thinking probably not, as there's a much greater chance of the IncludeRollup::new(&input).unwrap() panicking than the parsing reporting success but actually returning a different set. If the unwrap panics, we don't see the assertion message.
## Summary The black box tests were removed in a recent PR. This PR reinstates them using the new mock gRPC framework `astria-grpc-mock`. ## Background The tests would have needed heavy refactoring in #963, and we wanted to avoid adding a lot more code to that already-large PR. We decided to temporarily delete them and reinstate them using `astria-grpc-mock` to mock responses from the Celestia app. ## Changes The tests removed in #963 and a single test briefly added in #1001 then removed again have been restored. I also added a new test to check the relayer shuts down in a timely manner. Previously the tests leaned heavily on counts of blobs received by the mock Celestia app. The current tests retain these checks, but also query the `/status` endpoint of the sequencer-relayer to confirm its state. There was also a single test previously which checked the state of the postsubmit.json file. I didn't think this was necessary given that we're querying the state in a more "black box" way now (via the http server), but I didn't remove the function to perform this check (`TestSequencerRelayer::assert_state_files_are_as_expected`) pending a decision on whether to reinstate that check or not inside the `later_height_in_state_leads_to_expected_relay` test. As @SuperFluffy predicted, all the new protobuf packages added in #963 had to be added to the `pbjson_build` builder to generate the serde impls required for using them in `astria-grpc-mock`. This accounts for the vast majority of the new LoC in this PR. I made a few small changes to the mock framework: - added `Mock::up_to_n_times` to avoid failures due to a single-use mock response being sent multiple times - added `DynamicResponse` to support constructing mock responses which can be passed the relevant gRPC request - changed `MockGuard::wait_until_satisfied` to take `self` by ref rather than value, since if it consumes self and `wait_until_satisfied` times out, the `MockGuard` panics in its `Drop` impl, meaning we don't get a good indication from e.g. a `tokio::time::timeout` as to what went wrong Most of the new functionality lives in `MockCelestiaAppServer` and `TestSequencerRelayer`. For the former, all gRPCs except for `BroadcastTx` and `GetTx` are set up to always return valid-enough responses - i.e. these don't need to be mounted individually in every test case. In the case of `MockCelestiaAppServer`, the new functionality is a combination of support for mounting mock responses and functions with timeouts to query the `/status`, `/healthz` and `/readyz` http endpoints of the sequencer-relayer. ## Testing These changes are tests. Closes #1008.
Summary
A new configuration env var has been added to allow filtering data submission to a subset of rollups.
Background
This allows more flexibility in what is actually submitted to the Celestia chain.
Changes
A new env var was added, mapped to a new field
Config::rollup_id_filter. This represents a list of rollups whose transactions should be included in blobs submitted to Celestia. Rollups not in the list do not have their data submitted. However, if the list is empty, no filtering is applied; all rollups have their data submitted.The collection of filtered rollup IDs is passed down to the
write::conversion::convertfunction where sequencer blocks are converted to blobs. The blobs are filtered inside this function, partly since this is relatively early in the flow, but also so that theConversionInfostruct can carry information about blobs being excluded. This data is logged at the start ofwrite::submit_blobsin an info-level log message. It might be worthwhile adding metrics around the number of excluded rollups per submission, but I didn't see a clear benefit to that and it can be added in a follow-up PR if required.Testing
RollupIds.Added a black box test where a sequencer block is constructed with several transactions, half from included rollups and half from excluded ones. Existing black box tests ensure that when the filter is empty, no filtering is done.These all got removed in feat(sequencer-relayer)!: submit blobs directly to celestia app #963, and should be restored as part of Restore black box tests removed from sequencer-relayer #1008.Breaking Changelist
ASTRIA_SEQUENCER_RELAYER_ONLY_INCLUDE_ROLLUPSwhich can be empty or can be a comma-separated list.