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

Misc fixes #11510

Merged
merged 2 commits into from
Feb 23, 2020
Merged

Misc fixes #11510

merged 2 commits into from
Feb 23, 2020

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Feb 22, 2020

Did some code reading and made random changes as I went.

Did some code reading and made random changes as I went.
@dvdplm dvdplm self-assigned this Feb 22, 2020
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Feb 22, 2020
//// TODO: cloning for `State` shouldn't be possible in general; Remove this and use
//// checkpoints where possible.
// TODO: cloning for `State` shouldn't be possible in general; Remove this and use
// checkpoints where possible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have an issue for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not that I know of no. I'm hesitant to add one because I don't fully understand why state should not be cloneable. Do you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it was added in #4632,
other related PRs

I assume that using checkpoints instead of Clones is more efficient, since we only store the diff.
We still use clones here https://github.com/paritytech/parity-ethereum/blob/856a0755888a30d4dc52a68cb2638a8bfd5786ac/ethcore/src/block.rs#L239

cc @tomusdrw

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ethcore/src/client/client.rs Outdated Show resolved Hide resolved
/// Creates new cache config with cumulative size equal `total`.
/// Creates new cache config with cumulative size equal to `total`, distributed as follows: 70%
/// to rocksdb, 10% to the blockchain cache and 20% to the state cache. The transaction queue
/// cache size is set to 40Mb and the trace cache to 20Mb.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should reconsider the defaults

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You might be right. Or not. Really hard to even have an opinion about this without some serious testing.

Co-Authored-By: Andronik Ordian <write@reusable.software>
@dvdplm dvdplm merged commit bbcd094 into master Feb 23, 2020
@dvdplm dvdplm deleted the dp/chore/misc-fixes branch February 23, 2020 15:49
ordian added a commit that referenced this pull request Mar 6, 2020
* master: (27 commits)
  Faster kill_garbage (#11514)
  [EngineSigner]: don't sign message with only zeroes (#11524)
  fix compilation warnings (#11522)
  [ethcore cleanup]: various unrelated fixes from `#11493` (#11507)
  Add benchmark for transaction execution (#11509)
  Add Smart Contract License v1.0
  Misc fixes (#11510)
  [dependencies]: unify `rustc-hex` (#11506)
  Activate on-chain randomness in POA Sokol (#11505)
  Grab bag of cleanup (#11504)
  Implement eth/64 (EIP-2364) and drop support for eth/62 (#11472)
  [dependencies]: remove `util/macros` (#11501)
  OpenEthereum bootnodes are added (#11499)
  [ci benches]: use `all-features` (#11496)
  [verification]: make test-build compile standalone (#11495)
  complete null-signatures removal (#11491)
  Include the seal when populating the header for a new block (#11475)
  fix compilation warnings (#11492)
  cargo update -p cmake (#11490)
  update to published rlp-derive (#11489)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants