-
Notifications
You must be signed in to change notification settings - Fork 799
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
Limit network actions to tracked chains (in main
)
#2393
Changes from all commits
e0ef37e
ff8565c
91dcdd0
df1a875
27d7b20
9ea30f6
16fe341
19aa4ed
0ae1b41
af3ecc9
3b6cae5
e9acb48
ecb1f71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ use std::{ | |
convert::Infallible, | ||
iter, | ||
ops::{Deref, DerefMut}, | ||
sync::Arc, | ||
sync::{Arc, RwLock}, | ||
}; | ||
|
||
use dashmap::{ | ||
|
@@ -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 | ||
tracked_chains: Arc<RwLock<HashSet<ChainId>>>, | ||
/// References to clients waiting for chain notifications. | ||
notifier: Arc<Notifier<Notification>>, | ||
/// A copy of the storage client so that we don't have to lock the local node client | ||
|
@@ -111,10 +114,16 @@ where | |
storage: S, | ||
max_pending_messages: usize, | ||
cross_chain_message_delivery: CrossChainMessageDelivery, | ||
tracked_chains: impl IntoIterator<Item = ChainId>, | ||
) -> Self { | ||
let state = WorkerState::new_for_client("Client node".to_string(), storage.clone()) | ||
.with_allow_inactive_chains(true) | ||
.with_allow_messages_from_deprecated_epochs(true); | ||
let tracked_chains = Arc::new(RwLock::new(tracked_chains.into_iter().collect())); | ||
let state = WorkerState::new_for_client( | ||
"Client node".to_string(), | ||
storage.clone(), | ||
tracked_chains.clone(), | ||
) | ||
.with_allow_inactive_chains(true) | ||
.with_allow_messages_from_deprecated_epochs(true); | ||
let local_node = LocalNodeClient::new(state); | ||
|
||
Self { | ||
|
@@ -124,6 +133,7 @@ where | |
max_pending_messages, | ||
message_policy: MessagePolicy::new(BlanketMessagePolicy::Accept, None), | ||
cross_chain_message_delivery, | ||
tracked_chains, | ||
notifier: Arc::new(Notifier::default()), | ||
storage, | ||
} | ||
|
@@ -141,6 +151,15 @@ where | |
&self.local_node | ||
} | ||
|
||
#[tracing::instrument(level = "trace", skip(self))] | ||
/// Adds a chain to the set of chains tracked by the local node. | ||
pub fn track_chain(&self, chain_id: ChainId) { | ||
self.tracked_chains | ||
.write() | ||
.expect("Panics should not happen while holding a lock to `tracked_chains`") | ||
.insert(chain_id); | ||
} | ||
|
||
#[tracing::instrument(level = "trace", skip_all, fields(chain_id, next_block_height))] | ||
/// Creates a new `ChainClient`. | ||
#[allow(clippy::too_many_arguments)] | ||
|
@@ -2501,6 +2520,12 @@ where | |
executed_block.message_id_for_operation(0, OPEN_CHAIN_MESSAGE_INDEX) | ||
}) | ||
.ok_or_else(|| ChainClientError::InternalError("Failed to create new chain"))?; | ||
// Add the new chain to the list of tracked chains | ||
self.client.track_chain(ChainId::child(message_id)); | ||
self.client | ||
.local_node | ||
.retry_pending_cross_chain_requests(self.chain_id) | ||
.await?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(found the answer I think) |
||
return Ok(ClientOutcome::Committed((message_id, certificate))); | ||
} | ||
} | ||
|
@@ -2791,6 +2816,16 @@ where | |
.await | ||
} | ||
|
||
#[tracing::instrument(level = "trace")] | ||
/// Handles any cross-chain requests for any pending outgoing messages. | ||
pub async fn retry_pending_outgoing_messages(&self) -> Result<(), ChainClientError> { | ||
self.client | ||
.local_node | ||
.retry_pending_cross_chain_requests(self.chain_id) | ||
.await?; | ||
Ok(()) | ||
} | ||
|
||
#[tracing::instrument(level = "trace", skip(from, limit))] | ||
pub async fn read_hashed_certificate_values_downward( | ||
&self, | ||
|
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.
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)