SYS-3965: added first stage migration of processed events mechanism to eth-bridge #390

MBrozhko34 · 2024-04-24T15:26:06Z

Proposed changes

Type of change/Merge

🚨What type of change is this PR?

Put an x in the boxes that apply

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR.

You describe the purpose of the PR, e.g.:
- What does it do?
- Highlight what important points reviewers should know about;
- Indicates if there is something left for follow-up PRs.
Documentation updated
Business logic tested successfully
Verify First, Write Last: In Substrate development, it is important that you always ensure preconditions are met and return errors at the beginning. After these checks have completed, then you may begin the function's computation.

Further comments

Introduces the data structures and the voting mechanism for accepting events ranges from ethereum. Ranges that are too big will be broken down into smaller partitions, voted upon individually, and accepted as one. Follow up commit will do the validate unsigned and processing of the partitions. Jira tickets: - SYS-3635 - SYS-3930

Updates the validate unsigned function to support submit_discovered_events extrinsic. Jira tickets: - SYS-3635 - SYS-3930

This PR introduces support for processing votes upon achieving consensus. It extends extrinsics and data structures accordingly. Once the final vote is cast, the voting round for the Ethereum range partition is finalized, and the partition is processed. Additionally, this PR extends the subscription mechanism of the eth-bridge pallet. By default, pallets ignore processed events, requiring the implementation of a handler to process them. Benchmarking for certain actions will be addressed in a separate PR. Jira Tasks: - SYS-3930 - SYS-3635

Signed-off-by: Ivan Cholakov <icholakov1@gmail.com> Co-authored-by: Thanos Doukoudakis <56822898+thadouk@users.noreply.github.com>

thadouk · 2024-04-29T13:03:13Z

pallets/eth-bridge/src/lib.rs

 }

- fn process_ethereum_event<T: Config>(event: &EthEvent) {
+ fn process_ethereum_event<T: Config>(event: &EthEvent) -> Result<(), DispatchError> {


i think we should consume any errors here, rather than bubbling up the error

thadouk · 2024-04-29T13:05:53Z

pallets/token-manager/src/mock.rs

@@ -376,6 +377,10 @@ impl ProcessedEventsChecker for TestRuntime {
 fn check_event(event_id: &EthEventId) -> bool {
 return PROCESSED_EVENTS.with(|l| l.borrow_mut().iter().any(|event| event == event_id))
 }
+
+ fn add_event(event_id: &EthEventId, processed: bool) -> DispatchResult {


do we need an overwrite of the default one?

It is not possible to implement only one of the trait items without the other, this change was to resolve errors occurring as a result of adding trait functions

thadouk · 2024-04-29T13:06:11Z

pallets/validators-manager/src/tests/mock.rs

@@ -350,6 +351,10 @@ impl ProcessedEventsChecker for TestRuntime {
 })
 })
 }
