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

runtime: pipelined preparation for incoming and delayed receipts #11904

Conversation

nagisa
Copy link
Collaborator

@nagisa nagisa commented Aug 7, 2024

Part of #11319 and the final change in integration with the transaction runtime as all interesting receipt types are handled now. There are also receipt types like yield timeouts which only result in generation of new (delayed) receipts, so they don't need to be handled by this mechanism.

@nagisa nagisa requested a review from a team as a code owner August 7, 2024 13:30
@nagisa nagisa requested review from wacban and Ekleog-NEAR and removed request for wacban August 7, 2024 13:30
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 96.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.45%. Comparing base (4facc16) to head (1d43cac).
Report is 11 commits behind head on master.

Files Patch % Lines
runtime/runtime/src/lib.rs 95.08% 3 Missing ⚠️
runtime/runtime/src/pipelining.rs 93.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11904      +/-   ##
==========================================
- Coverage   71.47%   71.45%   -0.03%     
==========================================
  Files         810      812       +2     
  Lines      163515   163656     +141     
  Branches   163515   163656     +141     
==========================================
+ Hits       116877   116943      +66     
- Misses      41581    41670      +89     
+ Partials     5057     5043      -14     
Flag Coverage Δ
backward-compatibility 0.23% <0.00%> (-0.01%) ⬇️
db-migration 0.23% <0.00%> (-0.01%) ⬇️
genesis-check 1.34% <0.00%> (-0.01%) ⬇️
integration-tests 38.37% <95.33%> (+0.01%) ⬆️
linux 71.21% <96.66%> (-0.04%) ⬇️
linux-nightly 71.05% <96.66%> (-0.03%) ⬇️
macos 54.39% <23.33%> (-0.06%) ⬇️
pytests 1.60% <0.00%> (-0.01%) ⬇️
sanity-checks 1.40% <0.00%> (-0.01%) ⬇️
unittests 65.73% <94.00%> (-0.02%) ⬇️
upgradability 0.28% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nagisa nagisa force-pushed the implements-pipelined-prep-for-incoming-and-delayed-receipts branch from b05c4dc to d440b0b Compare August 7, 2024 15:15
@nagisa nagisa force-pushed the implements-pipelined-prep-for-incoming-and-delayed-receipts branch from d440b0b to 77d15b8 Compare August 8, 2024 14:18
@nagisa nagisa force-pushed the implements-pipelined-prep-for-incoming-and-delayed-receipts branch from 77d15b8 to 9eb4e47 Compare August 8, 2024 16:54
Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +1877 to +1890
if let Some(nsi) = &mut next_schedule_after {
*nsi = nsi.saturating_sub(1);
if *nsi == 0 {
// We're about to process a receipt that has been submitted for
// preparation, so lets submit the next one in anticipation that it might
// be processed too (it might also be not if we run out of gas/compute.)
next_schedule_after = schedule_contract_preparation(
&mut processing_state.pipeline_manager,
&processing_state.state_update,
&mut prep_lookahead_iter,
);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This piece of code seems to be repeating for every receipt type, is there any room to refactor it? I know it's already in a pretty good state but if you can reduce it to a single invocation it would be awesome :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I've been banging my head as to how to best deduplicate this and what I have here is the best I could come up with outside of trying to come up with a way to unify how the receipt types themselves are processed. Perhaps we ought to do that eventually, but it is also non-trivial (esp. due to how different is the handling of delayed receipts.)

runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved
state_update: &TrieUpdate,
mut iterator: impl Iterator<Item = R>,
) -> Option<usize> {
let scheduled_receipt_offset = iterator.position(|peek| {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think a for loop with an early return would be more readable. I guess it depends on individual preference so up to you.

runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved
@nagisa nagisa enabled auto-merge August 13, 2024 12:13
@nagisa nagisa added this pull request to the merge queue Aug 13, 2024
Merged via the queue into near:master with commit 781805d Aug 13, 2024
30 checks passed
@nagisa nagisa deleted the implements-pipelined-prep-for-incoming-and-delayed-receipts branch August 13, 2024 12:47
shreyan-gupta added a commit that referenced this pull request Aug 14, 2024
Trisfald added a commit to Trisfald/nearcore that referenced this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants