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

Consider epoch id when sending challenge on double signing #1628

Merged
merged 5 commits into from Nov 4, 2019

Conversation

@bowenwang1996
Copy link
Member

bowenwang1996 commented Nov 3, 2019

Right now we would send a challenge as long as we receive two different blocks with the same height, whereas it is possible that the two blocks belong to different epochs and are thus signed by different block producers. In that case, no challenge should be produced.
TODO: update chain store cache after committing ChainStoreUpdate.

}));
}
} else {
for (epoch_id, block_hash) in epoch_id_to_hash.iter() {

This comment has been minimized.

Copy link
@ilblackdragon

ilblackdragon Nov 3, 2019

Member

let's remove challenge across different epochs.
Also finality gadget adds protection to make sure we work of finalized epoch

store_update
.set_ser(COL_BLOCK_PER_HEIGHT, &index_to_bytes(header.inner.height), &hash)
.map_err::<Error, _>(|e| e.into())?;
match self.chain_store.get_any_block_hash_by_height(header.inner.height) {

This comment has been minimized.

Copy link
@ilblackdragon

ilblackdragon Nov 3, 2019

Member
let map = match self.chain_store.get_any_block_hash_by_height(header.inner.height) {
                Ok(map) => map, 
                Err(_) => &HashMap::new() // if ref here doesn't work can clone above
};
if !map.contains_key(&header.inner.epoch_id) {
  let mut new_map = map.clone();
  new_map.insert(header.inner.epoch_id.clone(), hash);
  store_update
                            .set_ser(
                                COL_BLOCK_PER_HEIGHT,
                                &index_to_bytes(header.inner.height),
                                &new_map,
                            )
                            .map_err::<Error, _>(|e| e.into())?;
}
@@ -203,6 +203,10 @@ impl Client {
&self.runtime_adapter.get_epoch_id_from_prev_block(&head.last_block_hash).unwrap(),
next_height,
)?;
println!(

This comment has been minimized.

Copy link
@ilblackdragon

ilblackdragon Nov 3, 2019

Member

remove print

@@ -245,6 +245,7 @@ impl EpochManager {
last_block_hash: &CryptoHash,
rng_seed: RngSeed,
) -> Result<EpochId, EpochError> {
println!("finalize epoch");

This comment has been minimized.

Copy link
@ilblackdragon

ilblackdragon Nov 3, 2019

Member

remove print here and below

* ChainStore cache update

* Rename store method in ChainStore

* refactor
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (staging@68c77c9). Click here to learn what that means.
The diff coverage is 96.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             staging    #1628   +/-   ##
==========================================
  Coverage           ?   79.17%           
==========================================
  Files              ?      124           
  Lines              ?    22637           
  Branches           ?        0           
==========================================
  Hits               ?    17922           
  Misses             ?     4715           
  Partials           ?        0
Impacted Files Coverage Δ
chain/chain/src/test_utils.rs 78.85% <ø> (ø)
chain/client/tests/process_blocks.rs 78.84% <100%> (ø)
core/primitives/src/types.rs 100% <100%> (ø)
core/primitives/src/sharding.rs 99% <100%> (ø)
chain/client/src/test_utils.rs 79.77% <100%> (ø)
chain/client/tests/chunks_management.rs 100% <100%> (ø)
chain/epoch_manager/src/lib.rs 96.39% <100%> (ø)
chain/chain/src/chain.rs 90.75% <90%> (ø)
chain/client/tests/challenges.rs 94.35% <90%> (ø)
chain/chain/src/store.rs 95.43% <98.96%> (ø)

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 68c77c9...349709d. Read the comment docs.

@ilblackdragon ilblackdragon merged commit d8f4cbe into staging Nov 4, 2019
3 checks passed
3 checks passed
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
gitlab-ci
Details
@ilblackdragon ilblackdragon deleted the fix-challenge2 branch Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.