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

Make all_messages_delivered_up_to take tracked_chains into account. #2562

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

afck
Copy link
Contributor

@afck afck commented Oct 2, 2024

Motivation

#2035 introduced a list of "tracked chains": In the local node, messages to non-tracked chains are not delivered and stay in the outbox.

However, all_messages_delivered_up_to doesn't take that into account yet, so if a chain is sending a message to a non-tracked chain, it would never return true, and if wait_for_outgoing_messages is true, handle_certificate could wait forever.

Proposal

Return true from all_messages_delivered_up_to if all messages to tracked chains have been delivered, ignoring remaining outbox entries for untracked chains.

TBD: This makes wait_for_outgoing_messages async and potentially much slower. We could instead add tracked_outbox_counters that count only messages to tracked chains. This would need to be updated when the set of tracked chains changes, however.

Test Plan

TBD: I should add a regression test for this.

Release Plan

  • These changes should be backported to the latest devnet branch, then
    • be released in a new SDK,
  • These changes should be backported to the latest testnet branch, then
    • be released in a new SDK,

Links

@afck afck requested review from ma2bd and jvff October 2, 2024 16:14
tracing::debug!(
"Messages left in {:.8}'s outbox: {:?}",
self.chain_id(),
self.outbox_counters.get()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead increment the counters right away for the messages to untracked chains?

Copy link
Contributor

Choose a reason for hiding this comment

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

or just what you said: tracked_outbox_counters
perhaps we can also have: untracked_outbox_counters to support future changes in the tracked chains

Copy link
Contributor Author

@afck afck Oct 3, 2024

Choose a reason for hiding this comment

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

Actually, maybe the outbox_counters just need to refer only to the tracked chains. When those change, the counters have to be recomputed from the outboxes themselves anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

…but then I wonder if we should move it from the (persisted) chain state view to the worker state?

Copy link
Contributor

Choose a reason for hiding this comment

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

This works for me!

Copy link
Contributor

Choose a reason for hiding this comment

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

(now that chain states are long lived)

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