SYS-4002: Fix primary validator for summary operations #385

micaelffrancoAV · 2024-04-19T13:43:14Z

Proposed changes

PR to change avn primary validator to be calculated based on the block number.

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

…or_summary_operations' into SYS-4002_Fix_primary_validator_for_summary_operations

nahuseyoum · 2024-04-19T14:56:52Z

pallets/avn/src/lib.rs

@@ -181,20 +175,14 @@ pub mod pallet {

 #[pallet::storage]
 #[pallet::getter(fn get_primary_collator)]
- pub type PrimaryCollator<T: Config> = StorageValue<_, PrimaryCollatorData, ValueQuery>;
+ pub type PrimaryCollator<T: Config> = StorageValue<_, u8, ValueQuery>;


Can we rename this to make it more explicit? Something like primaryCollatorIndexForEth? I also think we should use a large type for the value. u8 give 255 only and its possible we will end up sending more than that. Perhaps a u32?

nahuseyoum · 2024-04-19T15:42:17Z

pallets/ethereum-events/src/lib.rs

- let is_primary =
- AVN::<T>::is_primary(OperationType::Ethereum, &this_validator.account_id);
- if is_primary.is_err() {
+ let is_primary_validator_for_sending =


Suggested change

let is_primary_validator_for_sending =

let is_primary =

nahuseyoum · 2024-04-19T15:43:55Z

pallets/ethereum-events/src/lib.rs

@@ -836,7 +835,7 @@ pub mod pallet {
 return InvalidTransaction::Custom(ERROR_CODE_INVALID_EVENT_DATA).into()
 }

- if AVN::<T>::is_primary(OperationType::Ethereum, &result.checked_by)
+ if AVN::<T>::is_primary_validator_for_sending(&result.checked_by)


Suggested change

if AVN::<T>::is_primary_validator_for_sending(&result.checked_by)

if AVN::<T>::is_primary_for_block(&result.checked_by)

nahuseyoum · 2024-04-19T15:44:21Z

pallets/ethereum-events/src/mock.rs

 validator: &AccountId,
 ) -> Result<bool, avn_error<TestRuntime>> {
- return AVN::is_primary(op_type, validator)
+ return AVN::is_primary_validator_for_sending(validator)


Suggested change

return AVN::is_primary_validator_for_sending(validator)

return AVN::is_primary_for_block(validator)

nahuseyoum · 2024-04-19T15:44:38Z

pallets/ethereum-events/src/tests/tests.rs

@@ -190,7 +190,7 @@ fn test_is_primary_blocknumber_3() {
 ext.execute_with(|| {
 let block_number = 3;
 let expected_primary = account_id_1();
- let result = EthereumEvents::is_primary(OperationType::Ethereum, &expected_primary);
+ let result = EthereumEvents::is_primary_validator_for_sending(&expected_primary);


Suggested change

let result = EthereumEvents::is_primary_validator_for_sending(&expected_primary);

let result = EthereumEvents::is_primary_for_block(&expected_primary);

nahuseyoum · 2024-04-19T15:44:46Z

pallets/ethereum-events/src/tests/tests.rs

@@ -312,7 +312,7 @@ fn is_primary_fails_with_no_validators() {
 let mut ext = ExtBuilder::build_default().as_externality();
 ext.execute_with(|| {
 let block_number = 1;
- let result = EthereumEvents::is_primary(OperationType::Ethereum, &account_id_1());
+ let result = EthereumEvents::is_primary_validator_for_sending(&account_id_1());


Suggested change

let result = EthereumEvents::is_primary_validator_for_sending(&account_id_1());

let result = EthereumEvents::is_primary_for_block(&account_id_1());

nahuseyoum · 2024-04-19T15:47:14Z

pallets/summary/src/lib.rs

@@ -751,7 +751,7 @@ pub mod pallet {
 safe_add_block_numbers::<BlockNumberFor<T>>(Self::current_slot(), 1u32.into())
 .map_err(|_| Error::<T>::Overflow)?;

- let new_validator_account_id = AVN::<T>::advance_primary_validator(OperationType::Avn)?;
+ let new_validator_account_id = AVN::<T>::advance_primary_validator_for_sending()?;


I don't think we should be doing that here. The index is only incremented when sending I think.

nahuseyoum · 2024-04-19T15:47:36Z

pallets/summary/src/tests/tests_challenge.rs

@@ -152,19 +159,20 @@ mod challenge_slot_if_required {
 assert!(pool_state.read().transactions.is_empty());

 System::set_block_number(context.block_after_grace_period);
+ // let challenge = get_valid_challenge(&context);


Suggested change

// let challenge = get_valid_challenge(&context);

nahuseyoum · 2024-04-19T15:48:09Z

pallets/summary/src/tests/tests_vote.rs

@@ -897,7 +897,7 @@ mod end_voting_period {
 Summary::set_previous_summary_slot(5);

 let primary_validator_id =
- AVN::<TestRuntime>::advance_primary_validator(OperationType::Avn)
+ AVN::<TestRuntime>::advance_primary_validator_for_sending()


do we need to advance here? I think only sending should advance.

nahuseyoum · 2024-04-19T15:49:18Z

pallets/avn/src/tests/tests.rs

@@ -131,13 +58,13 @@ mod calling_is_primary_validator_for_ethereum {
 let mut ext = ExtBuilder::build_default().with_validators().as_externality();
 ext.execute_with(|| {
 let expected_primary = 1;
- let eth_index_before: u8 = get_index_based_on_operation_type(&OperationType::Ethereum);
+ let eth_index_before: u8 = AVN::get_primary_collator();


where is this defined?

Its the getter for the primary collator storage index.

nahuseyoum · 2024-04-19T15:49:41Z

pallets/eth-bridge/src/tx.rs

@@ -143,7 +141,7 @@ fn generate_msg_hash<T: pallet::Config>(
 }

 fn assign_sender<T: Config>() -> Result<T::AccountId, Error<T>> {
- match AVN::<T>::advance_primary_validator(OperationType::Ethereum) {
+ match AVN::<T>::advance_primary_validator_for_sending() {


nahuseyoum · 2024-04-22T12:37:12Z

Cargo.toml

@@ -12,7 +12,7 @@ lto = "fat"
 codegen-units = 1

 [workspace.package]
-version = "5.2.3"
+version = "5.2.4"


nahuseyoum · 2024-04-22T12:37:27Z

runtime/avn/src/lib.rs

@@ -174,7 +174,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 spec_name: create_runtime_str!("avn-parachain"),
 impl_name: create_runtime_str!("avn-parachain"),
 authoring_version: 1,
- spec_version: 62,
+ spec_version: 63,


Can we revert this change please?

nahuseyoum · 2024-04-22T12:37:29Z

runtime/test/src/lib.rs

@@ -169,7 +169,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 spec_name: create_runtime_str!("avn-test-parachain"),
 impl_name: create_runtime_str!("avn-test-parachain"),
 authoring_version: 1,
- spec_version: 62,
+ spec_version: 63,


and this too

SYS-4002: Fix primary validator for summary operations

5136f04

micaelffrancoAV requested review from ivan-cholakov, nahuseyoum, fluorostani, alexpArtos and thadouk April 19, 2024 13:43

aventus-ci-agent and others added 4 commits April 19, 2024 13:44

cargo package, spec and impl versions increased

ae820d8

SYS-4002: Test run time fix

ee28954

Merge remote-tracking branch 'origin/SYS-4002_Fix_primary_validator_f…

58521f1

…or_summary_operations' into SYS-4002_Fix_primary_validator_for_summary_operations

SYS-4002: Test run time fix

a792a63

nahuseyoum approved these changes Apr 19, 2024

View reviewed changes

micaelffrancoAV added 3 commits April 19, 2024 16:26

SYS-4002: Name fixes

9546b86

SYS-4002: More name fixes

d5a3021

SYS-4002: Remove unused operationType enum

cbcf2d5

nahuseyoum reviewed Apr 19, 2024

View reviewed changes

SYS-4002: PR comments

54d3d11

ivan-cholakov approved these changes Apr 22, 2024

View reviewed changes

SYS-4002: Fix tests

995f01a

nahuseyoum approved these changes Apr 22, 2024

View reviewed changes

micaelffrancoAV added 2 commits April 22, 2024 13:46

SYS-4002: Revert version

635876f

SYS-4002: Update cargo lock

670fc8e

micaelffrancoAV merged commit 95e9252 into main Apr 22, 2024
5 checks passed

micaelffrancoAV deleted the SYS-4002_Fix_primary_validator_for_summary_operations branch April 22, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SYS-4002: Fix primary validator for summary operations #385

SYS-4002: Fix primary validator for summary operations #385

micaelffrancoAV commented Apr 19, 2024 •

edited

Loading

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 19, 2024

micaelffrancoAV Apr 22, 2024

nahuseyoum Apr 19, 2024

nahuseyoum Apr 22, 2024

nahuseyoum Apr 22, 2024

nahuseyoum Apr 22, 2024

	if AVN::<T>::is_primary_validator_for_sending(&result.checked_by)
	if AVN::<T>::is_primary_for_block(&result.checked_by)

	return AVN::is_primary_validator_for_sending(validator)
	return AVN::is_primary_for_block(validator)

	let result = EthereumEvents::is_primary_validator_for_sending(&expected_primary);
	let result = EthereumEvents::is_primary_for_block(&expected_primary);

SYS-4002: Fix primary validator for summary operations #385

SYS-4002: Fix primary validator for summary operations #385

Conversation

micaelffrancoAV commented Apr 19, 2024 • edited Loading

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

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

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

micaelffrancoAV commented Apr 19, 2024 •

edited

Loading