Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

get rid of hidden mutability of Spec #10904

Merged
merged 4 commits into from
Jul 30, 2019
Merged

get rid of hidden mutability of Spec #10904

merged 4 commits into from
Jul 30, 2019

Conversation

debris
Copy link
Collaborator

@debris debris commented Jul 19, 2019

changes in this pr:

  • change Spec::state_root_memo from RwLock<H256> to H256
  • cleanup how SpecHardcodedSync is being deserialized from json and printed

@debris debris requested review from ordian and niklasad1 July 19, 2019 09:35
genesis_state: self.genesis_state.clone(),
}
}
}

/// Part of `Spec`. Describes the hardcoded synchronization parameters.
#[derive(Debug, Clone)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clone replaces impl Clone bolierplate.
Debug is has replaced to_json which was used to print this structure

&Default::default(),
BasicBackend(journaldb::new_memory_db()),
)?;

self.state_root_memo = root;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

run_constructors no longer mutates self.state_root_memo, so we need to set the root manutally

.collect();
let genesis_state: PodState = s.accounts.into();

let (state_root_memo, _) = run_constructors(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

by running constructors always before Spec is created, we ensure that spec_root_memo is always correct

ethcore/src/spec/spec.rs Outdated Show resolved Hide resolved
#[serde(deny_unknown_fields)]
#[serde(rename_all = "camelCase")]
pub struct HardcodedSync {
/// Hexadecimal of the RLP encoding of the header of the block to start synchronization from.
pub header: String,
pub header: Bytes,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it should be Bytes, not String. Someone did it out of laziness and, because of that we had to deal unparsed string in ethcore

@@ -68,7 +68,6 @@ fn make_spec(chain: &BlockChain) -> Spec {
let state = chain.pre_state.clone().into();
spec.set_genesis_state(state).expect("unable to set genesis state");
spec.overwrite_genesis_params(genesis);
assert!(spec.is_state_root_valid());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's always valid now

@debris debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 19, 2019
@ordian ordian added this to the 2.7 milestone Jul 19, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 21, 2019
@debris debris requested a review from tomusdrw July 29, 2019 14:37
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm

db
)?;

assert_eq!(root, self.state_root(), "Spec's state root has not been precomputed correctly.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it safe / expected to panic here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, cause self.state_root() is always created during init with the same constructors

@debris debris merged commit 6c7d0fe into master Jul 30, 2019
@debris debris deleted the spec-cleanup branch July 30, 2019 10:48
ordian added a commit that referenced this pull request Aug 7, 2019
* master:
  journaldb changes (#10929)
  Allow default block parameter to be blockHash (#10932)
  Enable sealing when engine is ready (#10938)
  Fix some warnings and typos. (#10941)
  Updated security@parity.io key (#10939)
  Change the return type of step_inner function. (#10940)
  get rid of hidden mutability of Spec (#10904)
  simplify BlockReward::reward implementation (#10906)
  Kaspersky AV whitelisting (#10919)
  additional arithmetic EVM opcode benchmarks (#10916)
  [Cargo.lock] cargo update -p crossbeam-epoch (#10921)
  Fixes incorrect comment. (#10913)
  Add file path to disk map write/read warnings (#10911)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants