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

Conversation

vedhavyas
Copy link
Member

We recently introduced changes to Messenger disallowing future XDMs beyond the defined nonce limit. But relayer still tries to broadcast all the XDMs that are confirmed only to be rejected the dst_chain's tx_pool if the future nonce limit is reached. This PR updates the relayer to broadcast XDMs that will be accepted by the dst_chain's tx_pool.

Relayer will filter the XDM messages using the channel state available in its Aux store. Channel state is broadcasted every time there is an update to channel. This should be sufficient most of the time. But there are cases where an operator recently joined the network and has been no XDM in the recent past. In this case, relayer of the new operator will not have channel state in their Aux store. To handle this scenario, for every 300 blocks, we broadcast all the channel's state.

Other considered approaches:

  • Relayer either keeping track of last broadcasted nonce and then submitting next set of XDMs based on the previous nonce. While this works, relayer do not have access to tx_pool of the dst_chain and might end up submitting next set only to be rejected by the dst_chain's tx_pool.
  • Relayer filtering the messages from ready to submit XDM after initial filtering of previously submitted XDMs and this has the same problem as above.

Overall, I prefer the currently implemented version since relayer can reliably predict the max nonce allowed based on the channel state which gets published as soon as there is update.
Only caveat I see could be the delay in channel state broadcast due to network

Closes: #3371

Code contributor checklist:

@vedhavyas vedhavyas added the audit-P3 Low audit priority label Feb 12, 2025
teor2345
teor2345 previously approved these changes Feb 13, 2025
Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, no blocking changes, feel free to resolve them straight away if you have other priorities.

domains/client/relayer/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +1392 to +1403
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
},
)
}
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

Comment on lines +234 to +235
/// Returns all the open channels to other chains.
fn open_channels() -> BTreeSet<(ChainId, ChannelId)>;
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.

@vedhavyas vedhavyas added this pull request to the merge queue Feb 13, 2025
Merged via the queue into main with commit d170f27 Feb 13, 2025
8 checks passed
@vedhavyas vedhavyas deleted the xdm_relayer_limit branch February 13, 2025 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-P3 Low audit priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't construct and relay all ready XDM at once
3 participants