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

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

Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
76d87a2
Sys 3931 identify events (#368)
ivan-cholakov Mar 20, 2024
22252f3
SYS-3635: extends the voting mechanism
thadouk Mar 14, 2024
668ec6b
SYS-3930: Extends validate unsigned for new extrinsic (#370)
thadouk Mar 21, 2024
31f33bf
SYS-3930: Implement Vote Processing (#371)
thadouk Apr 9, 2024
5d718a7
feat: added runtime-api for eth-event polling (#376)
ivan-cholakov Apr 10, 2024
ad9c0ff
feat: added runtime-api for eth-event polling (#376)
ivan-cholakov Apr 10, 2024
88f7000
SYS-3932 - implement initial version of polling mechanism (#378)
ivan-cholakov Apr 16, 2024
b535a33
added functionality
MBrozhko34 Apr 24, 2024
bc8a0fd
tests, mock, more changes
MBrozhko34 May 2, 2024
9af749d
Introduces initial range consesnus mechanism (#391)
thadouk May 2, 2024
61cb434
Exposes initial range voting via the runtime API (#393)
thadouk May 2, 2024
13b32f3
SYS-3930 Adds unit tests for event votes (#372)
thadouk May 2, 2024
787caa3
Merge branch 'feat/SYS-3560-add-event-listener' into SYS-3965-migrati…
MBrozhko34 May 2, 2024
0bf2b26
tidy-up, add comments
MBrozhko34 May 2, 2024
5b1abf1
fix merge issue, add more comments
MBrozhko34 May 2, 2024
c483d18
SYS-3930 Cleanup and finalises interface with runtime (#396)
thadouk May 3, 2024
842e507
Merge remote-tracking branch 'origin/feat/SYS-3560-add-event-listener…
thadouk May 3, 2024
41933c7
Squashed commit of the following:
thadouk May 3, 2024
e87e939
Merge branch 'test-squash-thadouk-michaels-work' into SYS-3965-migrat…
thadouk May 3, 2024
e807d3c
rename event and update
nahuseyoum May 3, 2024
0cc35cb
Merge branch 'SYS-3965-migration-processed-events-mechanism-first' of…
nahuseyoum May 3, 2024
c0c44f9
update code and fix tests
nahuseyoum May 13, 2024
5b8b876
Change error handling in process events, fmt code
MBrozhko34 May 14, 2024
0b52c39
re-add accepted and rejected events
MBrozhko34 May 15, 2024
3a41efd
fix error handling
MBrozhko34 May 16, 2024
e19db3b
revert error handling, do not throw errors
MBrozhko34 May 21, 2024
4b74809
revert again
MBrozhko34 May 21, 2024
f527116
small tests fix
MBrozhko34 May 21, 2024
66e0143
Update pallets/eth-bridge/src/lib.rs
MBrozhko34 May 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions pallets/avn/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

fn add_processed_event(event_id: &EthEventId, accepted: bool);
}

impl ProcessedEventsChecker for () {
fn check_event(_event_id: &EthEventId) -> bool {
return false
fn processed_event_exists(_event_id: &EthEventId) -> bool {
false
}

fn add_processed_event(_event_id: &EthEventId, _accepted: bool) {}
}

pub trait OnGrowthLiftedHandler<Balance> {
Expand Down Expand Up @@ -749,7 +753,7 @@ pub trait BridgeInterfaceNotification {
fn process_lower_proof_result(_: u32, _: Vec<u8>, _: Result<Vec<u8>, ()>) -> DispatchResult {
Ok(())
}
fn on_event_processed(_event: &EthEvent) -> DispatchResult {
fn on_incoming_event_processed(_event: &EthEvent) -> DispatchResult {
Ok(())
}
}
Expand All @@ -770,8 +774,8 @@ impl BridgeInterfaceNotification for Tuple {
Ok(())
}

fn on_event_processed(_event: &EthEvent) -> DispatchResult {
for_tuples!( #( Tuple::on_event_processed(_event)?; )* );
fn on_incoming_event_processed(_event: &EthEvent) -> DispatchResult {
for_tuples!( #( Tuple::on_incoming_event_processed(_event)?; )* );
Ok(())
}
}
Expand Down
64 changes: 45 additions & 19 deletions pallets/eth-bridge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ use frame_system::{
};
use pallet_avn::{
self as avn, BridgeInterface, BridgeInterfaceNotification, Error as avn_error, LowerParams,
MAX_VALIDATOR_ACCOUNTS,
ProcessedEventsChecker, MAX_VALIDATOR_ACCOUNTS,
};

use pallet_session::historical::IdentificationTuple;
Expand Down Expand Up @@ -103,6 +103,9 @@ mod benchmarking;
#[path = "tests/event_listener_tests.rs"]
mod event_listener_tests;
#[cfg(test)]
#[path = "tests/incoming_events_tests.rs"]
mod incoming_events_tests;
#[cfg(test)]
#[path = "tests/lower_proof_tests.rs"]
mod lower_proof_tests;
#[cfg(test)]
Expand Down Expand Up @@ -183,6 +186,7 @@ pub mod pallet {
IdentificationTuple<Self>,
CorroborationOffence<IdentificationTuple<Self>>,
>;
type ProcessedEventsChecker: ProcessedEventsChecker;
type EthereumEventsFilter: EthereumEventsFilterTrait;
}

Expand Down Expand Up @@ -219,9 +223,13 @@ pub mod pallet {
BoundedVec<(BoundedVec<u8, TypeLimit>, BoundedVec<u8, ValueLimit>), ParamsLimit>,
caller_id: BoundedVec<u8, CallerIdLimit>,
},
/// EventProcessed(bool, EthEventId)
EventProcessed {
accepted: bool,
EventAccepted {
eth_event_id: EthEventId,
},
EventRejected {
eth_event_id: EthEventId,
},
Comment on lines +226 to +231
Copy link
Contributor

Choose a reason for hiding this comment

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

why use two events for signaling the processing events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@nahuseyoum nahuseyoum May 21, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

DuplicateEventSubmission {
MBrozhko34 marked this conversation as resolved.
Show resolved Hide resolved
eth_event_id: EthEventId,
},
}
Expand Down Expand Up @@ -296,13 +304,16 @@ pub mod pallet {
pub enum Error<T> {
CorroborateCallFailed,
DuplicateConfirmation,
DuplicateEventSubmission,
MBrozhko34 marked this conversation as resolved.
Show resolved Hide resolved
EmptyFunctionName,
ErrorAssigningSender,
EthTxHashAlreadySet,
EthTxHashMustBeSetBySender,
ExceedsConfirmationLimit,
ExceedsCorroborationLimit,
ExceedsFunctionNameLimit,
EventAlreadyProcessed,
EventNotProcessed,
FunctionEncodingError,
FunctionNameError,
HandlePublishingResultFailed,
Expand Down Expand Up @@ -565,12 +576,13 @@ pub mod pallet {
);

let mut votes = EthereumEvents::<T>::get(&events_partition);

votes.try_insert(author.account_id).map_err(|_| Error::<T>::EventVotesFull)?;

if votes.len() < AVN::<T>::quorum() as usize {
EthereumEvents::<T>::insert(&events_partition, votes);
} else {
process_ethereum_events_partition::<T>(&active_range, &events_partition);
process_ethereum_events_partition::<T>(&active_range, &events_partition)?;
advance_partition::<T>(&active_range, &events_partition);
}

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

Choose a reason for hiding this comment

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

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.

// Remove entry from storage. Ignore votes.
let _ = EthereumEvents::<T>::take(partition);
for discovered_event in partition.events().iter() {
match ValidEvents::try_from(&discovered_event.event.event_id.signature) {
Some(valid_event) =>
if active_range.event_types_filter.contains(&valid_event) {
process_ethereum_event::<T>(&discovered_event.event);
process_ethereum_event::<T>(&discovered_event.event)?;
} else {
log::warn!("Ethereum event signature ({:?}) included in approved range ({:?}), but not part of the expected ones {:?}", &discovered_event.event.event_id.signature, active_range.range, active_range.event_types_filter);
},
None => log::warn!(
"Unknown Ethereum event signature in range {:?}",
&discovered_event.event.event_id.signature
),
};
None => {
log::warn!(
"Unknown Ethereum event signature in range {:?}",
&discovered_event.event.event_id.signature
);
},
}
}

// Cleanup
for (partition, votes) in EthereumEvents::<T>::drain() {
// TODO raise offences
log::info!("Collators with invalid votes on ethereum events (range: {:?}, partition: {}): {:?}", partition.range(), partition.partition(), votes);
}

Ok(())
}

fn process_ethereum_event<T: Config>(event: &EthEvent) {
// 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!(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nahuseyoum nahuseyoum May 21, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. events added and being processed while upgrading.
  2. 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.

Copy link
Contributor

@nahuseyoum nahuseyoum May 21, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

false == T::ProcessedEventsChecker::processed_event_exists(&event.event_id.clone()),
Error::<T>::EventAlreadyProcessed
);

let mut event_accepted = false;

match T::BridgeInterfaceNotification::on_incoming_event_processed(&event) {
Ok(_) => {
<Pallet<T>>::deposit_event(Event::<T>::EventProcessed {
accepted: true,
event_accepted = true;
<Pallet<T>>::deposit_event(Event::<T>::EventAccepted {
eth_event_id: event.event_id.clone(),
});
},
Err(err) => {
log::error!("💔 Processing ethereum event failed: {:?}", err);
<Pallet<T>>::deposit_event(Event::<T>::EventProcessed {
accepted: false,
<Pallet<T>>::deposit_event(Event::<T>::EventRejected {
eth_event_id: event.event_id.clone(),
});
},
};

// Add record of succesful processing via ProcessedEventsChecker
T::ProcessedEventsChecker::add_processed_event(&event.event_id.clone(), event_accepted);

Ok(())
}

#[pallet::validate_unsigned]
Expand Down
138 changes: 138 additions & 0 deletions pallets/eth-bridge/src/tests/incoming_events_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// Copyright 2023 Aventus Network Systems (UK) Ltd.

#![cfg(test)]
use crate::{eth::generate_send_calldata, mock::*, request::*, *};
use frame_support::{
assert_err, assert_noop, assert_ok, dispatch::DispatchResultWithPostInfo, error::BadOrigin,
};
use sp_runtime::{testing::UintAuthorityId, DispatchError};
pub extern crate alloc;
use alloc::collections::BTreeSet;
use sp_avn_common::event_discovery::EthBridgeEventsFilter;

const ROOT_HASH: &str = "30b83f0d722d1d4308ab4660a72dbaf0a7392d5674eca3cd21d57256d42df7a0";
const REWARDS: &[u8] = b"15043665996000000000";
const AVG_STAKED: &[u8] = b"9034532443555111110000";
const PERIOD: &[u8] = b"3";
const T2_PUB_KEY: &str = "14aeac90dbd3573458f9e029eb2de122ee94f2f0bc5ee4b6c6c5839894f1a547";
const T1_PUB_KEY: &str = "23d79f6492dddecb436333a5e7a4cfcc969f568e01283fa2964aae15327fb8a3b685a4d0f3ef9b3c2adb20f681dbc74b7f82c1cf8438d37f2c10e9c79591e9ea";

// Added this function as in event_listener_tests to initialize the active event range
fn init_active_range() {
ActiveEthereumRange::<TestRuntime>::put(ActiveEthRange {
range: EthBlockRange { start_block: 1, length: 1000 },
partition: 0,
event_types_filter: EthBridgeEventsFilter::try_from(
vec![
ValidEvents::AddedValidator,
ValidEvents::Lifted,
ValidEvents::AvtGrowthLifted,
ValidEvents::AvtLowerClaimed,
]
.into_iter()
.collect::<BTreeSet<ValidEvents>>(),
)
.unwrap(),
});
}

mod process_events {
use super::*;
use sp_avn_common::event_types::EthEventId;

// succesfully process the specified ethereum_event
#[test]
fn succesful_event_processing_accepted() {
let mut ext = ExtBuilder::build_default().with_validators().as_externality();
ext.execute_with(|| {
let context = setup_context();
init_active_range();

// Two calls needed as upon the first there are not enough votes to pass the condition
// in lib.rs line 563, to reach the call of process_ethereum_events_partition()
assert_ok!(EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author.clone(),
context.mock_event_partition.clone(),
context.test_signature.clone()
));
assert_ok!(EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author_two.clone(),
context.mock_event_partition.clone(),
context.test_signature_two.clone()
));

assert!(System::events().iter().any(|record| record.event ==
mock::RuntimeEvent::EthBridge(Event::<TestRuntime>::EventAccepted {
eth_event_id: context.eth_event_id.clone(),
})));
});
}

// This test should fail processing the ethereum_event and emit the specified event
#[test]
fn succesful_event_processing_not_accepted() {
let mut ext = ExtBuilder::build_default().with_validators().as_externality();
ext.execute_with(|| {
let context = setup_context();
init_active_range();
assert_ok!(EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author.clone(),
context.bad_mock_event_partition.clone(),
context.test_signature.clone()
));
assert_ok!(EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author_two.clone(),
context.bad_mock_event_partition.clone(),
context.test_signature.clone()
));

assert!(System::events().iter().any(|record| record.event ==
mock::RuntimeEvent::EthBridge(Event::<TestRuntime>::EventRejected {
eth_event_id: context.bad_eth_event_id.clone(),
})));
});
}

// This test should fail on the check
// T::ProcessedEventsChecker::processed_event_exists(&event.event_id.clone()), if the event is
// already in the system
#[test]
fn event_already_processed() {
let mut ext = ExtBuilder::build_default().with_validators().as_externality();
ext.execute_with(|| {
let context = setup_context();
init_active_range();
assert_ok!(EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author.clone(),
context.mock_event_partition.clone(),
context.test_signature.clone()
));
assert_ok!(EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author_two.clone(),
context.mock_event_partition.clone(),
context.test_signature_two.clone()
));
assert_ok!(EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author.clone(),
context.second_mock_event_partition.clone(),
context.test_signature.clone()
));
assert_noop!(
EthBridge::submit_ethereum_events(
RuntimeOrigin::none(),
context.author_two.clone(),
context.second_mock_event_partition.clone(),
context.test_signature_two.clone()
),
Error::<TestRuntime>::EventAlreadyProcessed
);
});
}
}
Loading