New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use signed 256-bit integer for sstore gas refund substate #9746

Merged
merged 6 commits into from Oct 15, 2018

Conversation

@sorpaas
Member

sorpaas commented Oct 14, 2018

While the overall transaction sstore gas refund cannot go below zero, on an individual execution frame it can. This replaces saturating_sub by a signed integer version to fix the issue.

fn add_assign(&mut self, other: U256) {
match self.0 {
Sign::Positive => {
self.1 += other;

This comment has been minimized.

@tomusdrw

tomusdrw Oct 15, 2018

Contributor

should we care about overflows/panics here (and in other cases/SubAssign)?

@cheme

cheme approved these changes Oct 15, 2018

@@ -1091,7 +1091,8 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
let schedule = self.schedule;

// refunds from SSTORE nonzero -> zero
let sstore_refunds = substate.sstore_clears_refund;
assert!(substate.sstore_clears_refund.is_nonnegative(), "On transaction level, sstore clears refund cannot go below zero.");

This comment has been minimized.

@cheme

cheme Oct 15, 2018

Contributor

Setting to 0 in case of negative plus a trace could be another way to do it.

This comment has been minimized.

@sorpaas

sorpaas Oct 15, 2018

Member

I would rather panic here -- our recent experience shows that it's easier to find consensus bug this way, and if negative ever happens (which we do have some informal proof that it shouldn't), then the network is broken anyway!


/// Representation of a signed 256-bit integer.
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
pub struct I256(Sign, U256);

This comment has been minimized.

@cheme

cheme Oct 15, 2018

Contributor

I would not call it I256 but U256_And_Signed or anything that shows that we are not on 256bit only.

Show resolved Hide resolved ethcore/src/externalities.rs
@@ -1091,7 +1091,8 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
let schedule = self.schedule;

// refunds from SSTORE nonzero -> zero
let sstore_refunds = substate.sstore_clears_refund;
assert!(substate.sstore_clears_refund >= 0, "On transaction level, sstore clears refund cannot go below zero.");
let sstore_refunds = U256::from(substate.sstore_clears_refund as u64);

This comment has been minimized.

@sorpaas

sorpaas Oct 15, 2018

Member

i128::max_value() as u64 == u64::max_value(), and we checked above that it's always non-negative.

sorpaas added some commits Oct 15, 2018

@cheme

This comment has been minimized.

Contributor

cheme commented Oct 15, 2018

Seems good using i128, but I would also switch sstore_refund and others usize parameters to explicit u64.

@sorpaas

This comment has been minimized.

Member

sorpaas commented Oct 15, 2018

@cheme The issue is that all our gas definitions use usize. So if we do that it means we do another casting from usize to u64.

@cheme

This comment has been minimized.

Contributor

cheme commented Oct 15, 2018

@sorpaas yes it may be to much a change for this pr and is not strictly required (but it is something I am considering for instance if we want at sometime to compile the interpreter to wasm this usize definition is problematic).

@sorpaas sorpaas merged commit 5319d33 into master Oct 15, 2018

1 of 7 checks passed

continuous-integration/gitlab-cargo-audit Build stage: optional; status: failed
Details
continuous-integration/gitlab-test-android Build stage: optional; status: failed
Details
continuous-integration/gitlab-test-beta Build stage: optional; status: running
Details
continuous-integration/gitlab-test-darwin Build stage: optional; status: running
Details
continuous-integration/gitlab-test-nightly Build stage: optional; status: running
Details
continuous-integration/gitlab-test-windows Build stage: optional; status: running
Details
continuous-integration/gitlab-test-linux Build stage: test; status: success
Details

@sorpaas sorpaas deleted the sp-signed-refund branch Oct 15, 2018

sorpaas added a commit that referenced this pull request Oct 15, 2018

Use signed 256-bit integer for sstore gas refund substate (#9746)
* Add signed refund

* Use signed 256-bit integer for sstore gas refund substate

* Fix tests

* Remove signed mod and use i128 directly

* Fix evm test case casting

* Fix jsontests ext signature

sorpaas added a commit that referenced this pull request Oct 15, 2018

Use signed 256-bit integer for sstore gas refund substate (#9746)
* Add signed refund

* Use signed 256-bit integer for sstore gas refund substate

* Fix tests

* Remove signed mod and use i128 directly

* Fix evm test case casting

* Fix jsontests ext signature

5chdn added a commit that referenced this pull request Oct 15, 2018

Stable release 2.0.8 backports (#9748)
* parity-version: mark 2.0.8 stable as critical

* Use signed 256-bit integer for sstore gas refund substate  (#9746)

* Add signed refund

* Use signed 256-bit integer for sstore gas refund substate

* Fix tests

* Remove signed mod and use i128 directly

* Fix evm test case casting

* Fix jsontests ext signature

* Add --force to cargo audit install script (#9735)

* heads ref not present for branches beta and stable (#9741)

* aura: fix panic on extra_info with unsealed block (#9755)

* aura: fix panic when unsealed block passed to extra_info

* aura: use hex formatting for EmptyStep hashes

* aura: add test for extra_info

5chdn added a commit that referenced this pull request Oct 15, 2018

Beta release 2.1.3 backports (#9749)
* parity-version: mark 2.1.3 beta as critical

* Use signed 256-bit integer for sstore gas refund substate  (#9746)

* Add signed refund

* Use signed 256-bit integer for sstore gas refund substate

* Fix tests

* Remove signed mod and use i128 directly

* Fix evm test case casting

* Fix jsontests ext signature

* Add --force to cargo audit install script (#9735)

* heads ref not present for branches beta and stable (#9741)

* aura: fix panic on extra_info with unsealed block (#9755)

* aura: fix panic when unsealed block passed to extra_info

* aura: use hex formatting for EmptyStep hashes

* aura: add test for extra_info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment