Limit network actions to tracked chains (in main)
#2393
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afck
approved these changes
Aug 21, 2024
| self.client | ||
| .local_node | ||
| .retry_pending_cross_chain_requests(self.chain_id) | ||
| .await?; |
There was a problem hiding this comment.
I'm not even sure we always want that. E.g. the faucet probably shouldn't track all its children. Maybe it's better to default to not doing that?
There was a problem hiding this comment.
Summary of what we discussed offline: for most users it makes sense to track child chains they create. For the Faucet it's not great, but not tracking them would increase the size of its outboxes, which could be less scalable.
There was a problem hiding this comment.
Just to be sure: By "tracking" here, do you mean subscribing to notifications or just allowing a local chain state to exist?
(found the answer I think)
jvff
force-pushed
the
limit-network-actions-to-tracked-chains-in-testnet-abel
branch
2 times, most recently
from
5d83e58 to
f1f8c3e
Compare
passes for me on this branch. ✔️ |
jvff
changed the title
testnet_abel)main)
jvff
force-pushed
the
limit-network-actions-to-tracked-chains-in-testnet-abel
branch
from
f1f8c3e to
d1d8128
Compare
afck
approved these changes
Aug 27, 2024
jvff
force-pushed
the
limit-network-actions-to-tracked-chains-in-testnet-abel
branch
from
640108c to
354f2bb
Compare
Prepare to only create network actions for the set of tracked chains.
Share it with the created chain worker actors.
Keep track of the chains that client is interested in.
Specify which chains should be tracked by a new `Client`.
Configure the worker based on the client's selection.
Avoid handling chains that aren't interesting to the client.
Allow resending messages intended for chains that weren't tracked when the outgoing message was scheduled, but became tracked later.
Allow adding more chains to the initial set of tracked chains.
Ensure that chains that the client open are tracked.
So that the worker can properly handle it.
Ensure that they are properly executed during the benchmark.
Remember to replace the quick-fix with a more comprehensive refactor.
jvff
force-pushed
the
limit-network-actions-to-tracked-chains-in-testnet-abel
branch
from
354f2bb to
dac2387
Compare
Check all executed blocks for messages that open new chains, and add the new chain IDs to the set of tracked chains.
jvff
force-pushed
the
limit-network-actions-to-tracked-chains-in-testnet-abel
branch
from
dac2387 to
ecb1f71
Compare
ma2bd
reviewed
Aug 28, 2024
| @@ -91,6 +91,9 @@ where | |||
| message_policy: MessagePolicy, | |||
| /// Whether to block on cross-chain message delivery. | |||
| cross_chain_message_delivery: CrossChainMessageDelivery, | |||
| /// Chains that should be tracked by the client. | |||
| // TODO(#2412): Merge with set of chains the client is receiving notifications from validators | |||
There was a problem hiding this comment.
Ok that answers my question. We actually don't want that for the faucet, so perhaps we need two notions of "tracked" (replication allowed <> subscription)
jvff
deleted the
limit-network-actions-to-tracked-chains-in-testnet-abel
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This PR is a port of #2035 to the
mainbranch.Currently, processing a chain on the client creates network actions for all other chains it sends messages to. This leads to cross-chain requests that create chain states on storage for those other chains. These other chains might not be relevant to the client, which means it wastes storage and hinders performance.
As a real world example, the faucet chain is used to create new chains. Every block creates a new chain. That means that when a user uses the wallet to create a chain using the faucet, it will keep in storage the initial chain states for all chains created by the faucet before it.
Proposal
Add an optional
tracked_chainsfield toChainWorkerState, and only createNetworkActionsfor those tracked chains. The set of tracked chains is updated by theClient.Test Plan
@afck, @Twey, could you please check to see if this fixes the issue you ran into?
Release Plan
This changes internal client behavior, so:
Links