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

Limiting the processing of a block in Runtime by delaying receipts #1610

Merged
merged 15 commits into from
Nov 15, 2019

Conversation

evgenykuzyakov
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov commented Oct 31, 2019

  • Passing gas_limit from chunk_extra to the Runtime.
  • Processing receipts until the current total burnt_gas doesn't reach or exceed the gas_limit.
  • Update balance checker to work with the delayed receipts.
  • Tests to cover this. Update challenge test partial state, cause we touch another key in the Runtime to check for delayed receipts.

Going to update nomicon before merging this PR.

Ref #1507

@codecov
Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #1610 into staging will increase coverage by 0.05%.
The diff coverage is 93.04%.

Impacted file tree graph

@@             Coverage Diff             @@
##           staging    #1610      +/-   ##
===========================================
+ Coverage    79.88%   79.93%   +0.05%     
===========================================
  Files          130      131       +1     
  Lines        24504    24764     +260     
===========================================
+ Hits         19574    19795     +221     
- Misses        4930     4969      +39
Impacted Files Coverage Δ
chain/chain/src/test_utils.rs 84.13% <ø> (ø) ⬆️
core/primitives/src/errors.rs 14.2% <0%> (-0.53%) ⬇️
chain/chain/src/validate.rs 83.44% <100%> (+0.22%) ⬆️
core/primitives/src/utils.rs 88.23% <100%> (+0.27%) ⬆️
chain/client/tests/challenges.rs 92.81% <100%> (+0.06%) ⬆️
chain/client/src/client.rs 91.21% <100%> (+0.01%) ⬆️
chain/chain/src/types.rs 96.42% <100%> (+0.03%) ⬆️
near/src/runtime.rs 72.05% <100%> (+0.23%) ⬆️
chain/chain/src/chain.rs 90.54% <80%> (-0.04%) ⬇️
runtime/runtime/src/balance_checker.rs 93.69% <90.62%> (-0.58%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d4fda8...b4fafef. Read the comment docs.

chain/client/src/client.rs Outdated Show resolved Hide resolved
runtime/runtime/tests/runtime_group_tools/mod.rs Outdated Show resolved Hide resolved
runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved
runtime/runtime/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

One last suggestion: #1610 (comment)

@evgenykuzyakov
Copy link
Collaborator Author

Documentation for delayed receipts and Runtime crate: nearprotocol/nomicon#7

@@ -1153,13 +1219,17 @@ impl Runtime {

#[cfg(test)]
mod tests {
use super::*;

use near::config::INITIAL_GAS_PRICE;
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit weird to depend on near for just a constant that doesn't matter anyway.

Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov Nov 15, 2019

Choose a reason for hiding this comment

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

It was used within gas_burnt_to_reward. So right now it's indirect dependency anyway. I can refactor all of this in a different PR to remove dependency on near from fees_utils

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -635,6 +637,8 @@ impl RuntimeAdapter for NightshadeRuntime {
epoch_length: self.genesis_config.epoch_length,
gas_price,
block_timestamp,
// NOTE: verify transaction doesn't use gas limit
gas_limit: None,
Copy link
Member

Choose a reason for hiding this comment

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

should we use gas limit for verification as well?
because if block limit is small-ish and tx is gigantic, it won't fit in the block as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We probably shouldn't have TXs that are large enough to not fit into the block. We'll be able to refactor this later.

Copy link
Member

@ilblackdragon ilblackdragon left a comment

Choose a reason for hiding this comment

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

Generally looks good. Let's just remove near from dev-dependencies in runtime as it's not really needed.

Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

On a second look, I think we should split gas_limit into transaction_gas_limit and receipt_gas_limit, instead of dividing gas_limit by 2.

@evgenykuzyakov
Copy link
Collaborator Author

On a second look, I think we should split gas_limit into transaction_gas_limit and receipt_gas_limit, instead of dividing gas_limit by 2.

This is not an easy fix, cause it requires to change half of the chain, cause we recalculate gas_limit in the block limit.
Let's do this in a different PR.

@MaksymZavershynskyi
Copy link
Contributor

#1700

@evgenykuzyakov evgenykuzyakov merged commit 4a68466 into staging Nov 15, 2019
@evgenykuzyakov evgenykuzyakov deleted the block-limiting branch November 15, 2019 19:28
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