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

Block 0 is valid in queries #8891

Merged
merged 1 commit into from
Jun 14, 2018
Merged

Block 0 is valid in queries #8891

merged 1 commit into from
Jun 14, 2018

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Jun 13, 2018

Early exit for block nr 0 leads to spurious error about pruning: …your node is running with state pruning….

Fixes #7547, #8762

@dvdplm
Block 0 is valid in queries
24dd29b 
Early exit for block nr 0 leads to spurious error about pruning: `…your node is running with state pruning…`.

Fixes #7547, #8762
@dvdplm dvdplm self-assigned this Jun 13, 2018
@dvdplm dvdplm added F2-bug 🐞 The client fails to follow expected behavior. A0-pleasereview 🤓 Pull request needs code review. B0-patchthis M4-core ⛓ Core client code / Rust. labels Jun 13, 2018
@5chdn 5chdn removed the F2-bug 🐞 The client fails to follow expected behavior. label Jun 13, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 13, 2018
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 14, 2018
debris
debris reviewed Jun 14, 2018
View reviewed changes
ethcore/src/client/client.rs
@@ -1047,7 +1047,8 @@ impl Client {
/// Otherwise, this can fail (but may not) if the DB prunes state.
pub fn state_at_beginning(&self, id: BlockId) -> Option<State<StateDB>> {
match self.block_number(id) {
None | Some(0) => None,
None => None,
Some(0) => self.state_at(id),
Some(n) => self.state_at(BlockId::Number(n - 1)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

any idea why is it n - 1 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the state at the beginning of the block, i.e. last block's end state.

andresilva
andresilva reviewed Jun 14, 2018
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Since this method returns the state at the beginning of the block, for genesis I'm not sure whether this should be None or some empty state. I think genesis state should only exist after genesis block (or is it beginning of genesis? 🤔).

ethcore/src/client/client.rs
@@ -1047,7 +1047,8 @@ impl Client {
/// Otherwise, this can fail (but may not) if the DB prunes state.
pub fn state_at_beginning(&self, id: BlockId) -> Option<State<StateDB>> {
match self.block_number(id) {
None | Some(0) => None,
None => None,
Some(0) => self.state_at(id),
Some(n) => self.state_at(BlockId::Number(n - 1)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the state at the beginning of the block, i.e. last block's end state.

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 14, 2018

I'm not sure whether this should be None or some empty state.

@andresilva I wasn't sure either tbh, but the RPC output is "some empty state": {"jsonrpc":"2.0","result":[],"id":1}, which seem to make more sense than the misleading {"jsonrpc":"2.0","error":{"code":-32000,"message":"This request is not supported because your node is running with state pruning. Run with --pruning=archive."},"id":1} we return now.

Are you ok merging this?

andresilva
andresilva approved these changes Jun 14, 2018
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM

@andresilva andresilva merged commit 05e7c13 into master Jun 14, 2018
@debris debris deleted the fix/replayBlockTransactions_respond_with_spurious_error branch June 15, 2018 12:04
@ankitchiplunkar
Copy link

I'm not sure whether this should be None or some empty state.

I think the result of using method eth_traceReplayBlockTransactions with mode stateDiff (for the mainnet) at block 0 should be the genesis rewards. More details here: https://ethereum.stackexchange.com/questions/44196/which-node-can-provide-transactions-in-block-0

@andresilva
Copy link
Contributor

@ankitchiplunkar That's what I was getting at when I said this:

"I think genesis state should only exist after genesis block (or is it beginning of genesis? 🤔)."

So if genesis state exists at the beginning of the genesis block then stateDiff should be empty, if it's applied at the end of the genesis block then as you said stateDiff should show those state changes. TBH I don't think there's a right answer here, it's just semantics for an edge case we can define it whatever way we want. The current implementation will not return the stateDiff as you suggested and will just return [] instead (I think).

@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 15, 2018

just return [] instead (I think).

Correct.

@ankitchiplunkar ankitchiplunkar mentioned this pull request Jun 16, 2018
Closed
dvdplm added a commit that referenced this pull request Jun 16, 2018
@dvdplm
Merge branch 'master' into refactor/hashdb-generic
8b219ad 
* master:
  Minor fix in chain supplier and light provider (#8906)
  Block 0 is valid in queries (#8891)
  fixed osx permissions (#8901)
  Atomic create new files with permissions to owner in ethstore (#8896)
  Add ETC Cooperative-run load balanced parity node (#8892)
  Add support for --chain tobalaba (#8870)
  fix some warns on nightly (#8889)
  Add new ovh bootnodes and fix port for foundation bootnode 3.2 (#8886)
  SecretStore: service pack 1 (#8435)
tavakyan referenced this pull request in C4Coin/c4coin-parity Jun 18, 2018
@dvdplm @tavakyan
Block 0 is valid in queries (#8891)
b8b1aaf 
Early exit for block nr 0 leads to spurious error about pruning: `…your node is running with state pruning…`.

Fixes #7547, #8762
This was referenced Jun 18, 2018
Closed
Closed
andresilva pushed a commit that referenced this pull request Jun 18, 2018
@dvdplm @andresilva
Block 0 is valid in queries (#8891)
36f8ca0 
Early exit for block nr 0 leads to spurious error about pruning: `…your node is running with state pruning…`.

Fixes #7547, #8762
@andresilva andresilva mentioned this pull request Jun 18, 2018
Merged
18 tasks
ascjones pushed a commit that referenced this pull request Jun 18, 2018
@dvdplm @ascjones
Block 0 is valid in queries (#8891)
bdef4b3 
Early exit for block nr 0 leads to spurious error about pruning: `…your node is running with state pruning…`.

Fixes #7547, #8762
@ascjones ascjones mentioned this pull request Jun 18, 2018
Merged
9 tasks
5chdn pushed a commit that referenced this pull request Jun 19, 2018
@ascjones @5chdn
[stable] Backports (#8919)
ba79cad 
* Fixed AuthorityRound deadlock on shutdown, closes #8088 (#8803)

* CI: Fix docker tags (#8822)

* scripts: enable docker builds for beta and stable

* scripts: docker latest should be beta not master

* scripts: docker latest is master

* Fix concurrent access to signer queue (#8854)

* Fix concurrent access to signer queue

* Put request back to the queue if confirmation failed

* typo: fix docs and rename functions to be more specific

`request_notify` does not need to be public, and it's renamed to `notify_result`.
`notify` is renamed to `notify_message`.

* Change trace info "Transaction" -> "Request"

* Add new ovh bootnodes and fix port for foundation bootnode 3.2 (#8886)

* Add new ovh bootnodes and fix port for foundation bootnode 3.2

* Remove old bootnodes.

* Remove duplicate 1118980bf48b0a3640bdba04e0fe78b1add18e1cd99bf22d53daac1fd9972ad650df52176e7c7d89d1114cfef2bc23a2959aa54998a46afcf7d91809f0855082

* Block 0 is valid in queries (#8891)

Early exit for block nr 0 leads to spurious error about pruning: `…your node is running with state pruning…`.

Fixes #7547, #8762

* update jsonrpc libs, fixed ipc leak, closes #8774 (#8876)

Instead of cherrypicking 8b78141, just ran cargo update -p jsonrpc-core

* Add ETC Cooperative-run load balanced parity node (#8892)

* Minor fix in chain supplier and light provider (#8906)

* fix chain supplier increment

* fix light provider block_headers
5chdn pushed a commit that referenced this pull request Jun 19, 2018
@andresilva @5chdn
[beta] Backports (#8916)
cc44ae9 
* `duration_ns: u64 -> duration: Duration` (#8457)

* duration_ns: u64 -> duration: Duration

* format on millis {:.2} -> {}

* Keep all enacted blocks notify in order (#8524)

* Keep all enacted blocks notify in order

* Collect is unnecessary

* Update ChainNotify to use ChainRouteType

* Fix all ethcore fn defs

* Wrap the type within ChainRoute

* Fix private-tx and sync api

* Fix secret_store API

* Fix updater API

* Fix rpc api

* Fix informant api

* Eagerly cache enacted/retracted and remove contain_enacted/retracted

* Fix indent

* tests: should use full expr form for struct constructor

* Use into_enacted_retracted to further avoid copy

* typo: not a function

* rpc/tests: ChainRoute -> ChainRoute::new

* Handle removed logs in filter changes and add geth compatibility field (#8796)

* Add removed geth compatibility field in log

* Fix mocked tests

* Add field block hash in PollFilter

* Store last block hash info for log filters

* Implement canon route

* Use canon logs for fetching reorg logs

Light client removed logs fetching is disabled. It looks expensive.

* Make sure removed flag is set

* Address grumbles

* Fixed AuthorityRound deadlock on shutdown, closes #8088 (#8803)

* CI: Fix docker tags (#8822)

* scripts: enable docker builds for beta and stable

* scripts: docker latest should be beta not master

* scripts: docker latest is master

* ethcore: fix ancient block error msg handling (#8832)

* Disable parallel verification and skip verifiying already imported txs. (#8834)

* Reject transactions that are already in pool without verifying them.

* Avoid verifying already imported transactions.

* Fix concurrent access to signer queue (#8854)

* Fix concurrent access to signer queue

* Put request back to the queue if confirmation failed

* typo: fix docs and rename functions to be more specific

`request_notify` does not need to be public, and it's renamed to `notify_result`.
`notify` is renamed to `notify_message`.

* Change trace info "Transaction" -> "Request"

* Don't allocate in expect_valid_rlp unless necessary (#8867)

* don't allocate via format! in case there's no error

* fix test?

* fixed ipc leak, closes #8774 (#8876)

* Add new ovh bootnodes and fix port for foundation bootnode 3.2 (#8886)

* Add new ovh bootnodes and fix port for foundation bootnode 3.2

* Remove old bootnodes.

* Remove duplicate 1118980bf48b0a3640bdba04e0fe78b1add18e1cd99bf22d53daac1fd9972ad650df52176e7c7d89d1114cfef2bc23a2959aa54998a46afcf7d91809f0855082

* Block 0 is valid in queries (#8891)

Early exit for block nr 0 leads to spurious error about pruning: `…your node is running with state pruning…`.

Fixes #7547, #8762

* Add ETC Cooperative-run load balanced parity node (#8892)

* Minor fix in chain supplier and light provider (#8906)

* fix chain supplier increment

* fix light provider block_headers

* Check whether we need resealing in miner and unwrap has_account in account_provider (#8853)

* Remove unused Result wrap in has_account

* Check whether we need to reseal for external transactions

* Fix reference to has_account interface

* typo: missing )

* Refactor duplicates to prepare_and_update_sealing

* Fix build

* Allow disabling local-by-default for transactions with new config entry (#8882)

* Add tx_queue_allow_unknown_local config option

- Previous commit messages:

dispatcher checks if we have the sender account

Add `tx_queue_allow_unknown_local` to MinerOptions

Add `tx_queue_allow_unknown_local` to config

fix order in MinerOptions to match Configuration

add cli flag for tx_queue_allow_unknown_local

Update refs to `tx_queue_allow_unknown_local`

Add tx_queue_allow_unknown_local to config test

revert changes to dispatcher

Move tx_queue_allow_unknown_local to `import_own_transaction`

Fix var name

if statement should return the values

derp de derp derp derp semicolons

Reset dispatch file to how it was before

fix compile issues + change from FLAG to ARG

add test and use `into`

import MinerOptions, clone the secret

Fix tests?

Compiler/linter issues fixed

Fix linter msg - case of constants

IT LIVES

refactor to omit yucky explict return

update comments

Fix based on diff AccountProvider.has_account method

* Refactor flag name + don't change import_own_tx behaviour

fix arg name

Note: force commit to try and get gitlab tests working again 😠

* Add fn to TestMinerService

* Avoid race condition from trusted sources

- refactor the miner tests a bit to cut down on code reuse
- add `trusted` param to dispatch_transaction and import_claimed_local_transaction

Add param to `import_claimed_local_transaction`

Fix fn sig in tests
ordian added a commit to ordian/parity that referenced this pull request Jun 20, 2018
@ordian
Merge branch 'master' of https://github.com/paritytech/parity into wa…
fd88e46 
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity: (29 commits)
  Block 0 is valid in queries (openethereum#8891)
  fixed osx permissions (openethereum#8901)
  Atomic create new files with permissions to owner in ethstore (openethereum#8896)
  Add ETC Cooperative-run load balanced parity node (openethereum#8892)
  Add support for --chain tobalaba (openethereum#8870)
  fix some warns on nightly (openethereum#8889)
  Add new ovh bootnodes and fix port for foundation bootnode 3.2 (openethereum#8886)
  SecretStore: service pack 1 (openethereum#8435)
  Handle removed logs in filter changes and add geth compatibility field (openethereum#8796)
  fixed ipc leak, closes openethereum#8774 (openethereum#8876)
  scripts: remove md5 checksums (openethereum#8884)
  hardware_wallet/Ledger `Sign messages` + some refactoring (openethereum#8868)
  Check whether we need resealing in miner and unwrap has_account in account_provider (openethereum#8853)
  docker: Fix alpine build (openethereum#8878)
  Remove mac os installers etc (openethereum#8875)
  README.md: update the list of dependencies (openethereum#8864)
  Fix concurrent access to signer queue (openethereum#8854)
  Tx permission contract improvement (openethereum#8400)
  Limit the number of transactions in pending set (openethereum#8777)
  Use sealing.enabled to emit eth_mining information (openethereum#8844)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@debris debris debris approved these changes

@andresilva andresilva andresilva approved these changes

Assignees

@dvdplm dvdplm

Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.0 Ethereum
Development

Successfully merging this pull request may close these issues.
5 participants
@dvdplm @ankitchiplunkar @andresilva @debris @5chdn