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.

Sign up for GitHub

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

Jump to bottom

[bridges] Prune messages from confirmation tx body, not from the on_idle #4944

Merged
merged 5 commits into from
Jul 10, 2024
Merged

[bridges] Prune messages from confirmation tx body, not from the on_idle #4944

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4fbda94
prune messages from confirmation tx, not from the on_idle (#2211)
bkontur Jul 4, 2024
28bdcf0
Added `do_try_state_for_outbound_lanes` which checks unpruned messages
bkontur Jul 4, 2024
667641e
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
Jul 4, 2024
7aa55e2
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
Jul 4, 2024
fb8d0ba
Aviod iteration all the time to the beginning
bkontur Jul 5, 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: 7 additions & 9 deletions bridges/modules/messages/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,18 +640,16 @@ pub mod pallet {

// collect message_nonces that were supposed to be pruned
let mut unpruned_message_nonces = Vec::new();
let mut nonce_to_check = expected_last_prunned_nonce;
loop {
const MAX_MESSAGES_ITERATION: u64 = 16;
let start_nonce =
expected_last_prunned_nonce.checked_sub(MAX_MESSAGES_ITERATION).unwrap_or(0);
for current_nonce in start_nonce..=expected_last_prunned_nonce {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with try_state(). Why do we use MAX_MESSAGES_ITERATION and not iterate all the way ?

Otherwise replacing the loop wit ha for looks good.

Copy link
Contributor Author

@bkontur bkontur Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not familiar with try_state(). Why do we use MAX_MESSAGES_ITERATION and not iterate all the way ?

Because, on every runtime upgrade it would be more and more waste of time, e.g.

  • first time it would run from e.g. message with nonce 1000 to 0
  • another runtime upgrade (when more messages happen) it would run e.g. 3000 to 0 (we previously checked interval 1000 to 0)
  • another runtime upgrade (when more messages happen) it would run e.g. 6000 to 0 (we previously checked interval 3000 to 0)
  • and so on

Yes, that's why I updated description that maybe oldest_unpruned_nonce and do_try_state_for_outbound_lanes are not really needed.

Slava wanted to create migration, that remove all messages < oldest_unpruned_nonce, but I think his concert was valid only if we have already more than one lane, because on_idle he picked different/random lanes to prune depending on ActiveOutboundLanes::count, which is for fellows 1 :). I think MBM migration is too much for this and also for standard migration, we would need to handle consumed weights and store last pruned block somewhere else, because oldest_unpruned_nonce is incremented on delivery now.

But as I am thinking about it again now, I think we should really remove attribute lane.oldest_unpruned_nonce, because we prune on delivery and this attribute does not make sense. So, the removal of this attribute could cause storage version change and migration, which would clean everything :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, MBM migration and removed oldest_unpruned_nonce, would do the job :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't use oldest_unpruned_nonce for relayer, anyway we can leave also without these migration, the worst can happen that we will have just a few messages in the storage, which we don't need. We add monitoring or do some sanity checks with seaching storage manually after upgrade, and when few messages left, we could remove them with set_prefix / kill_storage.

// check a message for current_nonce
if OutboundMessages::<T, I>::contains_key(MessageKey {
lane_id,
nonce: nonce_to_check,
nonce: current_nonce,
}) {
unpruned_message_nonces.push(nonce_to_check);
}
if let Some(new_nonce_to_check) = nonce_to_check.checked_sub(One::one()) {
nonce_to_check = new_nonce_to_check;
} else {
break;
unpruned_message_nonces.push(current_nonce);
}
}

Expand Down
34 changes: 32 additions & 2 deletions bridges/modules/messages/src/tests/pallet_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ use bp_runtime::{BasicOperatingMode, PreComputedSize, RangeInclusiveExt, Size};
use bp_test_utils::generate_owned_bridge_module_tests;
use codec::Encode;
use frame_support::{
assert_noop, assert_ok,
assert_err, assert_noop, assert_ok,
dispatch::Pays,
storage::generator::{StorageMap, StorageValue},
weights::Weight,
};
use frame_system::{EventRecord, Pallet as System, Phase};
use sp_core::Get;
use sp_runtime::DispatchError;
use sp_runtime::{BoundedVec, DispatchError};

fn get_ready_for_events() {
System::<TestRuntime>::set_block_number(1);
Expand Down Expand Up @@ -983,3 +983,33 @@ fn maybe_outbound_lanes_count_returns_correct_value() {
Some(mock::ActiveOutboundLanes::get().len() as u32)
);
}

#[test]
fn do_try_state_for_outbound_lanes_works() {
run_test(|| {
let lane_id = TEST_LANE_ID;

// setup delivered nonce 1
OutboundLanes::<TestRuntime>::insert(
lane_id,
OutboundLaneData {
oldest_unpruned_nonce: 2,
latest_received_nonce: 1,
latest_generated_nonce: 0,
},
);
// store message for nonce 1
OutboundMessages::<TestRuntime>::insert(
MessageKey { lane_id, nonce: 1 },
BoundedVec::default(),
);
assert_err!(
Pallet::<TestRuntime>::do_try_state(),
sp_runtime::TryRuntimeError::Other("Found unpruned lanes!")
);

// remove message for nonce 1
OutboundMessages::<TestRuntime>::remove(MessageKey { lane_id, nonce: 1 });
assert_ok!(Pallet::<TestRuntime>::do_try_state());
})
}
Loading