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

Remove gossipnet from relayer #366

Closed
wants to merge 1 commit into from
Closed

Conversation

emhane
Copy link
Contributor

@emhane emhane commented Sep 14, 2023

Summary

remove gossipnet from relayer

Background

relayer has in the past submitted blocks to gossipnet for conductor to receive them earlier than celestia. conductor will receive blocks via RPC instead of p2p from one trusted connection to a sequencer node for the near future.

Changes

  • remove gossip net from relayer

Testing

removing mock conductor from relayer integration tests, this should not alter the way celestia interacts with tests

Breaking Changelist

  • relayer integration test slow_celestia_leads_to_bundled_blobs stops working though removing code that submits a block from gossip net doesn't effect the way a block is submitted to celestia. I am positive it has to do with the test framework, specifically how it interleaves time, which is out of scope of this pr to fix.

Related Issues

#332, #325

closes #333

@github-actions github-actions bot added the sequencer-relayer pertaining to the astria-sequencer-relayer crate label Sep 14, 2023
@emhane emhane temporarily deployed to BUF September 14, 2023 10:14 — with GitHub Actions Inactive
@SuperFluffy
Copy link
Member

I fixed the issue testing the batched celestia blobs.

You are right that it has to do with how time is handled in tokio tests: it auto-advances to the next tick, but that happens faster than the mock response + jsonrpc + deserialization, causing all sorts of issues.

@SuperFluffy
Copy link
Member

@emhane If you give permission I will push my patches fixing the blackbox tests to this branch so that CI passes (Monday or Tuesday).

@emhane
Copy link
Contributor Author

emhane commented Sep 18, 2023

I fixed the issue testing the batched celestia blobs.

You are right that it has to do with how time is handled in tokio tests: it auto-advances to the next tick, but that happens faster than the mock response + jsonrpc + deserialization, causing all sorts of issues.

if you advance time by the tick interval, naturally this happens. not quite sure how you mean the issue is how tokio handles time in tests? the time is controlled manually in these tests.

if you push the patch to a branch I can check it out. will add anything from here if it's not outdated after your changes #347 .

@SuperFluffy
Copy link
Member

@emhane see PR #384 which fixes the tests

Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

The config must be changed to reflect that writing to celestia is now the only functionality of sequencer-relayer.

Please fix the failing tests before merge. They are fixed in #384

SuperFluffy added a commit that referenced this pull request Sep 18, 2023
## Summary
Replace all implicit eyre errors by context specific typed errors

## Background
Same reasoning as for #375 as
part of the work for #332:

`sequencer-types` has to be made a dependency of `sequencer-client`,
which only uses typed errors. Therefore `sequencer-types` must have
typed errors (because otherwise `sequencer-client` would be "infected"
by eyre, which would require changing all of sequencer-client to also
use eyre.

## Changes
- Introduced one error variant per distinct eyre error.

## Testing
N/A. Nothing changed on the code level. Testing that the errors are
actually triggered could be done in a follow-up PR, but big parts of the
code will be replaced by the work in
#355 and follow-up PRs to it,
moving the types exchanged over RPCs to the astria-proto crate.

## Breaking Changelist
- No observable breaking changes in the astria services. Breaking
changes in the library crates because their public API changed (errors),
but we are not considering them right now

## Related Issues
#12
#86
#366
#332
@emhane
Copy link
Contributor Author

emhane commented Sep 18, 2023

@emhane see PR #384 which fixes the tests

your open pr implements the same issue. I implemented it because it was assigned to me, shame with duplicate work, so would have preferred if you would have just assigned yourself.

@emhane emhane closed this Sep 18, 2023
@SuperFluffy
Copy link
Member

SuperFluffy commented Sep 18, 2023

@emhane

@emhane see PR #384 which fixes the tests

your open pr implements the same issue. I implemented it because it was assigned to me, shame with duplicate work, so would have preferred if you would have just assigned yourself.

I needed to investigate why the tests were failing to merge this PR, which is the brunt of #384, and because you noted this:

I am positive it has to do with the test framework, specifically how it interleaves time, which is out of scope of this pr to fix.

@SuperFluffy SuperFluffy deleted the emhane/remove-gossipnet branch November 23, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sequencer-relayer pertaining to the astria-sequencer-relayer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove gossipnet from relayer
3 participants