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

PVF: drop backing jobs if it is too late #5616

Merged
merged 123 commits into from
Nov 7, 2024
Merged

Conversation

AndreiEres
Copy link
Contributor

@AndreiEres AndreiEres commented Sep 6, 2024

Fixes #5530

This PR introduces the removal of backing jobs that have been back pressured for longer than allowedAncestryLen, as these candidates are no longer viable.

It is reasonable to expect a result for a backing job execution within allowedAncestryLen blocks. Therefore, we set the job TTL as a relay block number and synchronize the validation host by sending activated leaves.

polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/backing/src/lib.rs Outdated Show resolved Hide resolved
polkadot/node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
polkadot/node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀

Comment on lines +331 to +335
if in_active_fork {
None
} else {
Some(index)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could directly filter on the queue, so if in active fork we send ValidationError::ExecutionDeadline and return true otherwise false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't send the result inside the closure, that why I didn't use retain

Copy link
Contributor

Choose a reason for hiding this comment

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

why not? the send function is not async and we ignore the result

polkadot/node/core/pvf/src/error.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Nice! Just a couple of suggestions

polkadot/node/core/pvf/src/execute/queue.rs Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/execute/queue.rs Outdated Show resolved Hide resolved
@AndreiEres AndreiEres added this pull request to the merge queue Nov 7, 2024
Merged via the queue into master with commit 6c8a347 Nov 7, 2024
194 of 197 checks passed
@AndreiEres AndreiEres deleted the AndreiEres/issue5530 branch November 7, 2024 13:46
paritytech-ci pushed a commit that referenced this pull request Nov 7, 2024
Fixes #5530

This PR introduces the removal of backing jobs that have been back
pressured for longer than `allowedAncestryLen`, as these candidates are
no longer viable.

It is reasonable to expect a result for a backing job execution within
`allowedAncestryLen` blocks. Therefore, we set the job TTL as a relay
block number and synchronize the validation host by sending activated
leaves.

---------

Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T0-node This PR/Issue is related to the topic “node”. T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
Development

Successfully merging this pull request may close these issues.

PVF: drop backing jobs if it is too late
9 participants