Skip to content

Feat/offchain aggregation #232

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

Merged
merged 19 commits into from
May 15, 2025
Merged

Feat/offchain aggregation #232

merged 19 commits into from
May 15, 2025

Conversation

driemworks
Copy link
Contributor

@driemworks driemworks commented May 9, 2025

This PR solves two issues encountered while running as a parachain collator.

It partially addresses #211
It close #224
It closes #231
It closes #230

  • Previously, nodes would take pulses from local storage instead of reading. While this minimizes local storage reqs, it causes issues when chain re-orgs happen (and a collator has to rebuild a block) where historic pulses are missing. This PR simplifies storage as a FIFO queue.
  • As a consequence of this simplification, we still need to filter the set of pulses before they are aggregated and encoded on-chain. Within the create_inherent hook, we now filter and aggregate pulses.
  • This also removes the custom block import wrappers. Since offchain pulse storage is just a bounded collection, we don't need to know the 'latest round' when pruning storage, so we no longer require a custom block import wrapper.
  • It removes custom pulse filtering logic and associated structs/types & docs

@driemworks driemworks self-assigned this May 9, 2025
Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

The solution looks good, I think that we should still have a Pulse object or something similar that we can control with a trait

@driemworks driemworks marked this pull request as ready for review May 13, 2025 21:43
Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

I have updated the main comment as it closes #230, it does not fully close #229 as there are some comments and docs related to the filtering so I didn't add it to the comment too.

The PR looks good, a lot of simplifications that I love. The thing that worries me the most is the most is the change from 2s to 4s block time

@juangirini
Copy link
Contributor

@driemworks I have also added this small PR #241 for updating the dispatch_weight fn

@driemworks driemworks merged commit ccbbdea into main May 15, 2025
5 checks passed
@driemworks driemworks deleted the feat/offchain-aggregation branch May 15, 2025 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants