-
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
refactor: Combine contract accesses and deployments into ContractUpdates #12326
Conversation
This reverts commit a4e861a.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12326 +/- ##
===========================================
+ Coverage 38.11% 71.26% +33.15%
===========================================
Files 837 838 +1
Lines 168711 169346 +635
Branches 168711 169346 +635
===========================================
+ Hits 64298 120686 +56388
+ Misses 100525 43414 -57111
- Partials 3888 5246 +1358
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.
Looks good!
) { | ||
if let Some(partial_storage) = partial_storage { | ||
let ContractUpdates { contract_accesses, contract_deploys } = contract_updates; |
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 the interaction layer with store, verifying whether it's okay to not have contract_accesses and contract_deploys sorted?
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.
Just checked, I think we are storing vec here, so should be fine
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.
We do not rely on sorted-ness of the hashes (eg. not keeping a hash of the list etc.) so using HashSet should be fine. We store the hashes in the DB as Vector but the storage is temporary and again does not depend on the sortedness.
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.
By the way, do you know the reason to store hashes as vectors? Is it to save space?
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.
yeah two considerations, first I started using vector all over the place in the first PR and did not want to change the protocol and DB structures. Second, I thought vec would store them more compactly and we turn them into hashset when processing anyways, thus did not want to deal with a migration.
/// Code-hashes of contracts deployed while applying the previous chunk. | ||
pub(crate) contract_deploys: BTreeSet<CodeHash>, | ||
/// Contracts accessed and deployed while applying the chunk. | ||
pub contract_updates: ContractUpdates, |
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.
pub(crate)?
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.
done (will send a commit)
@@ -422,13 +415,15 @@ impl Client { | |||
/// Sends the contract accesses to the same chunk validators | |||
/// (except for the chunk producers that track the same shard), | |||
/// which will receive the state witness for the new chunk. | |||
fn send_contract_accesses_to_chunk_validators( | |||
fn send_contract_updates_to_validators( |
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.
Seems like we are still only sending contract_accesses and not contract_updates. Do we want to rename this function?
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.
I am planning to also send the deployments, but yeah I can rename in the later PRs. Reverted the naming change here.
…as/nearcore into contract-dist-refactor
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.
🚀
) { | ||
if let Some(partial_storage) = partial_storage { | ||
let ContractUpdates { contract_accesses, contract_deploys } = contract_updates; |
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.
By the way, do you know the reason to store hashes as vectors? Is it to save space?
@@ -43,7 +43,7 @@ enum AccessedContractsState { | |||
Unknown, | |||
/// Received `ChunkContractAccesses` and sent `ContractCodeRequest`, | |||
/// waiting for response from the chunk producer. | |||
Requested { contract_hashes: BTreeSet<CodeHash>, requested_at: Instant }, | |||
Requested { contract_hashes: HashSet<CodeHash>, requested_at: Instant }, |
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.
What're the pros and cons of BTreeSet vs HashSet here? Why are we using one vs the other? Would Vec also work?
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.
The reason for set vs vector is to indicate using the type that the items are unique.
HashSet is known to be faster than BTreeSet and also since we do not need order/sortedness, thus I wanted to replace it with HashSet.
The propagation of contract accesses (calls) and deployments are done together. Thus, introducing a wrapper called
ContractUpdates
that contains the accesses and deployments and this is passed through the call stack from the chunk application up to the point we send the messages.Also address a TODO to change BTreeSet to HashSet to contain the code hashes.
We do NOT change the representation of code hashes in database and network messages, but only the internal data structures.