Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions ethcore/res/ethereum/multisig_test.json

Large diffs are not rendered by default.

26 changes: 25 additions & 1 deletion ethcore/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use client::{BlockInfo, CallContract};
use error::Error;
use executive::Executive;
use header::{BlockNumber, Header};
use spec::CommonParams;
use spec::{CommonParams, IrregularStateChangeAccount};
use state::{CleanupMode, Substate};
use trace::{NoopTracer, NoopVMTracer, Tracer, ExecutiveTracer, RewardType, Tracing};
use transaction::{self, SYSTEM_ADDRESS, UnverifiedTransaction, SignedTransaction};
Expand Down Expand Up @@ -193,6 +193,30 @@ impl EthereumMachine {
.and_then(|b| state.transfer_balance(child, beneficiary, &b, CleanupMode::NoEmpty))?;
}
}

if let Some(irregular_changes) = self.params.irregular_state_changes.get(&block.header().number()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block does not depend on ethash_params, does it have to be inside that particular if? If yes could you please add a comment explaining that.

Copy link
Contributor

Choose a reason for hiding this comment

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

definitely should be moved outside. other engines might have irregular state changes too.

let state = block.state_mut();
for &(address, ref irregular_account) in irregular_changes {
match irregular_account {
&IrregularStateChangeAccount::Set {
Copy link
Contributor

@rphmeier rphmeier Apr 15, 2018

Choose a reason for hiding this comment

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

single arm match can be replaced by an irrefutable destructuring.
let &IrregularStateChangeAccount::Set { nonce, balance, ref code, ref storage } = irregular_account;

Copy link
Collaborator

@sorpaas sorpaas Apr 15, 2018

Choose a reason for hiding this comment

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

I think the idea here was to also support other types of "irregular changes" with this format. For example, we might also have IrregularStateChangeAccount::Collect { accounts: Vec<Address>, target: Address }, collecting all accounts to target (thus support DAO hard fork change). Once we have more than one type we can't use let any more.

Another issue is that with let, if the IrregularStateChangeAccount enum changed to have more variants, it cannot detect it and throw type errors.

So I suggest we keep it.

Copy link
Contributor

@rphmeier rphmeier Apr 15, 2018

Choose a reason for hiding this comment

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

if the IrregularStateChangeAccount enum changed to have more variants, it cannot detect it and throw type errors.

that's not true. only irrefutable pattern matches can be used in a let-binding. adding another variant would make that pattern match no longer irrefutable.

I'm not going to push it any further because it's a small issue, but I don't think we should introduce more verbosity just because we might add different variants in the future.

Also: the DAO hard fork could also be implemented using only the variant we have now; the post-fork balances are historically available now that the fork itself is over.

nonce, balance, ref code, ref storage
} => {
if let Some(nonce) = nonce {
state.set_nonce(&address, &nonce)?;
}
if let Some(balance) = balance {
state.set_balance(&address, &balance)?;
}
if let &Some(ref code) = code {
state.reset_code(&address, code.clone())?;
}
for &(key, value) in storage {
state.set_storage(&address, key, value)?;
}
},
}
}
}
}

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ mod seal;
mod spec;

pub use self::genesis::Genesis;
pub use self::spec::{Spec, SpecHardcodedSync, SpecParams, CommonParams, OptimizeFor};
pub use self::spec::{Spec, SpecHardcodedSync, SpecParams, CommonParams, OptimizeFor, IrregularStateChangeAccount};
38 changes: 37 additions & 1 deletion ethcore/src/spec/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

//! Parameters for a block chain.

use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};
use std::io::Read;
use std::path::Path;
use std::sync::Arc;
Expand Down Expand Up @@ -53,6 +53,39 @@ fn fmt_err<F: ::std::fmt::Display>(f: F) -> String {
format!("Spec json is invalid: {}", f)
}

/// Ireegular state change accounts applied at specific blocks.
#[derive(Debug, Clone, PartialEq)]
pub enum IrregularStateChangeAccount {
/// Force setting values on an account.
Set {
/// New nonce forced setting.
nonce: Option<U256>,
/// New code forced setting.
code: Option<Bytes>,
/// New balance forced setting.
balance: Option<U256>,
/// Storage values forced setting.
storage: Vec<(H256, H256)>,
}
}

impl From<::ethjson::spec::IrregularStateChangeAccount> for IrregularStateChangeAccount {
fn from(p: ::ethjson::spec::IrregularStateChangeAccount) -> Self {
match p {
::ethjson::spec::IrregularStateChangeAccount::Set {
nonce, code, balance, storage
} => {
IrregularStateChangeAccount::Set {
nonce: nonce.map(Into::into),
code: code.map(Into::into),
balance: balance.map(Into::into),
storage: storage.unwrap_or_default().into_iter().map(|(k, v)| (k.into(), v.into())).collect(),
}
},
}
}
}

/// Parameters common to ethereum-like blockchains.
/// NOTE: when adding bugfix hard-fork parameters,
/// add to `contains_bugfix_hard_fork`
Expand Down Expand Up @@ -123,6 +156,8 @@ pub struct CommonParams {
pub max_code_size_transition: BlockNumber,
/// Transaction permission managing contract address.
pub transaction_permission_contract: Option<Address>,
/// Irregular state change list.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to mention that the irregular state changes are applied at the very beginning of the block to avoid confusion.

pub irregular_state_changes: HashMap<u64, Vec<(Address, IrregularStateChangeAccount)>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does u64 represent here? Maybe use BlockNumber instead?

}

