-
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
Changes from 21 commits
a4e861a
9c023d4
f6dcdb8
2b710b9
8288d28
83d09b7
0bb5f0c
47f348b
6569508
5f50a51
d3514ef
e3e685a
e1b9c99
9c609ae
45a89d1
a8591dc
1943fa7
94092d6
2c43a6e
aaff662
9903349
7c5bd82
edd733d
2ffa191
d0dbe45
f97d0e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use std::collections::BTreeSet; | ||
use std::collections::HashSet; | ||
use std::num::NonZeroUsize; | ||
use std::sync::Arc; | ||
|
||
|
@@ -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 }, | ||
/// Received a valid `ContractCodeResponse`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
Received(Vec<CodeBytes>), | ||
} | ||
|
@@ -145,7 +145,7 @@ struct CacheEntry { | |
|
||
enum CacheUpdate { | ||
WitnessPart(PartialEncodedStateWitness, Arc<WitnessEncoder>), | ||
AccessedContractHashes(BTreeSet<CodeHash>), | ||
AccessedContractHashes(HashSet<CodeHash>), | ||
AccessedContractCodes(Vec<CodeBytes>), | ||
} | ||
|
||
|
@@ -234,7 +234,7 @@ impl CacheEntry { | |
} | ||
} | ||
|
||
fn set_requested_contracts(&mut self, contract_hashes: BTreeSet<CodeHash>) { | ||
fn set_requested_contracts(&mut self, contract_hashes: HashSet<CodeHash>) { | ||
match &self.accessed_contracts { | ||
AccessedContractsState::Unknown => { | ||
self.accessed_contracts = AccessedContractsState::Requested { | ||
|
@@ -251,7 +251,7 @@ impl CacheEntry { | |
fn set_received_contracts(&mut self, contract_codes: Vec<CodeBytes>) { | ||
match &self.accessed_contracts { | ||
AccessedContractsState::Requested { contract_hashes, requested_at } => { | ||
let actual = BTreeSet::from_iter( | ||
let actual = HashSet::from_iter( | ||
contract_codes.iter().map(|code| CodeHash(CryptoHash::hash_bytes(&code.0))), | ||
); | ||
let expected = contract_hashes; | ||
|
@@ -380,7 +380,7 @@ impl PartialEncodedStateWitnessTracker { | |
pub fn store_accessed_contract_hashes( | ||
&mut self, | ||
key: ChunkProductionKey, | ||
hashes: BTreeSet<CodeHash>, | ||
hashes: HashSet<CodeHash>, | ||
) -> Result<(), Error> { | ||
tracing::debug!(target: "client", ?key, ?hashes, "store_accessed_contract_hashes"); | ||
let update = CacheUpdate::AccessedContractHashes(hashes); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use std::collections::{BTreeSet, HashMap, HashSet}; | ||
use std::collections::{HashMap, HashSet}; | ||
use std::sync::Arc; | ||
|
||
use near_async::messaging::{CanSend, IntoSender}; | ||
|
@@ -12,7 +12,7 @@ use near_primitives::hash::{hash, CryptoHash}; | |
use near_primitives::receipt::Receipt; | ||
use near_primitives::sharding::{ChunkHash, ReceiptProof, ShardChunk, ShardChunkHeader}; | ||
use near_primitives::stateless_validation::contract_distribution::{ | ||
ChunkContractAccesses, CodeHash, | ||
ChunkContractAccesses, ContractUpdates, | ||
}; | ||
use near_primitives::stateless_validation::state_witness::{ | ||
ChunkStateTransition, ChunkStateWitness, | ||
|
@@ -37,8 +37,7 @@ struct StateTransitionData { | |
main_transition: ChunkStateTransition, | ||
implicit_transitions: Vec<ChunkStateTransition>, | ||
applied_receipts_hash: CryptoHash, | ||
contract_accesses: BTreeSet<CodeHash>, | ||
contract_deploys: BTreeSet<CodeHash>, | ||
contract_updates: ContractUpdates, | ||
} | ||
|
||
/// Result of creating witness. | ||
|
@@ -48,10 +47,8 @@ struct StateTransitionData { | |
pub(crate) struct CreateWitnessResult { | ||
/// State witness created. | ||
pub(crate) state_witness: ChunkStateWitness, | ||
/// Code-hashes of contracts accessed while applying the previous chunk. | ||
pub(crate) contract_accesses: BTreeSet<CodeHash>, | ||
/// 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. done (will send a commit) |
||
|
||
impl Client { | ||
|
@@ -76,14 +73,13 @@ impl Client { | |
|
||
let my_signer = | ||
validator_signer.as_ref().ok_or(Error::NotAValidator(format!("send state witness")))?; | ||
let CreateWitnessResult { state_witness, contract_accesses, contract_deploys } = self | ||
.create_state_witness( | ||
my_signer.validator_id().clone(), | ||
prev_block_header, | ||
prev_chunk_header, | ||
chunk, | ||
transactions_storage_proof, | ||
)?; | ||
let CreateWitnessResult { state_witness, contract_updates } = self.create_state_witness( | ||
my_signer.validator_id().clone(), | ||
prev_block_header, | ||
prev_chunk_header, | ||
chunk, | ||
transactions_storage_proof, | ||
)?; | ||
|
||
if self.config.save_latest_witnesses { | ||
self.chain.chain_store.save_latest_chunk_state_witness(&state_witness)?; | ||
|
@@ -108,12 +104,12 @@ impl Client { | |
if ProtocolFeature::ExcludeContractCodeFromStateWitness.enabled(protocol_version) { | ||
// TODO(#11099): Currently we consume contract_deploys only for the following log message. Distribute it to validators | ||
// that will not validate the current witness so that they can follow-up with requesting the contract code. | ||
tracing::debug!(target: "client", ?contract_accesses, ?contract_deploys, "Contract accesses and deploys while sending state witness"); | ||
if !contract_accesses.is_empty() { | ||
self.send_contract_accesses_to_chunk_validators( | ||
tracing::debug!(target: "client", ?contract_updates, "Contract accesses and deploys while sending state witness"); | ||
if !contract_updates.contract_accesses.is_empty() { | ||
self.send_contract_updates_to_validators( | ||
epoch_id, | ||
&chunk_header, | ||
contract_accesses, | ||
contract_updates, | ||
my_signer.as_ref(), | ||
); | ||
} | ||
|
@@ -143,8 +139,7 @@ impl Client { | |
main_transition, | ||
implicit_transitions, | ||
applied_receipts_hash, | ||
contract_accesses, | ||
contract_deploys, | ||
contract_updates, | ||
} = self.collect_state_transition_data(&chunk_header, prev_chunk_header)?; | ||
|
||
let new_transactions = chunk.transactions().to_vec(); | ||
|
@@ -178,7 +173,7 @@ impl Client { | |
new_transactions, | ||
new_transactions_validation_state, | ||
); | ||
Ok(CreateWitnessResult { state_witness, contract_accesses, contract_deploys }) | ||
Ok(CreateWitnessResult { state_witness, contract_updates }) | ||
} | ||
|
||
/// Collect state transition data necessary to produce state witness for | ||
|
@@ -219,7 +214,7 @@ impl Client { | |
if current_shard_id != next_shard_id { | ||
// If shard id changes, we need to get implicit state | ||
// transition from current shard id to the next shard id. | ||
let (chunk_state_transition, _, _, _) = | ||
let (chunk_state_transition, _, _) = | ||
self.get_state_transition(¤t_block_hash, &next_epoch_id, next_shard_id)?; | ||
implicit_transitions.push(chunk_state_transition); | ||
} | ||
|
@@ -231,7 +226,7 @@ impl Client { | |
} | ||
|
||
// Add implicit state transition. | ||
let (chunk_state_transition, _, _, _) = self.get_state_transition( | ||
let (chunk_state_transition, _, _) = self.get_state_transition( | ||
¤t_block_hash, | ||
¤t_epoch_id, | ||
current_shard_id, | ||
|
@@ -247,19 +242,17 @@ impl Client { | |
implicit_transitions.reverse(); | ||
|
||
// Get the main state transition. | ||
let (main_transition, receipts_hash, contract_accesses, contract_deploys) = | ||
if prev_chunk_header.is_genesis() { | ||
self.get_genesis_state_transition(&main_block, &epoch_id, shard_id)? | ||
} else { | ||
self.get_state_transition(&main_block, &epoch_id, shard_id)? | ||
}; | ||
let (main_transition, receipts_hash, contract_updates) = if prev_chunk_header.is_genesis() { | ||
self.get_genesis_state_transition(&main_block, &epoch_id, shard_id)? | ||
} else { | ||
self.get_state_transition(&main_block, &epoch_id, shard_id)? | ||
}; | ||
|
||
Ok(StateTransitionData { | ||
main_transition, | ||
implicit_transitions, | ||
applied_receipts_hash: receipts_hash, | ||
contract_accesses: BTreeSet::from_iter(contract_accesses.into_iter()), | ||
contract_deploys: BTreeSet::from_iter(contract_deploys.into_iter()), | ||
contract_updates, | ||
}) | ||
} | ||
|
||
|
@@ -269,8 +262,7 @@ impl Client { | |
block_hash: &CryptoHash, | ||
epoch_id: &EpochId, | ||
shard_id: ShardId, | ||
) -> Result<(ChunkStateTransition, CryptoHash, BTreeSet<CodeHash>, BTreeSet<CodeHash>), Error> | ||
{ | ||
) -> Result<(ChunkStateTransition, CryptoHash, ContractUpdates), Error> { | ||
let shard_uid = self.chain.epoch_manager.shard_id_to_uid(shard_id, epoch_id)?; | ||
let stored_chunk_state_transition_data = self | ||
.chain | ||
|
@@ -303,15 +295,18 @@ impl Client { | |
contract_deploys, | ||
}) => (base_state, receipts_hash, contract_accesses, contract_deploys), | ||
}; | ||
let contract_updates = ContractUpdates { | ||
contract_accesses: HashSet::from_iter(contract_accesses.into_iter()), | ||
contract_deploys: HashSet::from_iter(contract_deploys.into_iter()), | ||
}; | ||
Ok(( | ||
ChunkStateTransition { | ||
block_hash: *block_hash, | ||
base_state, | ||
post_state_root: *self.chain.get_chunk_extra(block_hash, &shard_uid)?.state_root(), | ||
}, | ||
receipts_hash, | ||
BTreeSet::from_iter(contract_accesses.into_iter()), | ||
BTreeSet::from_iter(contract_deploys.into_iter()), | ||
contract_updates, | ||
)) | ||
} | ||
|
||
|
@@ -320,8 +315,7 @@ impl Client { | |
block_hash: &CryptoHash, | ||
epoch_id: &EpochId, | ||
shard_id: ShardId, | ||
) -> Result<(ChunkStateTransition, CryptoHash, BTreeSet<CodeHash>, BTreeSet<CodeHash>), Error> | ||
{ | ||
) -> Result<(ChunkStateTransition, CryptoHash, ContractUpdates), Error> { | ||
let shard_uid = self.epoch_manager.shard_id_to_uid(shard_id, &epoch_id)?; | ||
Ok(( | ||
ChunkStateTransition { | ||
|
@@ -331,7 +325,6 @@ impl Client { | |
}, | ||
hash(&borsh::to_vec::<[Receipt]>(&[]).unwrap()), | ||
Default::default(), | ||
Default::default(), | ||
)) | ||
} | ||
|
||
|
@@ -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( | ||
&self, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
epoch_id: &EpochId, | ||
chunk_header: &ShardChunkHeader, | ||
contract_accesses: BTreeSet<CodeHash>, | ||
contract_updates: ContractUpdates, | ||
my_signer: &ValidatorSigner, | ||
) { | ||
let ContractUpdates { contract_accesses, .. } = contract_updates; | ||
|
||
let chunk_production_key = ChunkProductionKey { | ||
epoch_id: *epoch_id, | ||
shard_id: chunk_header.shard_id(), | ||
|
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.