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

feat: allow awaiting payload in progress #11823

Merged
merged 7 commits into from
Oct 18, 2024
Merged

Conversation

klkvr
Copy link
Collaborator

@klkvr klkvr commented Oct 16, 2024

Closes #11716

Added wait_for_pending argument to resolve methods which allows to configure whether returned future should wait for a payload job in progress.

Added resolve method and removed unused new_payload and send_and_resolve_payload methods from PayloadBuilder trait.

@github-actions github-actions bot added A-block-building Related to block building C-enhancement New feature or request labels Oct 16, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I'd like to introduce an enum that represents the requested payload kind, this way could easier change this in the future and an enum is generally better for this than a bool arg

crates/engine/local/src/miner.rs Outdated Show resolved Hide resolved
crates/payload/primitives/src/traits.rs Outdated Show resolved Hide resolved
Comment on lines 59 to 62
fn resolve(
&mut self,
wait_for_pending: bool,
) -> (Self::ResolvePayloadFuture, KeepPayloadJobAlive);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer an enum instead of the bool that represents the targeted payload kind

@klkvr klkvr requested a review from mattsse October 17, 2024 11:07
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

a few more suggestions

Comment on lines 346 to 349
pub enum PayloadKind {
/// Returns the best available payload, without waiting for pending job to finish.
#[default]
BestAvailable,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this isn't quite right, from CL pov we want a payload as fast as possible, so where this would be relevant is if there's no best payload yet we start racing the empty payload against the in progress payload:

/// If no payload has been built so far, it will either return an empty payload or the result of the
/// in progress build job, whatever finishes first.

so this mode is more like Earliest

Comment on lines 60 to 61
&mut self,
wait_for_pending: bool,
) -> (Self::ResolvePayloadFuture, KeepPayloadJobAlive);
fn resolve(&mut self, kind: PayloadKind) -> (Self::ResolvePayloadFuture, KeepPayloadJobAlive);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to keep this non breaking because this is currently used by users.

we should instead introduce a new function that accepts the kind and resolve calls this with Kind::Earliest

Comment on lines 48 to 52
pub async fn resolve(
&self,
id: PayloadId,
wait_for_pending: bool,
kind: PayloadKind,
) -> Option<Result<T::BuiltPayload, PayloadBuilderError>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same pattern here, basically just as:

/// Adds a peer to the known peer set, with the given kind.
fn add_peer_kind(
&self,
peer: PeerId,
kind: PeerKind,
tcp_addr: SocketAddr,
udp_addr: Option<SocketAddr>,
);

/// Adds a trusted peer to the peer set with TCP `SocketAddr`.
fn add_trusted_peer(&self, peer: PeerId, tcp_addr: SocketAddr) {
self.add_peer_kind(peer, PeerKind::Trusted, tcp_addr, None);
}

/// Adds a peer to the peer set with TCP `SocketAddr`.
fn add_peer(&self, peer: PeerId, tcp_addr: SocketAddr) {
self.add_peer_kind(peer, PeerKind::Static, tcp_addr, None);
}

Comment on lines 534 to 539
let fut = ResolveBestPayload {
best_payload,
maybe_better,
empty_payload,
wait_for_pending: kind == PayloadKind::WaitForPending,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

here we don't need to pass the kind as argument instead we can control it based on whether we set the empty_payload
if Kind.is_earliest then we can keep this as none so the ResolveBestPayload will only race the two real payloads against each other

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

rn if payload job is still in progress ResolveBestPayload would always return the best_payload, so we need to somehow make it poll the pending one until completeion I think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

imo this is fine, because you still want to return as fast as possible and if we already have one we should probably return that asap, the pending could even be the same or is not guaranteed to be better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated, though I think there's an edge case when pending job fails leaving us with nothing to return

but given this is only used in --dev mode for now I think it should be OK

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse force-pushed the klkvr/wait-for-pending-payload branch from d5e5d95 to 67ed467 Compare October 18, 2024 10:31
@mattsse mattsse added this pull request to the merge queue Oct 18, 2024
Merged via the queue into main with commit 8d32fd7 Oct 18, 2024
39 checks passed
@mattsse mattsse deleted the klkvr/wait-for-pending-payload branch October 18, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-block-building Related to block building C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce trait for ResolvePayloadFuture: Future
2 participants