Skip to content

Commit

Permalink
refactor: split pending result enum from final
Browse files Browse the repository at this point in the history
We shrink the job of the final Query::Result, and add a new
::PendingValidationResult which is used to indicate some content
that is waiting to be validated.

This includes a big simplification of validation logic by Milos
(morph-dev).
  • Loading branch information
carver committed Sep 19, 2024
1 parent 6d76e9f commit 74fb7ae
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 32 deletions.
25 changes: 16 additions & 9 deletions portalnet/src/find/iterators/findcontent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,11 @@ pub enum FindContentQueryResponse<TNodeId> {
ConnectionId(u16),
}

/// An intermediate value, waiting to be validated. After validation, the content will become a
/// FindContentQueryResult.
#[derive(Debug)]
pub enum FindContentQueryResult<TNodeId> {
NoneFound,
pub enum FindContentQueryPending<TNodeId> {
NonePending,
/// Content returned, but not yet validated.
PendingContent {
content: Vec<u8>,
Expand All @@ -64,6 +66,11 @@ pub enum FindContentQueryResult<TNodeId> {
// channel used to send back the validated content info
valid_content_tx: Sender<ValidatedContent<TNodeId>>,
},
}

#[derive(Debug)]
pub enum FindContentQueryResult<TNodeId> {
NoneFound,
/// Content returned, but not yet validated. Also includes a list of peers that were cancelled
ValidContent(ValidatedContent<TNodeId>, Vec<TNodeId>),
}
Expand Down Expand Up @@ -129,6 +136,7 @@ where
{
type Response = FindContentQueryResponse<TNodeId>;
type Result = FindContentQueryResult<TNodeId>;
type PendingValidationResult = FindContentQueryPending<TNodeId>;

fn target(&self) -> Key<TNodeId> {
self.target_key.clone()
Expand Down Expand Up @@ -348,15 +356,15 @@ where
}
}

fn pending_result(&self, source: TNodeId) -> Self::Result {
fn pending_validation_result(&self, source: TNodeId) -> Self::PendingValidationResult {
match self.unchecked_content.get(&source) {
Some(UnvalidatedContent::Content(content)) => FindContentQueryResult::PendingContent {
Some(UnvalidatedContent::Content(content)) => FindContentQueryPending::PendingContent {
content: content.clone(),
nodes_to_poke: self.get_nodes_to_poke(&source),
peer: source,
valid_content_tx: self.content_tx.clone(),
},
Some(UnvalidatedContent::Connection(connection_id)) => FindContentQueryResult::Utp {
Some(UnvalidatedContent::Connection(connection_id)) => FindContentQueryPending::Utp {
connection_id: *connection_id,
nodes_to_poke: self.get_nodes_to_poke(&source),
peer: source,
Expand All @@ -367,7 +375,7 @@ where
// not be called unless the caller believes there to be content
// waiting to be validated.
warn!("pending_result called, but no content is available for {source}");
FindContentQueryResult::NoneFound
FindContentQueryPending::NonePending
}
}
}
Expand Down Expand Up @@ -611,8 +619,8 @@ mod tests {
content_peer = Some(k.clone());
}
// Immediately validate the content
match query.pending_result(*peer_node_id) {
FindContentQueryResult::PendingContent {
match query.pending_validation_result(*peer_node_id) {
FindContentQueryPending::PendingContent {
content,
nodes_to_poke: _,
peer,
Expand Down Expand Up @@ -680,7 +688,6 @@ mod tests {
"Not all peers have been contacted: {uncontacted:?}"
);
}
_ => panic!("Unexpected result."),
}
}

Expand Down
3 changes: 2 additions & 1 deletion portalnet/src/find/iterators/findnodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ where
{
type Response = Vec<TNodeId>;
type Result = Vec<TNodeId>;
type PendingValidationResult = ();

fn target(&self) -> Key<TNodeId> {
self.target_key.clone()
Expand Down Expand Up @@ -265,7 +266,7 @@ where
.collect()
}

fn pending_result(&self, _source: TNodeId) -> Self::Result {
fn pending_validation_result(&self, _sending_peer: TNodeId) -> Self::PendingValidationResult {
// FindNodes queries don't need to provide access to the result until the query is finished.
// This is because the query doesn't run content validation, so as soon as the nodes are
// returned, the query is considered finished.
Expand Down
5 changes: 4 additions & 1 deletion portalnet/src/find/iterators/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ pub trait Query<TNodeId> {
/// The type of the result produced by the query.
type Result;

/// The type of a pending result that is waiting for validation.
type PendingValidationResult;

/// Returns the target of the query.
fn target(&self) -> Key<TNodeId>;

Expand Down Expand Up @@ -113,7 +116,7 @@ pub trait Query<TNodeId> {

/// Returns a result that is waiting for validation. The attached peer is used to look up which
/// pending result to return.
fn pending_result(&self, sending_peer: TNodeId) -> Self::Result;
fn pending_validation_result(&self, sending_peer: TNodeId) -> Self::PendingValidationResult;
}

/// Stage of the query.
Expand Down
33 changes: 12 additions & 21 deletions portalnet/src/overlay/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ use crate::{
find::{
iterators::{
findcontent::{
FindContentQuery, FindContentQueryResponse, FindContentQueryResult,
ValidatedContent,
FindContentQuery, FindContentQueryPending, FindContentQueryResponse,
FindContentQueryResult, ValidatedContent,
},
findnodes::FindNodeQuery,
query::{Query, QueryConfig},
Expand Down Expand Up @@ -506,7 +506,7 @@ where
};
// Generate the query result here instead of in handle_find_content_query_event,
// because the query is only borrowed, and can't be moved to the query event.
let query_result = query.pending_result(sending_peer);
let query_result = query.pending_validation_result(sending_peer);
Poll::Ready(QueryEvent::Validating(content_key, query_result))
}
// Note that some query pools skip over Validating, straight to Finished (like the FindNode query pool)
Expand Down Expand Up @@ -652,14 +652,11 @@ where
}
QueryEvent::Validating(content_key, query_result) => {
match query_result {
FindContentQueryResult::NoneFound => {
FindContentQueryPending::NonePending => {
// This should be an unreachable path
error!("A FindContent query claimed to have some new data to validate, but none was available");
}
FindContentQueryResult::ValidContent(..) => {
panic!("A FindContent query's pending_result() must never return already validated content");
}
FindContentQueryResult::PendingContent {
FindContentQueryPending::PendingContent {
content,
nodes_to_poke,
peer,
Expand All @@ -679,7 +676,7 @@ where
.await;
});
}
FindContentQueryResult::Utp {
FindContentQueryPending::Utp {
connection_id,
peer,
nodes_to_poke,
Expand Down Expand Up @@ -776,13 +773,6 @@ where
}));
}
}
FindContentQueryResult::PendingContent { .. }
| FindContentQueryResult::Utp { .. } => {
// If no content gets verified, then the query must return NoneFound
unimplemented!(
"Unverified content should never be returned by the final query result"
);
}
}
}
}
Expand Down Expand Up @@ -2645,6 +2635,7 @@ where
}
}
}

/// The result of the `query_event_poll` indicating an action is required to further progress an
/// active query.
pub enum QueryEvent<TQuery: Query<NodeId>, TContentKey> {
Expand All @@ -2653,7 +2644,7 @@ pub enum QueryEvent<TQuery: Query<NodeId>, TContentKey> {
/// The query has timed out, possible returning peers.
TimedOut(QueryId, QueryInfo<TContentKey>, TQuery),
/// The query is working to validate the response.
Validating(TContentKey, TQuery::Result),
Validating(TContentKey, TQuery::PendingValidationResult),
/// The query has completed successfully.
Finished(QueryId, QueryInfo<TContentKey>, TQuery),
}
Expand Down Expand Up @@ -3945,8 +3936,8 @@ mod tests {
.expect("Query pool does not contain query");

// Query result should contain content.
match query.pending_result(bootnode_node_id) {
FindContentQueryResult::PendingContent {
match query.pending_validation_result(bootnode_node_id) {
FindContentQueryPending::PendingContent {
content: pending_content,
peer,
..
Expand Down Expand Up @@ -4015,8 +4006,8 @@ mod tests {
.expect("Query pool does not contain query");

// Query result should contain connection ID
match query.pending_result(bootnode_node_id) {
FindContentQueryResult::Utp {
match query.pending_validation_result(bootnode_node_id) {
FindContentQueryPending::Utp {
connection_id,
peer,
..
Expand Down

0 comments on commit 74fb7ae

Please sign in to comment.