feat(rln-relay): Keep track of the last processed event for membership changes

[rymnc]

Problem

To allow reconnections to the Ethereum node, we need to keep a track of the last event that was processed re: Membership changes. This allows the node to reconnect, and resume the tree sync from the last processed event to avoid inconsistencies in the tree root.

Suggested solution

The suggested solution would involve a cursor which is set back to 0 after each block is processed by the node. We already keep a track of the last block processed, which can then be used to resubscribe to the events starting from that block.

Alternatives considered

We could process blocks atomically.

Additional context

None

Acceptance criteria

• Maintain an event index cursor for each tree update

[staheri14]

Changed the state of this issue assuming it is in progress. If not, please move it back to the Backlog.

[staheri14]

The suggested solution would involve a cursor which is set back to 0 after each block is processed by the node.

I remember we agreed that cursor to be an always increasing value that keeps track of the index of the last inserted pk.
Why do we need to reset it back to 0 after processing each block?

[rymnc]

I remember we agreed that cursor to be an always increasing value that keeps track of the index of the last inserted pk. Why do we need to reset it back to 0 after processing each block?

That works too. It would not make a difference between the two. If you prefer it, I can use the index of the last inserted pk

[staheri14]

If there is no specific benefit regarding the per-block cursor, then a global cursor for the index of the last inserted pk is a more flexible solution (e.g., it is agnostic to how group synchronization works, and it would be also easily adaptable to any alternative solutions we want to use for the calculation of window of acceptable roots)

[s1fr0]

I'm not completely convinced about replacing the 0/1 cursor with the latest membership Index seen: that works well with registrations, but what about deletions? It seems like we need to store a value for each event we're listening to..

While a 0/1 flag that indicates if a block is fully processed or not is much more natural to me:

• a new block arrives and flag is set to 0 (block not processed)
• the tree is cloned,
• all block data is processed and the tree is updated,
• if no failure, the original tree is replaced and the flag is set to 1 (block processed)
• if any failure occurs, the original tree is restored and flag remains 0

Did I miss something?

[rymnc]

@s1fr0, with #1266, the roots will change on a per block basis. Although, I do understand your point for membership deletions. The event emitted on membership deletions looks to have the same structure as registrations - Maybe we could track registrations and deletions separately.
This logic comes handy in Step 2 of your solution, where the tree is cloned, and we lose connection to the ethereum node. The index allows us to resume tree changes to the cloned tree, instead of playing back the whole block. Although, playing back the whole block would have a minimal overhead, and would be the more correct way to have a consistent tree. wdyt @staheri14 @s1fr0 ?

[s1fr0]

I think what you say would have made more sense if processing each registration was a particularly expensive operation, hence saving some tree insertions was worth it.

However I don't think this is the case and we do not expect many registrations happening in a block (in the edge case of a block having exclusively membership registrations we cannot have more than ~600 in a block - registration costs ~51k Gas, block seems to have a limit of 30M Gas, but this ignores completely block size and other limits). From Zerokit tests, on an old laptop tree insertion takes roughly 900µs, hence 600 additions would take half a second.

Furthermore, we cannot download a block partially, so we have to go through its full content anyway.

I believe that processing all block's registrations/deletions at once is better: conceptually we only have one root change (and not intermediate ones in case of connection drops) and that's what we meant by allowing a windows of valid roots.

Zerokit exposes set_leaves API which allow to set tree leaves directly from an input buffer of commitments (but set is done starting from index 0, this was originally thought to restore the tree of a node): i can easily expose the starting index parameters so that multiple insertions can be done from the provided index. This allows to easily manage the case you mention as well, were you have available the latest index processed. Using such API is not strictly necessary (it's just a for loop over set_leaf) and depending how we deal with deletions as well, it may be simpler to collect all (commitment,index) in case of registrations and (0, index) in case of deletions and call set_leaf, regardless of the order they're processed.

[rymnc]

Agreed. Can we then deprecate this feature in favor of #1266 and process blocks atomically?

[staheri14]

@s1fr0 @rymnc
Both solutions are correct and they are not in conflict at all.

• Atomic processing of blocks (which is mentioned by @s1fr0 ) has already been planned out in these issues feat(rln-relay): Manage reconnections to the eth node  #1306 and feat(rln-relay): The window of acceptable roots should change per block, not per event  #1266 and are going to be implemented after this PR.
• Keeping track of the index of the last seen registered membership commitment is somewhat orthogonal to the atomic processing of blocks. It is more about state keeping for the waku rln relay node e.g., to check whether registration events arrive out of order or not (with a high probability they will be in order, but that is just a sanity check from our side). There are other future usecase to it, for instance, to implement a req/response protocol where waku nodes can query a portion of the tree from each other without actually having connections to any Eth client.

Does that address everyone's concerns?