impl CommonParams {
Expand Down Expand Up @@ -244,6 +279,7 @@ impl From<ethjson::spec::Params> for CommonParams {
BlockNumber::max_value(),
Into::into
),
irregular_state_changes: p.irregular_state_changes.unwrap_or_else(HashMap::new).into_iter().map(|(k, v)| (k.into(), v.into_iter().map(|(k, v)| (k.into(), v.into())).collect())).collect(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps split it into multiple lines for readability?

p.irregular_state_changes
  .unwrap_or_else(HashMap::new)
  .into_iter()
  .map(|(k, v)| (
    k.into(),
    v.into_iter().map(|(k, v)| (k.into(), v.into())).collect()
  ))
  .collect()

}
}
}
Expand Down
10 changes: 10 additions & 0 deletions ethcore/src/state/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,11 @@ impl Account {
self.nonce = self.nonce + U256::from(1u8);
}

/// Set the nonce of the account to a particular value.
pub fn set_nonce(&mut self, nonce: &U256) {
self.nonce = *nonce;
}

/// Increase account balance.
pub fn add_balance(&mut self, x: &U256) {
self.balance = self.balance + *x;
Expand All @@ -361,6 +366,11 @@ impl Account {
self.balance = self.balance - *x;
}

/// Directly set the account balance.
pub fn set_balance(&mut self, x: &U256) {
self.balance = *x;
}

/// Commit the `storage_changes` to the backing DB and update `storage_root`.
pub fn commit_storage(&mut self, trie_factory: &TrieFactory, db: &mut HashDB) -> trie::Result<()> {
let mut t = trie_factory.from_existing(db, &mut self.storage_root)?;
Expand Down
11 changes: 11 additions & 0 deletions ethcore/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,12 @@ impl<B: Backend> State<B> {
Ok(())
}

/// Directly set the balance of account `a`.
pub fn set_balance(&mut self, a: &Address, bala: &U256) -> trie::Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/bala/balance

self.require(a, false)?.set_balance(bala);
Ok(())
}

/// Subtracts `by` from the balance of `from` and adds it to that of `to`.
pub fn transfer_balance(&mut self, from: &Address, to: &Address, by: &U256, mut cleanup_mode: CleanupMode) -> trie::Result<()> {
self.sub_balance(from, by, &mut cleanup_mode)?;
Expand All @@ -664,6 +670,11 @@ impl<B: Backend> State<B> {
self.require(a, false).map(|mut x| x.inc_nonce())
}

/// Set the nonce of an account.
pub fn set_nonce(&mut self, a: &Address, nonce: &U256) -> trie::Result<()> {
self.require(a, false).map(|mut x| x.set_nonce(nonce))
}

/// Mutate storage of account `a` so that it is `value` for `key`.
pub fn set_storage(&mut self, a: &Address, key: H256, value: H256) -> trie::Result<()> {
trace!(target: "state", "set_storage({}:{:x} to {:x})", a, key, value);
Expand Down
2 changes: 1 addition & 1 deletion json/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub mod hardcoded_sync;
pub use self::account::Account;
pub use self::builtin::{Builtin, Pricing, Linear};
pub use self::genesis::Genesis;
pub use self::params::Params;
pub use self::params::{Params, IrregularStateChangeAccount};
pub use self::spec::Spec;
pub use self::seal::{Seal, Ethereum, AuthorityRoundSeal, TendermintSeal};
pub use self::engine::Engine;
Expand Down
28 changes: 28 additions & 0 deletions json/src/spec/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,35 @@

//! Spec params deserialization.

use std::collections::HashMap;
use uint::{self, Uint};
use hash::{H256, Address};
use bytes::Bytes;

/// See main CommonParams docs.
#[derive(Clone, Debug, PartialEq, Deserialize)]
pub enum IrregularStateChangeAccount {
/// See main CommonParams docs.
#[serde(rename="set")]
Set {
/// See main CommonParams docs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not super clear from CommonParams docs what it does. It seems safe explanatory, but I would add a comment here (especially for storage - it should clearly state that the storage entries are added and it doesn't replace existing storage entries)

#[serde(rename="nonce")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the renames are actually required.

nonce: Option<Uint>,

/// See main CommonParams docs.
#[serde(rename="code")]
code: Option<Bytes>,

/// See main CommonParams docs.
#[serde(rename="balance")]
balance: Option<Uint>,

/// See main CommonParams docs.
#[serde(rename="storage")]
storage: Option<HashMap<H256, H256>>,
}
}

/// Spec params.
#[derive(Debug, PartialEq, Deserialize)]
pub struct Params {
Expand Down Expand Up @@ -122,6 +147,9 @@ pub struct Params {
/// Wasm activation block height, if not activated from start
#[serde(rename="wasmActivationTransition")]
pub wasm_activation_transition: Option<Uint>,
/// See main CommonParams docs.
#[serde(rename="irregularStateChanges")]
pub irregular_state_changes: Option<HashMap<Uint, HashMap<Address, IrregularStateChangeAccount>>>,
}

#[cfg(test)]
Expand Down