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

[yrong]

Based on #814 from @ParthDesai
Companion for cumulus: Snowfork/cumulus#20

Fixes: SNO-328

[vgeddes]

Looks good! The implementation seems sound. Will give a more thorough review next week.

[vgeddes]

@yrong can you review #816, and if you agree it is good, then we can rebase this PR on top of it?

[vgeddes]

This is a good design ring buffer design, but I think we can do even better to optimize the overhead of the implementation.

It seems the mappings ExecutionHeaderMapping and SyncCommitteesMapping simply contain pointers to next inserted node.

So lets consider storing the next pointers with the actual nodes.

See this rough example which explains it better:

struct RingBufferNode<K, V> {
    value: V,
    next: Option<K>,
}

struct RingBufferCursor<K> {
    // last inserted node
    node: K,
    // number of nodes in ring buffer
    count: u32
}

struct RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;

impl<K, V, Nodes, Cursor> RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;
{
    fn insert(key: K, value: V) {
        // handle all cases here, much like a circular singly linked list    
    }
}

// Ring buffer nodes
#[pallet::storage]
pub(super) type SyncCommittees<T: Config> = StorageMap<_, Identity, H256, RingBufferNode<H256, SyncCommitteeOf<T>>, ValueQuery>;

// Ring buffer cursor
#[pallet::storage]
pub(crate) type SyncCommitteesCursor<T: Config> = StorageValue<_, RingBufferCursor<H256>, ValueQuery>;

With this design, we reduce the number of storage writes from 3 to 2, so uses less weight.

Let me know what you think.

[vgeddes]

This is a good design ring buffer design, but I think we can do even better to optimize the overhead of the implementation.

It seems the mappings ExecutionHeaderMapping and SyncCommitteesMapping simply contain pointers to next inserted node.

So lets consider storing the next pointers with the actual nodes.

See this rough example which explains it better:

struct RingBufferNode<K, V> {
    value: V,
    next: Option<K>,
}

struct RingBufferCursor<K> {
    // last inserted node
    node: K,
    // number of nodes in ring buffer
    count: u32
}

struct RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;

impl<K, V, Nodes, Cursor> RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;
{
    fn insert(key: K, value: V) {
        // handle all cases here, much like a circular singly linked list    
    }
}

// Ring buffer nodes
#[pallet::storage]
pub(super) type SyncCommittees<T: Config> = StorageMap<_, Identity, H256, RingBufferNode<H256, SyncCommitteeOf<T>>, ValueQuery>;

// Ring buffer cursor
#[pallet::storage]
pub(crate) type SyncCommitteesCursor<T: Config> = StorageValue<_, RingBufferCursor<H256>, ValueQuery>;

With this design, we reduce the number of storage writes from 3 to 2, so uses less weight.

Let me know what you think.

Actually, just realised this idea will have worse performance, since every time we need to update the node.next pointers, we will end up writing sync committees to storage redundantly. Ignore this idea

[ParthDesai]

This is a good design ring buffer design, but I think we can do even better to optimize the overhead of the implementation.

It seems the mappings ExecutionHeaderMapping and SyncCommitteesMapping simply contain pointers to next inserted node.

So lets consider storing the next pointers with the actual nodes.

See this rough example which explains it better:

struct RingBufferNode<K, V> {
    value: V,
    next: Option<K>,
}

struct RingBufferCursor<K> {
    // last inserted node
    node: K,
    // number of nodes in ring buffer
    count: u32
}

struct RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;

impl<K, V, Nodes, Cursor> RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;
{
    fn insert(key: K, value: V) {
        // handle all cases here, much like a circular singly linked list    
    }
}

// Ring buffer nodes
#[pallet::storage]
pub(super) type SyncCommittees<T: Config> = StorageMap<_, Identity, H256, RingBufferNode<H256, SyncCommitteeOf<T>>, ValueQuery>;

// Ring buffer cursor
#[pallet::storage]
pub(crate) type SyncCommitteesCursor<T: Config> = StorageValue<_, RingBufferCursor<H256>, ValueQuery>;

With this design, we reduce the number of storage writes from 3 to 2, so uses less weight.

Let me know what you think.

This is a good design ring buffer design, but I think we can do even better to optimize the overhead of the implementation.
It seems the mappings ExecutionHeaderMapping and SyncCommitteesMapping simply contain pointers to next inserted node.
So lets consider storing the next pointers with the actual nodes.
See this rough example which explains it better:

struct RingBufferNode<K, V> {
    value: V,
    next: Option<K>,
}

struct RingBufferCursor<K> {
    // last inserted node
    node: K,
    // number of nodes in ring buffer
    count: u32
}

struct RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;

impl<K, V, Nodes, Cursor> RingBufferTransient<K, V, Nodes, Cursor> 
where
    Nodes: StorageMap<K, RingBufferNode<K, V>>,
    Cursor: StorageValue<RingBufferCursor<K>>;
{
    fn insert(key: K, value: V) {
        // handle all cases here, much like a circular singly linked list    
    }
}

// Ring buffer nodes
#[pallet::storage]
pub(super) type SyncCommittees<T: Config> = StorageMap<_, Identity, H256, RingBufferNode<H256, SyncCommitteeOf<T>>, ValueQuery>;

// Ring buffer cursor
#[pallet::storage]
pub(crate) type SyncCommitteesCursor<T: Config> = StorageValue<_, RingBufferCursor<H256>, ValueQuery>;

With this design, we reduce the number of storage writes from 3 to 2, so uses less weight.
Let me know what you think.

Actually, just realised this idea will have worse performance, since every time we need to update the node.next pointers, we will end up writing sync committees to storage redundantly. Ignore this idea

Hmm. Make sense. One other concern is implemeting RingBuffer should be internal detail of the pallet. If we expose the structure via Storage, client can make assumptions on that which might not be good idea as if we modify the RingBuffer implementation later on then we will have to do storage migration.

[ParthDesai]

Overall make sense to me. Though, some unrelated changes are from the main, which I haven't reviewed as I do not have enough context for those.

[ParthDesai]

@yrong Is it possible to merge this PR now?

[yrong]

@yrong Is it possible to merge this PR now?

Yes, almost done from my side but will defer to @vgeddes if any other concern.

[vgeddes]

@ParthDesai Note, you need at least rust 1.70 (nightly) to successfully compile the light client in all modes that are required (debug, release, etc).

Older versions of Rust struggle: paritytech/substrate#14075