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

Implement NoProof for json tests and update tests reference (replaces #9744) #9814

Merged
merged 17 commits into from
Nov 1, 2018

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Oct 26, 2018

Closes #9590 .

Update to latest ethereum/tests .
Fix cost for constantinople test json.
Implements 'NoProof' engine from ethereum/tests#464 .

At this point with this PR everything pass except the case spotted by @holiman this week in https://gist.github.com/holiman/0154f00d5fcec5f89e85894cbb46fcb2 .

I skip those tests. For completeness, @sorpaas indicated that it is not an easy fix : so since those case cannot happen on a mainchain I did not open and reference an issue for them (in the json defining tests to skip I only added a comment but not issue reference).

Note for reviewer:

  • an other version of this PR exists with previous review comments is Implement NoProof for json tests and update tests reference #9744 , this PR replaces it .
  • when running NoProof with CanonNoSeal, the upper gas limit verification is required : I switch it from verify_block_basic to the header check. I got to put another trait method for it, a lighter change would be to make this check for every engine but it would change a bit the existing behavior (might not be an issue).

Block test are really not working so I disabled a few by commenting
directly in source.
Since tests has been regenerated those one were failing on block
difficulty check.

Update ethereum/tests, waiting for cost fix (block test are still
commented).
identified case).
Fix block reward of constantinople json.
verification, the test is running in `verify_header_param`.
Note that test was previously only for ethash, and now for any engine.
Update tests submodule to latest master (lot of new sstore tests
passing)
@cheme cheme added A0-pleasereview 🤓 Pull request needs code review. B1-patch-beta 🕷🕷 M4-core ⛓ Core client code / Rust. B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. labels Oct 26, 2018
@parity-cla-bot
Copy link

