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

Implement packet clearing in relayer next #3488

Merged

Conversation

ljoss17
Copy link
Contributor

@ljoss17 ljoss17 commented Jul 18, 2023

Closes: #XXX

Description


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@ljoss17 ljoss17 marked this pull request as ready for review July 24, 2023 07:44
@ljoss17 ljoss17 requested a review from soareschen July 31, 2023 08:10
@@ -0,0 +1,3 @@
pub mod impls;
pub mod traits;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can move most of this module to relayer-components. The only extra part here is the worker, which requires the runtime to have spawner function, which introduces non-determinism.

height: Qualified::SmallerEqual(QueryHeight::Specific(*height)),
};
let mut events = vec![];
for sequence in sequences.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be rather inefficient, as one query is made per sequence. I think we should be able to use query_send_packet_events from crates/relayer/src/link/packet_events.rs to get all events in one query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand query_send_packet_events from crates/relayer/src/link/packet_events.rs does the following:

  1. Build the QueryPacketEventDataRequest and call query_packet_events
  2. If the height is Qualified::Equal query the packets from block (not currently done in the this implementation for relayer-next)
  3. If the height is Qualified::SmallerEqual query the packets from TXs
  4. query_packet_from_txs calls tx_search(packet_query(..)) for each sequence number, similar to what is being done in this implementation
    1. With the addition of mapping the ABCI events to IbcEventWithHeight by calling packet_from_tx_search_response.

I don't see how this is more efficient, and the issue is that the cosmos implementation uses ABCI Events as its Event type so try_extract_send_packet_event expects an ABCI Event and not a IbcEventWithHeight.

And query_packet_events_with from crates/relayer/src/link/packet_events.rs is called with query_send_packet_events and query_write_ack_events as query_fn, so it also ends up calling query_packet_events.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm you are right. Looks like the algorithm is ultimately based on query_packets_from_txs in crates/relayer/src/chain/cosmos/query/tx.rs. Maybe we should note that down and add a todo for investigating more efficient approach.

One more nit for this: it should be possible to use the Stream construct with .map() to query for all events in parallel and then collect them into a vec.

Copy link
Contributor

@soareschen soareschen Aug 3, 2023

Choose a reason for hiding this comment

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

Probably a more structured way to define this is to first define a singular CanQuerySendPacketsFromSequence trait (without -s), and then implement CanQuerySendPacketsFromSequences by using Stream::map and calling the singular method for each sequence.

.iter()
.filter_map(<CosmosChain<Chain> as OfaChain>::try_extract_send_packet_event)
.map(|event| {
<CosmosChain<Chain> as OfaChain>::extract_packet_from_send_packet_event(&event)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes sense to move the concrete implementation for try_extract_write_acknowledgement_event and try_extract_send_packet_event into methods/ so that we can call it directly as plain function.


let (unreceived_packet_sequences, height): (Vec<Sequence>, Height) =
CanQueryUnreceivedPacketSequences::query_unreceived_packet_sequences(
relay_context.relay_a_to_b().src_chain(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can call relay_context.relay_a_to_b().src_chain().query_unreceived_packet_sequences() directly.

Since relay_context.relay_a_to_b() is called repeatedly, we can simplify it by assigning it a variable like let relay = birelay.relay_a_to_b();. Similarly we can assign let chain_a = relay.src_chain();.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e0b5727

{
/// Given a list of sequences, a channel and port will query a list of outgoing
/// packets which have not been relayed.
async fn query_unreceived_packets(
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the method should also change to query_send_packets_from_sequences. Also, let's move this trait to a separate file like send_packets.rs.

@soareschen soareschen merged commit 13bc2c8 into soares/relayer-next Aug 4, 2023
51 checks passed
@soareschen soareschen deleted the luca_joss/relayer-next-clear-packet-worker branch August 4, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants