EIP 1283: Net gas metering for SSTORE without dirty maps

[sorpaas]

This implements EIP-1283.

This implementation only modifies EVM gas costs, wasm gas costs is unaffected by eip1283Transition flag.

[cheme]

This needs a bit of comment I think. From what I understand it is to manage reentrant call by addressing the first checkpoint initial values; if it is the case renaming to first_transaction_checkpoint_index also makes sense.

[sorpaas]

This value actually just means to be transaction_checkpoint_index -- the checkpoint where if the transaction gets reverted. We set it at the first toplevel call/create executive because that checkpoint is the same as the "transaction checkpoint". I think the understanding that it's the "first checkpoint initial values" is correct, but I think it's not first_transaction_checkpoint_index.

[cheme]

is 'at last' compatible with checkpoint_index parameter.

[cheme]

I don't get this, the function is only used in reverted_storage_at to initiate origin value of the storage key, so why are we looking for the new value value of the key (cached_storage_at query first the storage changes). Maybe it is cached_original_storage_at that was meant to be called?

[sorpaas]

The checkpoint stores an account entry. When a reversion happens, it gets swapped with the current cache (or cause the current cache to get invalidated). So here, we first check whether we can get the storage key value in the checkpoint account entry, if not, then this value has not been changed, so we fetch the original value since last state commit.

[cheme]

Is it possible here that the checkpoint do not have the value because of the indexed checkpoint mechanism (I am unsure about the indexed checkpoint mechanism seeing that it was added in a later commit, I am wondering), leading to issues?

[sorpaas]

Yeah the logic was flawed there. My current fix is recursive, and I can't think of a better way right now using the current checkpointing method.

I think we need to finish the state mod refactoring before this PR lands.

[sorpaas]

I realized that I was doing the index change in wrong direction. Just fixed this. Current solution is still recursive, but it has an upper bound. So I think this would work fine without #9427. However, #9427 would still be a really nice optimization.

[cheme]

Hello sorpaas, I started to review your code but I'am a bit to unsure about the current_checkpoint_index. So I just left you a few comments.

[cheme]

The code looks is similar to your eip description (great eip by the way), but it is not that easy to ensure that all state transition are covered just by reading it.

So I find it useful to write a few additional comments for checking the algorithm:

pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, current: &U256, new: &U256) {
	let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas);
 	if current == new {
		// No refund
	} else {
		if original == current {
			if original.is_zero() {
				// No refund
			} else {
				if new.is_zero() {
					ext.inc_sstore_refund(sstore_clears_schedule);
				}
			}
		} else {
      // put in a `dirty_refund` function??
			if !original.is_zero() {
        // refund case
				if current.is_zero() {
          // a refund happened, revert refund
					ext.dec_sstore_refund(sstore_clears_schedule);
				} else if new.is_zero() {
          // refund
					ext.inc_sstore_refund(sstore_clears_schedule);
				}
			}
			if original == new {
        // reverting to init state
				if original.is_zero() {
          // revert sstore full cost (minus sload amount)
					let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas);
					ext.inc_sstore_refund(refund);
				} else {
          // revert sstore change cost (revert of refund done in previous conditions)
					let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas);
					ext.inc_sstore_refund(refund);
				}
			}
		}
	}
}

[tomusdrw]

Perhaps pattern matching here as well? Not sure if it would be more readable though.

[sorpaas]

Addressed all current round of grumbles. This PR is again ready for review!

[tomusdrw]

Great work @sorpaas! LGTM! 👍

[tomusdrw]

To be removed before merge

[tomusdrw]

I still believe it's better to move it inside the if banch

[sorpaas]

Sorry I missed this. Moved!

[cheme]

I do a fresh pass yesterday, some minor comments.

[cheme]

Could orig be initiated in this conditional branch ?

[sorpaas]

We need Ext handle for that, but right now we only need to pass schedule to the function, so I think it may not be necessary.

[cheme]

You address it in your last commit (my comment was unclear)

[cheme]

What is the first number in this comments (others are ok)? It is nice to have this test.

[sorpaas]

The first number is the original value. Added the tests!

[cheme]

This original_storage_cache behave a lot like storage_root + storage_cache. It creates some additional code, sometime a bit redundant. Maybe it is possible to create a new struct and associated method to try to reduce the size of the additional code. This comment is only a suggestion (question).

[sorpaas]

Yeah, original_storage_cache is a lot like storage_root + storage_cache. However, the usage for them are really different so I'm not sure if refactoring would save code.

And TBH, after we reviewed and merged this PR, I think the next reasonable step would be to speed up finishing #9427 -- the current checkpoint logic is not easy to follow, so moving the code would only help a little. If we make it so that account/storage changes struct are cheap to clone (through PDS), then I think checkpoint logic can be simplified a lot.

[cheme]

Here and in the next PR code, we use original_storage_root as a H256 with KECCAK_NULL_RLP as a special value. In existing code, storage_root seems to prefer use of Option<H256> (in function prototype). Personally I also prefer the use of option. For the codebase I think it would be good if both mechanism use the same thing.

[cheme]

I may have a wrong impression about the existing code (I noted that yesterday evening), still likes Option version better, but it is only a suggestion.

[sorpaas]

I think it may actually be more confusing. In account model, we currently distinguish:

• Account that does not exist. (None)
• Account exists, and storage is empty. (Some(KECCAK_NULL_RLP))
• Account exists, and storage is not empty. (Some(x) where x != KECCAK_NULL_RLP)

For the purpose of original_storage_root, we don't need to distinguish None and Some(KECCAK_NULL_RLP). So without Option it may work better here.

[cheme]

I guess it is good to keep it in mind, could be a point to consider in #9427 (Option<Option< is ugly but there is other ways).

[cheme]

Putting this block it in its own function could be possible (since you take time to separate concerns with an explicit enum). Personally I got nothing against big function, but generally code quality tools like short function with low cyclomatic complexity.

[sorpaas]

The block is actually small and mostly consists of comments. :)

I think it would be better to keep ReturnKind to be inside checkpoint_storage_at, but putting another function would mean that's not possible.

[cheme]

It's fine with me.

[cheme]

unreachable macro ? Not sure it is a good idea, personnaly I like when unexpected behavior panics but in this context it may not be the proper choice.

[sorpaas]

This should indeed be unreachable! if we're absolutely certain our code contains no bug. I think we try to make sure the logic is correct as best as we can, but if we made some mistakes elsewhere and it accidentally reached this cause, this reasoning logic provided in this cause is still correct, and we would avoid a client panic.

[cheme]

That is what I thought, as I say I prefer to fail than having an unexpected behavior, but I understand that in a production context it is not really possible. Ideally we would have a test build with panics and a production build without. A log alert trace could be a good option for such case.

[sorpaas]

Currently codecov shows that this cause is indeed not triggered by any tests. I would actually consider it more like a failsafe (it does not necessarily break consensus) -- if this cause is ever reached, returning H256::new() is the only possible correct value.

Added a warn! so that if it ever happens, we'll (hopefully) get reports!

[cheme]

The new_contract does not seems to be mandatory, it contradicts the comment of init_code, but was use this way in other test. Let's keep it.

[sorpaas]

Yeah, this is just to test whether checkpointing is working with new_contract. In reality we would not encounter new_contract called on an existing account.

[sorpaas]

Ah sorry, I was confused on this and the other test: In this case, we want to create new contracts and init code for x1, x2, y1, y2. new_contract must be called before init_code.

(Reading the code it seems indeed the case that calling new_contract is not necessary before calling init_code, but we've already done this in many other places.)

[cheme]

This makes sense to me (the replacement logic for new contracts and cache), but I do not have the background to be confident with this new behavior in the context of the evm. (I am not requesting anything, jut pointing out that my review is a bit incomplete on this part of the code).

[sorpaas]

In this case, self.storage_cache does not equal other.storage_cache. This means one of them is KECCAK_NULL_RLP. We cannot merge the storage cache here. Because this function is to "overwrite" self with other, we just move other's storage cache to self.

[cheme]

Yes the logic is sound, thanks for confirming it, I was just worried about some 'assertion' that could have been made on existing code.

[cheme]

Approved.