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

Bound finalized header, execution header and sync committee by RingBuffer and BoundedVec #814

Closed
wants to merge 1 commit into from

Conversation

ParthDesai
Copy link
Contributor

@ParthDesai ParthDesai commented Apr 24, 2023

Description

This PR is improvement over #810 as suggested by @yrong. Instead of using Pruning, now we are using modified version of RingBuffer to limit the runtime storage.

How RingBuffer works?

If number of items go beyond some threshold, the RingBuffer::insert will remove oldest entry.

Advantage of RingBuffer

There is no while loop. The complexity is O(1). And since it is abstracted, there is no clutter in application logic.

@yrong
Copy link
Contributor

yrong commented Apr 26, 2023

@ParthDesai Awesome! Many thanks and I'll review it together with team.

@@ -165,7 +175,7 @@ pub mod pallet {

#[pallet::storage]
pub(super) type FinalizedBeaconHeaderSlots<T: Config> =
StorageValue<_, BoundedVec<u64, T::MaxFinalizedHeaderSlotArray>, ValueQuery>;
StorageValue<_, BoundedVec<(u64, H256), T::MaxFinalizedHeaderSlotArray>, ValueQuery>;
Copy link
Contributor

@yrong yrong Apr 27, 2023

Choose a reason for hiding this comment

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

Just reminder, also require change in relayer side here

@yrong
Copy link
Contributor

yrong commented Apr 27, 2023

LGTM! Definitely a more elegant way for the pruing. Cost is some auxiliary storage and more r/w operations introduced so more weight will be consumed. Anyway, it's some O(1) operation so should be fine and would be nice if some benchmark accordingly.

@yrong
Copy link
Contributor

yrong commented Apr 27, 2023

required Snowfork/cumulus#20

benchmark in comparison shows weight consumed increased by less than 1% can be safely ignored.

@yrong
Copy link
Contributor

yrong commented Apr 27, 2023

@ParthDesai There is some more changes required to run e2e stack, if you do not mind I'll draft this one and we can review #815 instead.

@ParthDesai
Copy link
Contributor Author

@ParthDesai There is some more changes required to run e2e stack, if you do not mind I'll draft this one and we can review #815 instead.

No problem. If you need any help with that, please let me know.

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 this pull request may close these issues.

2 participants