It looks like @cheme signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn mentioned this pull request Oct 27, 2018
13 tasks
@5chdn 5chdn added this to the 2.3 milestone Oct 29, 2018
@5chdn 5chdn added the B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. label Oct 29, 2018
@@ -222,6 +222,8 @@ impl Engine<EthereumMachine> for Arc<Ethash> {

fn maximum_uncle_count(&self, _block: BlockNumber) -> usize { 2 }

fn maximum_gas_limit(&self) -> Option<U256> { Some(0x7fffffffffffffffu64.into()) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use underscores for this number to make it a little bit more readable?

@@ -281,6 +281,11 @@ pub fn verify_header_params(header: &Header, engine: &EthEngine, is_full: bool,
if header.gas_limit() < &min_gas_limit {
return Err(From::from(BlockError::InvalidGasLimit(OutOfBounds { min: Some(min_gas_limit), max: None, found: header.gas_limit().clone() })));
}
if let Some(limit) = engine.maximum_gas_limit() {
if header.gas_limit() > &limit {
return Err(From::from(::error::BlockError::InvalidGasLimit(OutOfBounds { min: None, max: Some(limit), found: header.gas_limit().clone() })));
Copy link
Collaborator

Choose a reason for hiding this comment

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

clone() on copy-type

Suggested change
return Err(From::from(::error::BlockError::InvalidGasLimit(OutOfBounds { min: None, max: Some(limit), found: header.gas_limit().clone() })));
return Err(From::from(::error::BlockError::InvalidGasLimit(OutOfBounds { min: None, max: Some(limit), found: *header.gas_limit() })));

Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good, but I have too little knowledge of this to approve

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 👍 Just some whitespace grumbles.

} else {
Self::from_spec(make_spec(chain))
};

Copy link
Contributor

Choose a reason for hiding this comment

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

stray newline

}

fn from_spec_conf(spec: Spec, config: ClientConfig) -> Self {

Copy link
Contributor

Choose a reason for hiding this comment

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

stray newline

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

stray newline. Actually would be nice if you could run a pretty printer on this file :)

@5chdn 5chdn merged commit a511264 into openethereum:master Nov 1, 2018
This was referenced Nov 1, 2018
cheme added a commit that referenced this pull request Nov 1, 2018
…9744) (#9814)

* Update test reference.
Block test are really not working so I disabled a few by commenting
directly in source.

* Move ethtest commit cursor.

* Implements 'NoProof' engine from ethereum/tests#464 .

Since tests has been regenerated those one were failing on block
difficulty check.

Update ethereum/tests, waiting for cost fix (block test are still
commented).

* Update tests submodule reference to latest (all test passing except an
identified case).
Fix block reward of constantinople json.

* Restore broken test by using old json tests files.

* Use CanonNoSeal instead of a custom engine, still have to include some
additional tests code.

* Gas upper limit check in json_chain test was bad, moving the test to
verification, the test is running in `verify_header_param`.
Note that test was previously only for ethash, and now for any engine.

* Restore old behavior (gas uper limit only for ethash engine), at the
cost of an additional trait method.

* Proper rpc test fix.

* Update tests submodule, add SStore bug tests.

* Fix json issue tabulation.
Update tests submodule to latest master (lot of new sstore tests
passing)

* Switch ethereum/tests to tag 6.0.0-beta.1 (no tests changes from latest
synch).

* Display hex with separator, use indirection instead of clone for copy
types.
5chdn added a commit that referenced this pull request Nov 2, 2018
* version: mark 2.2.0 beta

* ci: remove failing tests for android, windows, and macos (#9788)

* ci: remove failing tests for android, windows, and macos

* ci: restore android build jobs

* Move state root verification before gas used (#9841)

* Classic.json Bootnode Update (#9828)

* fix: Update bootnodes list to only responsive nodes

* feat: Add more bootnodes to classic.json list

* feat: Add retested bootnodes

* Implement NoProof for json tests and update tests reference (replaces #9744) (#9814)

* Update test reference.
Block test are really not working so I disabled a few by commenting
directly in source.

* Move ethtest commit cursor.

* Implements 'NoProof' engine from ethereum/tests#464 .

Since tests has been regenerated those one were failing on block
difficulty check.

Update ethereum/tests, waiting for cost fix (block test are still
commented).

* Update tests submodule reference to latest (all test passing except an
identified case).
Fix block reward of constantinople json.

* Restore broken test by using old json tests files.

* Use CanonNoSeal instead of a custom engine, still have to include some
additional tests code.

* Gas upper limit check in json_chain test was bad, moving the test to
verification, the test is running in `verify_header_param`.
Note that test was previously only for ethash, and now for any engine.

* Restore old behavior (gas uper limit only for ethash engine), at the
cost of an additional trait method.

* Proper rpc test fix.

* Update tests submodule, add SStore bug tests.

* Fix json issue tabulation.
Update tests submodule to latest master (lot of new sstore tests
passing)

* Switch ethereum/tests to tag 6.0.0-beta.1 (no tests changes from latest
synch).

* Display hex with separator, use indirection instead of clone for copy
types.
DemiMarie pushed a commit to poanetwork/parity-ethereum that referenced this pull request Jan 23, 2019
* version: mark 2.2.0 beta

* ci: remove failing tests for android, windows, and macos (openethereum#9788)

* ci: remove failing tests for android, windows, and macos

* ci: restore android build jobs

* Move state root verification before gas used (openethereum#9841)

* Classic.json Bootnode Update (openethereum#9828)

* fix: Update bootnodes list to only responsive nodes

* feat: Add more bootnodes to classic.json list

* feat: Add retested bootnodes

* Implement NoProof for json tests and update tests reference (replaces openethereum#9744) (openethereum#9814)

* Update test reference.
Block test are really not working so I disabled a few by commenting
directly in source.

* Move ethtest commit cursor.

* Implements 'NoProof' engine from ethereum/tests#464 .

Since tests has been regenerated those one were failing on block
difficulty check.

Update ethereum/tests, waiting for cost fix (block test are still
commented).

* Update tests submodule reference to latest (all test passing except an
identified case).
Fix block reward of constantinople json.

* Restore broken test by using old json tests files.

* Use CanonNoSeal instead of a custom engine, still have to include some
additional tests code.

* Gas upper limit check in json_chain test was bad, moving the test to
verification, the test is running in `verify_header_param`.
Note that test was previously only for ethash, and now for any engine.

* Restore old behavior (gas uper limit only for ethash engine), at the
cost of an additional trait method.

* Proper rpc test fix.

* Update tests submodule, add SStore bug tests.

* Fix json issue tabulation.
Update tests submodule to latest master (lot of new sstore tests
passing)

* Switch ethereum/tests to tag 6.0.0-beta.1 (no tests changes from latest
synch).

* Display hex with separator, use indirection instead of clone for copy
types.
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. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Currently failing JSON tests
6 participants