-
Notifications
You must be signed in to change notification settings - Fork 248
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
Conversation
There was a problem hiding this 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.
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 | ||
}, | ||
) | ||
} |
There was a problem hiding this comment.
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<_>>()
There was a problem hiding this comment.
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
/// Returns all the open channels to other chains. | ||
fn open_channels() -> BTreeSet<(ChainId, ChannelId)>; |
There was a problem hiding this comment.
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:
/// 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)>; |
There was a problem hiding this comment.
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.
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 thedst_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:
tx_pool
of thedst_chain
and might end up submitting next set only to be rejected by thedst_chain's tx_pool
.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: