Skip to content

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

Closed
ParthDesai wants to merge 1 commit intoSnowfork:mainfrom
autonomys:add-ringbuffer
Closed

Bound finalized header, execution header and sync committee by RingBuffer and BoundedVec#814
ParthDesai wants to merge 1 commit intoSnowfork:mainfrom
autonomys:add-ringbuffer

Conversation

@ParthDesai
Copy link
Copy Markdown
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
Copy Markdown
Contributor

yrong commented Apr 26, 2023

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

#[pallet::storage]
pub(super) type FinalizedBeaconHeaderSlots<T: Config> =
StorageValue<_, BoundedVec<u64, T::MaxFinalizedHeaderSlotArray>, ValueQuery>;
StorageValue<_, BoundedVec<(u64, H256), T::MaxFinalizedHeaderSlotArray>, ValueQuery>;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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