+
+ fn add_event(event_id: &EthEventId, processed: bool) -> DispatchResult {


Adds a new endpoint that allows the chain to reach consenus over the initial ethereum range to be used. This allows to simplify the voting logic for active ranges, eliminating the special case of the first range. Jira ticket: - SYS-3930

Extends the runtime API for initial range consensus mechanism Jira task - SYS-3930

Adds basic unit tests that check the submit_ethereum_events interface Jira task: - SYS-3930 - SYS-3635

…on-processed-events-mechanism-first

MBrozhko34 · 2024-05-02T17:05:42Z

@nahuseyoum I have written some comments for the tests. So far I had created the values in the mock (although partially incorrectly as the successful test case does not pass) and identified the edge cases. The tests themselves still need to reach the required function process_ethereum_event().

- General cleanup - Rename of storage items - Removal of mocks - Wire up with runtime, implementing different events filters for each runtime.

…' into SYS-3965-migration-processed-events-mechanism-first

commit 842e507 Merge: 5b1abf1 c483d18 Author: Thanos <56822898+thadouk@users.noreply.github.com> Date: Fri May 3 14:57:01 2024 +0100 Merge remote-tracking branch 'origin/feat/SYS-3560-add-event-listener' into SYS-3965-migration-processed-events-mechanism-first commit 5b1abf1 Author: Michael Brozhko <michael@brozhko.org> Date: Thu May 2 20:03:48 2024 +0300 fix merge issue, add more comments commit 0bf2b26 Author: Michael Brozhko <michael@brozhko.org> Date: Thu May 2 19:43:01 2024 +0300 tidy-up, add comments commit 787caa3 Merge: bc8a0fd 13b32f3 Author: Michael Brozhko <michael@brozhko.org> Date: Thu May 2 19:28:22 2024 +0300 Merge branch 'feat/SYS-3560-add-event-listener' into SYS-3965-migration-processed-events-mechanism-first commit bc8a0fd Author: Michael Brozhko <michael@brozhko.org> Date: Thu May 2 11:26:03 2024 +0300 tests, mock, more changes commit b535a33 Author: Michael Brozhko <michael@brozhko.org> Date: Wed Apr 24 16:23:01 2024 +0100 added functionality

…ion-processed-events-mechanism-first

… github.com:Aventus-Network-Services/avn-parachain into SYS-3965-migration-processed-events-mechanism-first

pallets/eth-bridge/src/lib.rs

nahuseyoum · 2024-05-15T14:36:10Z

pallets/eth-bridge/src/lib.rs

- });
- // Add record of succesful processing via ProcessedEventsChecker
- T::ProcessedEventsChecker::add_processed_event(&event.event_id.clone(), event_accepted);
+ <Pallet<T>>::deposit_event(Event::<T>::DuplicateEventSubmission {


I see why you are emitting an event here, I am assuming its because we don't want to error here which is correct. But in this particular case, the event we are processing has already been processed so Its "safe" to return an error and undo any storage changes related to processing this event.

pallets/eth-bridge/src/tests/mock.rs

thadouk · 2024-05-21T11:40:09Z

pallets/avn/src/lib.rs

@@ -703,13 +703,17 @@ impl<ValidatorId: Member> Enforcer<ValidatorId> for () {
 }

 pub trait ProcessedEventsChecker {
- fn check_event(event_id: &EthEventId) -> bool;
+ fn processed_event_exists(event_id: &EthEventId) -> bool;
+


Suggested change

pallets/eth-bridge/src/lib.rs

thadouk · 2024-05-21T11:47:59Z

pallets/eth-bridge/src/lib.rs

+ EventAccepted {
+ eth_event_id: EthEventId,
+ },
+ EventRejected {
+ eth_event_id: EthEventId,
+ },


why use two events for signaling the processing events?

Would it not be due to the fact that there are two possibilities in the event being accepted vs rejected and to distinguish between these two circumstances?

We have connected components that are currently looking for these events and its a lesser change to maintain the interface. Its also easier to search and filter instead of using event args.

I beg to differ. It is much more lean to maintain a single event with a flag.
Reduces codebase size and code repetition.
Its also outside the scope of the task to change this interface.

thadouk · 2024-05-21T11:54:35Z

pallets/eth-bridge/src/lib.rs

@@ -794,48 +806,62 @@ pub mod pallet {
 fn process_ethereum_events_partition<T: Config>(
 active_range: &ActiveEthRange,
 partition: &EthereumEventsPartition,
- ) {
+ ) -> Result<(), DispatchError> {


This function should be infallible.
Once a voting round is concluded, the processing of each event should not affect the processing of the batch.

The errors should be handled, the appropriate events emited and processing continues.
Otherwise, the chain will be stuck in a loop of voting for the last vote and rejecting the processing.

thadouk · 2024-05-21T11:56:58Z

pallets/eth-bridge/src/lib.rs

- // TODO before processing ensure that the event has not already been processed
- match T::BridgeInterfaceNotification::on_event_processed(&event) {
+ fn process_ethereum_event<T: Config>(event: &EthEvent) -> Result<(), DispatchError> {
+ ensure!(


we shouldn't error here. If the event has been processed, we avoid double processing, signal/log/emit the appropriate info and return.

If we call this function with an event that has already been processed, it's because the upstream functions did not validate properly. I understand we shouldn't error here if we need to mark the transaction as processed (and failed) but for a case where its already processed, why shouldn't we error?

if the authors/collators have reached consensus on the tier1 state then we shouldn't block it.
There are valid cases that this can occur:

events added and being processed while upgrading.

events added to tier2 due starting from an earlier last block
We should definitely mark the case, but not block the event processing mechanism.
The error will cause the last vote to be stuck and recasted, only to be rejected again and again in a loop.

In both of these scenarios, and possibly others, I would expect the collators to check first if the event is valid before submitting it (ex: already processed or 0x000..).
But in any case, this function is too low level to be required to be infallible. I think we can still throw an error here and catch it on process_ethereum_events_partition and log it there and move on to the next event?
It feels like making an distinction between a bad event (one that is already processed, or is 0
something we should add in the validation i guess 😄 ) and a valid event that fails to be processed makes sense which is what this low level function is doing.

I am happy with either solution, although perhaps handling the error within the process_ethereum_events_partition function is the best solution.

I have pushed a commit the changes @nahuseyoum suggested @thadouk . This can be reverted if necessary although I believe it should be ok

@nahuseyoum collators don't filter events based on their processing status, as this can change and create inconsistency.
The collators report the events found in a range blocks based on the type of the event that are determined by the active ethereum range.

thadouk · 2024-05-21T16:52:23Z

pallets/eth-bridge/src/lib.rs

- // TODO before processing ensure that the event has not already been processed
- match T::BridgeInterfaceNotification::on_event_processed(&event) {
+ fn process_ethereum_event<T: Config>(event: &EthEvent) -> Result<(), DispatchError> {
+ ensure!(


@nahuseyoum collators don't filter events based on their processing status, as this can change and create inconsistency.
The collators report the events found in a range blocks based on the type of the event that are determined by the active ethereum range.

thadouk · 2024-05-21T16:59:14Z

pallets/eth-bridge/src/lib.rs

+ EventAccepted {
+ eth_event_id: EthEventId,
+ },
+ EventRejected {
+ eth_event_id: EthEventId,
+ },


I beg to differ. It is much more lean to maintain a single event with a flag.
Reduces codebase size and code repetition.
Its also outside the scope of the task to change this interface.

pallets/eth-bridge/src/lib.rs

Co-authored-by: Thanos Doukoudakis <56822898+thadouk@users.noreply.github.com> Signed-off-by: Michael <62653655+MBrozhko34@users.noreply.github.com>

…o eth-bridge (#390) Co-authored-by: Thanos <56822898+thadouk@users.noreply.github.com> Co-authored-by: nahuseyoum <nahu.seyoum@aventus.io>

ivan-cholakov and others added 8 commits April 18, 2024 16:37

Sys 3931 identify events (#368)

76d87a2

SYS-3930: Extends validate unsigned for new extrinsic (#370)

668ec6b

Updates the validate unsigned function to support submit_discovered_events extrinsic. Jira tickets: - SYS-3635 - SYS-3930

feat: added runtime-api for eth-event polling (#376)

5d718a7

Signed-off-by: Ivan Cholakov <icholakov1@gmail.com> Co-authored-by: Thanos Doukoudakis <56822898+thadouk@users.noreply.github.com>

feat: added runtime-api for eth-event polling (#376)

ad9c0ff

Signed-off-by: Ivan Cholakov <icholakov1@gmail.com> Co-authored-by: Thanos Doukoudakis <56822898+thadouk@users.noreply.github.com>

SYS-3932 - implement initial version of polling mechanism (#378)

88f7000

added functionality

b535a33

MBrozhko34 requested review from ivan-cholakov, nahuseyoum and thadouk and removed request for ivan-cholakov, nahuseyoum and thadouk April 24, 2024 15:26

thadouk reviewed Apr 29, 2024

View reviewed changes

MBrozhko34 and others added 7 commits May 2, 2024 11:26

tests, mock, more changes

bc8a0fd

Exposes initial range voting via the runtime API (#393)

61cb434

Extends the runtime API for initial range consensus mechanism Jira task - SYS-3930

SYS-3930 Adds unit tests for event votes (#372)

13b32f3

Adds basic unit tests that check the submit_ethereum_events interface Jira task: - SYS-3930 - SYS-3635

Merge branch 'feat/SYS-3560-add-event-listener' into SYS-3965-migrati…

787caa3

…on-processed-events-mechanism-first

tidy-up, add comments

0bf2b26

fix merge issue, add more comments

5b1abf1

thadouk added 4 commits May 3, 2024 14:46

SYS-3930 Cleanup and finalises interface with runtime (#396)

c483d18

- General cleanup - Rename of storage items - Removal of mocks - Wire up with runtime, implementing different events filters for each runtime.

Merge remote-tracking branch 'origin/feat/SYS-3560-add-event-listener…

842e507

…' into SYS-3965-migration-processed-events-mechanism-first

Merge branch 'test-squash-thadouk-michaels-work' into SYS-3965-migrat…

e87e939

…ion-processed-events-mechanism-first

thadouk force-pushed the feat/SYS-3560-add-event-listener branch from c483d18 to 22ed41e Compare May 3, 2024 15:42

nahuseyoum added 2 commits May 3, 2024 16:47

rename event and update

e807d3c

Merge branch 'SYS-3965-migration-processed-events-mechanism-first' of…

0cc35cb

… github.com:Aventus-Network-Services/avn-parachain into SYS-3965-migration-processed-events-mechanism-first

update code and fix tests

c0c44f9

MBrozhko34 marked this pull request as ready for review May 13, 2024 14:44

MBrozhko34 requested review from nahuseyoum and fluorostani as code owners May 13, 2024 14:44

MBrozhko34 requested review from thadouk and ivan-cholakov May 13, 2024 14:45

ivan-cholakov approved these changes May 13, 2024

View reviewed changes

MBrozhko34 added 2 commits May 14, 2024 16:39

Change error handling in process events, fmt code

5b8b876

re-add accepted and rejected events

0b52c39

MBrozhko34 force-pushed the SYS-3965-migration-processed-events-mechanism-first branch from 10b59ba to 0b52c39 Compare May 15, 2024 09:04

nahuseyoum reviewed May 15, 2024

View reviewed changes

fix error handling

3a41efd

MBrozhko34 requested a review from nahuseyoum May 16, 2024 16:25

nahuseyoum approved these changes May 20, 2024

View reviewed changes

thadouk requested changes May 21, 2024

View reviewed changes

revert error handling, do not throw errors

e19db3b

MBrozhko34 force-pushed the SYS-3965-migration-processed-events-mechanism-first branch from 9be69d1 to e19db3b Compare May 21, 2024 14:58

MBrozhko34 added 2 commits May 21, 2024 16:26

revert again

4b74809

small tests fix

f527116

MBrozhko34 requested a review from thadouk May 21, 2024 15:29

thadouk approved these changes May 21, 2024

View reviewed changes

Update pallets/eth-bridge/src/lib.rs

66e0143

Co-authored-by: Thanos Doukoudakis <56822898+thadouk@users.noreply.github.com> Signed-off-by: Michael <62653655+MBrozhko34@users.noreply.github.com>

MBrozhko34 merged commit aad39b3 into feat/SYS-3560-add-event-listener May 22, 2024

MBrozhko34 deleted the SYS-3965-migration-processed-events-mechanism-first branch May 22, 2024 10:00

thadouk added a commit that referenced this pull request May 23, 2024

SYS-3965: added first stage migration of processed events mechanism t…

be64221

…o eth-bridge (#390) Co-authored-by: Thanos <56822898+thadouk@users.noreply.github.com> Co-authored-by: nahuseyoum <nahu.seyoum@aventus.io>

thadouk added a commit that referenced this pull request Jun 5, 2024

SYS-3965: added first stage migration of processed events mechanism t…

ae896c2

…o eth-bridge (#390) Co-authored-by: Thanos <56822898+thadouk@users.noreply.github.com> Co-authored-by: nahuseyoum <nahu.seyoum@aventus.io>

thadouk added a commit that referenced this pull request Jun 26, 2024

SYS-3965: added first stage migration of processed events mechanism t…

f09ab96

…o eth-bridge (#390) Co-authored-by: Thanos <56822898+thadouk@users.noreply.github.com> Co-authored-by: nahuseyoum <nahu.seyoum@aventus.io>

thadouk added a commit that referenced this pull request Jun 26, 2024

SYS-3965: added first stage migration of processed events mechanism t…

128ef19

…o eth-bridge (#390) Co-authored-by: Thanos <56822898+thadouk@users.noreply.github.com> Co-authored-by: nahuseyoum <nahu.seyoum@aventus.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SYS-3965: added first stage migration of processed events mechanism to eth-bridge #390

SYS-3965: added first stage migration of processed events mechanism to eth-bridge #390

MBrozhko34 commented Apr 24, 2024

thadouk Apr 29, 2024

thadouk Apr 29, 2024

MBrozhko34 Apr 29, 2024

thadouk Apr 29, 2024

MBrozhko34 commented May 2, 2024

nahuseyoum May 15, 2024

thadouk May 21, 2024

thadouk May 21, 2024

MBrozhko34 May 21, 2024

nahuseyoum May 21, 2024 •

edited

Loading

thadouk May 21, 2024

thadouk May 21, 2024

thadouk May 21, 2024

nahuseyoum May 21, 2024 •

edited

Loading

thadouk May 21, 2024

nahuseyoum May 21, 2024 •

edited

Loading

MBrozhko34 May 21, 2024

MBrozhko34 May 21, 2024

thadouk May 21, 2024

thadouk May 21, 2024

thadouk May 21, 2024

SYS-3965: added first stage migration of processed events mechanism to eth-bridge #390

SYS-3965: added first stage migration of processed events mechanism to eth-bridge #390

Conversation

MBrozhko34 commented Apr 24, 2024

Proposed changes

Type of change/Merge

Checklist

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBrozhko34 commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahuseyoum May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahuseyoum May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahuseyoum May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahuseyoum May 21, 2024 •

edited

Loading

nahuseyoum May 21, 2024 •

edited

Loading

nahuseyoum May 21, 2024 •

edited

Loading