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

XDM: ensure relayer is only submitting XDMs that are within max nonce limit #3384

Merged
merged 3 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 4 additions & 0 deletions crates/subspace-fake-runtime-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,10 @@ sp_api::impl_runtime_apis! {
fn channel_storage_key(_chain_id: ChainId, _channel_id: ChannelId) -> Vec<u8> {
unreachable!()
}

fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
unreachable!()
}
}

impl sp_domains_fraud_proof::FraudProofApi<Block, DomainHeader> for Runtime {
Expand Down
4 changes: 4 additions & 0 deletions crates/subspace-runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1494,6 +1494,10 @@ impl_runtime_apis! {
fn channel_storage_key(chain_id: ChainId, channel_id: ChannelId) -> Vec<u8> {
Messenger::channel_storage_key(chain_id, channel_id)
}

fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
Messenger::open_channels()
}
}

impl sp_domains_fraud_proof::FraudProofApi<Block, DomainHeader> for Runtime {
Expand Down
36 changes: 28 additions & 8 deletions domains/client/relayer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ use parity_scale_codec::{Codec, Encode};
use sc_client_api::{AuxStore, HeaderBackend, ProofProvider, StorageProof};
use sc_utils::mpsc::TracingUnboundedSender;
use sp_api::{ApiRef, ProvideRuntimeApi};
use sp_core::H256;
use sp_core::{H256, U256};
use sp_domains::DomainsApi;
use sp_messenger::messages::{
BlockMessageWithStorageKey, BlockMessagesWithStorageKey, ChainId, CrossDomainMessage, Proof,
};
use sp_messenger::{MessengerApi, RelayerApi, XdmId};
use sp_messenger::{MessengerApi, RelayerApi, XdmId, MAX_FUTURE_ALLOWED_NONCES};
use sp_mmr_primitives::MmrApi;
use sp_runtime::traits::{Block as BlockT, CheckedSub, Header as HeaderT, NumberFor, One};
use sp_runtime::ArithmeticError;
Expand Down Expand Up @@ -314,13 +314,20 @@ where
// if this message should relay,
// check if the dst_chain inbox nonce is more than message nonce,
// if so, skip relaying since message is already executed on dst_chain
vedhavyas marked this conversation as resolved.
Show resolved Hide resolved
let relay_message = msg.nonce >= dst_channel_state.next_inbox_nonce;
let max_messages_nonce_allowed = dst_channel_state
.next_inbox_nonce
.saturating_add(MAX_FUTURE_ALLOWED_NONCES.into());
let relay_message = msg.nonce >= dst_channel_state.next_inbox_nonce
&& msg.nonce <= max_messages_nonce_allowed;
if !relay_message {
log::debug!(
target: LOG_TARGET,
"Skipping message relay from {:?} to {:?}",
"Skipping Outbox message relay from {:?} to {:?} of XDM[{:?}, {:?}]. Max Nonce allowed: {:?}",
msg.src_chain_id,
msg.dst_chain_id,
msg.channel_id,
msg.nonce,
max_messages_nonce_allowed
);
return false;
}
Expand Down Expand Up @@ -363,17 +370,30 @@ where
get_channel_state(backend, msg.dst_chain_id, msg.src_chain_id, msg.channel_id)
.ok()
.flatten()
&& let Some(dst_chain_outbox_response_nonce) =
dst_channel_state.latest_response_received_message_nonce
{
let next_nonce = if let Some(dst_chain_outbox_response_nonce) =
dst_channel_state.latest_response_received_message_nonce
{
// next nonce for outbox response will be +1 of current nonce for which
// dst_chain has received response.
dst_chain_outbox_response_nonce.saturating_add(U256::one())
} else {
// if dst_chain has not received the first outbox response,
// then next nonce will be just 0
U256::zero()
};
// relay inbox response if the dst_chain did not execute is already
let relay_message = msg.nonce > dst_chain_outbox_response_nonce;
let max_msg_nonce_allowed = next_nonce.saturating_add(MAX_FUTURE_ALLOWED_NONCES.into());
let relay_message = msg.nonce >= next_nonce && msg.nonce <= max_msg_nonce_allowed;
if !relay_message {
log::debug!(
target: LOG_TARGET,
"Skipping message relay from {:?} to {:?}",
"Skipping Inbox response message relay from {:?} to {:?} of XDM[{:?}, {:?}]. Max nonce allowed: {:?}",
msg.src_chain_id,
msg.dst_chain_id,
msg.channel_id,
msg.nonce,
max_msg_nonce_allowed
);
return false;
}
Expand Down
26 changes: 22 additions & 4 deletions domains/client/relayer/src/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ use crate::{BlockT, Error, GossipMessageSink, HeaderBackend, HeaderT, Relayer, L
use cross_domain_message_gossip::{ChannelUpdate, Message as GossipMessage, MessageData};
use futures::StreamExt;
use sc_client_api::{AuxStore, BlockchainEvents, ProofProvider};
use sp_api::ProvideRuntimeApi;
use sp_api::{ApiExt, ProvideRuntimeApi};
use sp_consensus::SyncOracle;
use sp_domains::{DomainId, DomainsApi};
use sp_messenger::messages::ChainId;
use sp_messenger::{MessengerApi, RelayerApi};
use sp_mmr_primitives::MmrApi;
use sp_runtime::traits::{CheckedSub, NumberFor, One};
use sp_runtime::traits::{CheckedSub, NumberFor, One, Zero};
use sp_runtime::SaturatedConversion;
use std::sync::Arc;

Expand Down Expand Up @@ -103,9 +103,27 @@ where
{
let api = client.runtime_api();

let updated_channels = api.updated_channels(block_hash)?;
let channels_status_to_broadcast = {
let updated_channels = api.updated_channels(block_hash)?;

// TODO: remove version check before next network
let relayer_api_version = api
.api_version::<dyn RelayerApi<Block, NumberFor<Block>, NumberFor<CBlock>, CBlock::Hash>>(block_hash)?
// It is safe to return a default version of 1, since there will always be version 1.
.unwrap_or(1);

// if there are no channel updates, broadcast channel's status for every 300 blocks
if updated_channels.is_empty()
&& relayer_api_version >= 2
&& block_number % 300u32.into() == Zero::zero()
{
api.open_channels(block_hash)?
} else {
updated_channels
}
};

for (dst_chain_id, channel_id) in updated_channels {
for (dst_chain_id, channel_id) in channels_status_to_broadcast {
let storage_key = api.channel_storage_key(block_hash, dst_chain_id, channel_id)?;
let proof = client
.read_proof(block_hash, &mut [storage_key.as_ref()].into_iter())
Expand Down
27 changes: 15 additions & 12 deletions domains/pallets/messenger/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,6 @@ use sp_messenger::messages::{
use sp_runtime::traits::{Extrinsic, Hash};
use sp_runtime::DispatchError;

/// Maximum number of XDMs per domain/channel with future nonces that are allowed to be validated.
/// Any XDM comes with a nonce above Maximum future nonce will be rejected.
// TODO: We need to benchmark how many XDMs can fit in to a
// - Single consensus block
// - Single domain block(includes all bundles filled with XDMs)
// Once we have that info, we can make a better judgement on how many XDMs
// we want to include per block while allowing other extrinsics to be included as well.
// Note: Currently XDM takes priority over other extrinsics unless they come with priority fee
const MAX_FUTURE_ALLOWED_NONCES: u32 = 256;

/// Transaction validity for a given validated XDM extrinsic.
/// If the extrinsic is not included in the bundle, extrinsic is removed from the TxPool.
const XDM_TRANSACTION_LONGEVITY: u64 = 10;
Expand Down Expand Up @@ -133,7 +123,7 @@ mod pallet {
use crate::{
BalanceOf, ChainAllowlistUpdate, Channel, ChannelId, ChannelState, CloseChannelBy,
FeeModel, HoldIdentifier, Nonce, OutboxMessageResult, StateRootOf, ValidatedRelayMessage,
MAX_FUTURE_ALLOWED_NONCES, STORAGE_VERSION, U256, XDM_TRANSACTION_LONGEVITY,
STORAGE_VERSION, U256, XDM_TRANSACTION_LONGEVITY,
};
#[cfg(not(feature = "std"))]
use alloc::boxed::Box;
Expand All @@ -158,7 +148,7 @@ mod pallet {
};
use sp_messenger::{
ChannelNonce, DomainRegistration, InherentError, InherentType, OnXDMRewards, StorageKeys,
INHERENT_IDENTIFIER,
INHERENT_IDENTIFIER, MAX_FUTURE_ALLOWED_NONCES,
};
use sp_runtime::traits::Zero;
use sp_runtime::{ArithmeticError, Perbill, Saturating};
Expand Down Expand Up @@ -1399,6 +1389,19 @@ mod pallet {
UpdatedChannels::<T>::get()
}

pub fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
Channels::<T>::iter().fold(
BTreeSet::new(),
|mut acc, (dst_chain_id, channel_id, channel)| {
if channel.state != ChannelState::Closed {
acc.insert((dst_chain_id, channel_id));
}

acc
},
)
}
Comment on lines +1392 to +1403
Copy link
Member

Choose a reason for hiding this comment

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

Optional: if we're concerned about performance with large numbers of open channels, this might make more efficient use of memory and CPU (or it might not, because it performs an extra collect::<Vec> and sort).

Channels::<T>::iter()
    .filter_map(|(dst_chain_id, channel_id, channel)| {
        if channel.state != ChannelState::Closed {
            Some((dst_chain_id, channel_id))
        } else {
            None
        }
    })
    .collect::<BTreeSet<_>>()

Copy link
Member Author

Choose a reason for hiding this comment

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

I would leave it as is


pub fn channel_nonce(chain_id: ChainId, channel_id: ChannelId) -> Option<ChannelNonce> {
Channels::<T>::get(chain_id, channel_id).map(|channel| {
let last_inbox_nonce = channel.next_inbox_nonce.checked_sub(U256::one());
Expand Down
14 changes: 14 additions & 0 deletions domains/primitives/messenger/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ use std::collections::BTreeSet;
/// Messenger inherent identifier.
pub const INHERENT_IDENTIFIER: InherentIdentifier = *b"messengr";

/// Maximum number of XDMs per domain/channel with future nonces that are allowed to be validated.
/// Any XDM comes with a nonce above Maximum future nonce will be rejected.
// TODO: We need to benchmark how many XDMs can fit in to a
// - Single consensus block
// - Single domain block(includes all bundles filled with XDMs)
// Once we have that info, we can make a better judgement on how many XDMs
// we want to include per block while allowing other extrinsics to be included as well.
// Note: Currently XDM takes priority over other extrinsics unless they come with priority fee
pub const MAX_FUTURE_ALLOWED_NONCES: u32 = 256;

/// Trait to handle XDM rewards.
pub trait OnXDMRewards<Balance> {
fn on_xdm_rewards(rewards: Balance);
Expand Down Expand Up @@ -188,6 +198,7 @@ pub struct ChannelNonce {

sp_api::decl_runtime_apis! {
/// Api useful for relayers to fetch messages and submit transactions.
#[api_version(2)]
pub trait RelayerApi<BlockNumber, CNumber, CHash>
where
BlockNumber: Encode + Decode,
Expand Down Expand Up @@ -219,6 +230,9 @@ sp_api::decl_runtime_apis! {

/// Returns storage key for channels for given chain and channel id.
fn channel_storage_key(chain_id: ChainId, channel_id: ChannelId) -> Vec<u8>;

/// Returns all the open channels to other chains.
fn open_channels() -> BTreeSet<(ChainId, ChannelId)>;
Comment on lines +234 to +235
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure when we're going to reset the API versions again, some of them might be around for a while after mainnet launch:

Suggested change
/// Returns all the open channels to other chains.
fn open_channels() -> BTreeSet<(ChainId, ChannelId)>;
/// Returns all the open channels to other chains.
/// Only in API versions 2 and later.
fn open_channels() -> BTreeSet<(ChainId, ChannelId)>;

Copy link
Member Author

Choose a reason for hiding this comment

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

Our code already handles the api version through checks, no reason to add it here.
If and when we reset the api versions, we have TODOs to remove the compatibility code as well.

}

/// Api to provide XDM extraction from Runtime Calls.
Expand Down
4 changes: 4 additions & 0 deletions domains/runtime/auto-id/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,10 @@ impl_runtime_apis! {
fn channel_storage_key(chain_id: ChainId, channel_id: ChannelId) -> Vec<u8> {
Messenger::channel_storage_key(chain_id, channel_id)
}

fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
Messenger::open_channels()
}
}

impl sp_domain_sudo::DomainSudoApi<Block> for Runtime {
Expand Down
4 changes: 4 additions & 0 deletions domains/runtime/evm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1418,6 +1418,10 @@ impl_runtime_apis! {
fn channel_storage_key(chain_id: ChainId, channel_id: ChannelId) -> Vec<u8> {
Messenger::channel_storage_key(chain_id, channel_id)
}

fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
Messenger::open_channels()
}
}

impl fp_rpc::EthereumRuntimeRPCApi<Block> for Runtime {
Expand Down
4 changes: 4 additions & 0 deletions domains/test/runtime/auto-id/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -954,6 +954,10 @@ impl_runtime_apis! {
fn channel_storage_key(chain_id: ChainId, channel_id: ChannelId) -> Vec<u8> {
Messenger::channel_storage_key(chain_id, channel_id)
}

fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
Messenger::open_channels()
}
}

impl sp_domain_sudo::DomainSudoApi<Block> for Runtime {
Expand Down
4 changes: 4 additions & 0 deletions domains/test/runtime/evm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1440,6 +1440,10 @@ impl_runtime_apis! {
fn channel_storage_key(chain_id: ChainId, channel_id: ChannelId) -> Vec<u8> {
Messenger::channel_storage_key(chain_id, channel_id)
}

fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
Messenger::open_channels()
}
}

impl fp_rpc::EthereumRuntimeRPCApi<Block> for Runtime {
Expand Down
4 changes: 4 additions & 0 deletions test/subspace-test-runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1547,6 +1547,10 @@ impl_runtime_apis! {
fn channel_storage_key(chain_id: ChainId, channel_id: ChannelId) -> Vec<u8> {
Messenger::channel_storage_key(chain_id, channel_id)
}

fn open_channels() -> BTreeSet<(ChainId, ChannelId)> {
Messenger::open_channels()
}
}

impl sp_domains_fraud_proof::FraudProofApi<Block, DomainHeader> for Runtime {
Expand Down
Loading