-
Notifications
You must be signed in to change notification settings - Fork 622
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(contract-distribution): Filter out contract accesses that are newly-deployed and not present #12350
base: master
Are you sure you want to change the base?
Conversation
This reverts commit a4e861a.
@@ -450,10 +450,6 @@ impl Client { | |||
.into_iter() | |||
.collect(); | |||
|
|||
// Since chunk validators will receive the newly deployed contracts as part of the state witness (as DeployActions in receipts), | |||
// they will update their contract cache while applying these deploy actions, thus we can exclude code-hash for these contracts from the message. | |||
let predeployed_contract_accesses = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not needed anymore, since we are doing the filtering in the ContractStorage::finalize
now.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12350 +/- ##
=======================================
Coverage 71.19% 71.19%
=======================================
Files 839 839
Lines 169743 169755 +12
Branches 169743 169755 +12
=======================================
+ Hits 120844 120854 +10
+ Misses 43645 43643 -2
- Partials 5254 5258 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this mean that we have to read all contract accesses from storage now? right now that happens rarely since most of them are served from the compiled contract cache
Even though they are served from the compiled contract cache, we still read them from the storage here: nearcore/core/store/src/trie/mod.rs Line 1615 in 8e30ccd
because we need to include them (along with the internal nodes in trie) in the witness. let me know if I miss anything. The compiled contract caches still optimizes the latency since we do not need to re-compiled the code. The ideal setup would be some kind of Another option is to not call nearcore/runtime/runtime/src/actions.rs Line 122 in 8e30ccd
record_call call when executing the action (nearcore/runtime/runtime/src/actions.rs Line 216 in 8e30ccd
|
Currently function call may fail if code does not exist (eg. code-hash of the account is either default-hash or wrong). In this case, the failure of executing the function comes later, so we already record that the missing contract is called.
To fix this, there are two approaches:
ContractStorage
, not include the code-hashes that a) are newly-deployed and 2) do not exist in the storage.We take the second approach here, since the first approach led to non-clear code and prone to errors later as the code changes. The second approach introduces a storage read for the accesses, but we are already doing these reads in the current protocol (since we record the read of the contract code).
We update some unittests based on this change and add a new unittest for calling a missing contract (which fails without this change).
We also remove an optimization from the witness producer to exclude the deployments from accesses, since we are doing it in the
ContractStorage
now.