fix: always include storage values for SLOAD and SSTORE in GethTrace

[Wodann]

The default tracer for debug_* RPC methods was not reporting warmed storage values. This deviates from Get's implementation and—seemingly—the documented expectation:

/// Represents a storage change during execution.
///
/// This maps to evm internals:
/// [JournalEntry::StorageChanged](revm::JournalEntry::StorageChanged)
///
/// It is used to track both storage change and warm load of a storage slot. For warm load in regard
/// to EIP-2929 AccessList had_value will be None.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub struct StorageChange {
    /// key of the storage slot
    pub key: U256,
    /// Current value of the storage slot
    pub value: U256,
    /// The previous value of the storage slot, if any
    pub had_value: Option<U256>,
    /// How this storage was accessed
    pub reason: StorageChangeReason,
}

In particular, the SLOAD for pc 235, 279, and 308 are missing "storage". This PR resolves the issue.

For reference:

revm-inspectors.json
geth.json

[mattsse]

one question re the removed journal change check

[mattsse]

I'm not sure if this is still unreachable now

[Wodann]

if matches!(op, opcode::SLOAD | opcode::SSTORE) on line 540 guarantees this.

[mattsse]

oh I get it now, ty

I think we need to update the snapshots now

[Wodann]

oh I get it now, ty

I think we need to update the snapshots now

I incorporated your feedback and split the solution in two parts:

1. Fix "state changes" to ensure that we log the first "storage warming"
2. Fix the GethTrace generation by inserting the complete storage state for SLOAD and SSTORE

For part 2, my understanding of geth's code is that we need to add the contract address's complete list of storage changes up to now:

	// Copy a snapshot of the current storage to a new container
	var storage Storage
	if !l.cfg.DisableStorage && (op == vm.SLOAD || op == vm.SSTORE) {
		// initialise new changed values storage container for this contract
		// if not present.
		if l.storage[contractAddr] == nil {
			l.storage[contractAddr] = make(Storage)
		}
		// capture SLOAD opcodes and record the read entry in the local storage
		if op == vm.SLOAD && stackLen >= 1 {
			var (
				address = common.Hash(stack[stackLen-1].Bytes32())
				value   = l.env.StateDB.GetState(contractAddr, address)
			)
			l.storage[contractAddr][address] = value
			storage = maps.Clone(l.storage[contractAddr]) // <-- FULL CONTRACT STORAGE IS CLONED
		} else if op == vm.SSTORE && stackLen >= 2 {
			// capture SSTORE opcodes and record the written entry in the local storage.
			var (
				value   = common.Hash(stack[stackLen-2].Bytes32())
				address = common.Hash(stack[stackLen-1].Bytes32())
			)
			l.storage[contractAddr][address] = value
			storage = maps.Clone(l.storage[contractAddr]) // <-- FULL CONTRACT STORAGE IS CLONED
		}
	}
	log.Storage = storage

Should I add more tests? If so, is there a test format that I can easily extend to generate some tests with SLOAD and SSTORE?

[Wodann]

I updated the snapshot writer logic to handle warmed storage slots. This also uncovered a (seemingly) erroneous "changed" error in some of the old snapshots. I stepped through all of the examples and I believe that the new values are in fact correct

Addressed by 7ce652d

Please let me know whether the update snapshot format is acceptable.

[Wodann]

The Option in had_value is used to signal whether this was a "warm" or a "change", as per the docs:

/// Represents a storage change during execution.
///
/// This maps to evm internals:
/// [JournalEntry::StorageChanged](revm::JournalEntry::StorageChanged)
///
/// It is used to track both storage change and warm load of a storage slot. For warm load in regard
/// to EIP-2929 AccessList had_value will be None.

[Wodann]

This maintains the old format

[Wodann]

This introduces a new format for "warmed" storage slots.

I explicitly didn't use the same format as changes, since it's possible to warm a storage slot that's set to zero. That would result in a confusing "0 → 0"

[Wodann]

I believe this was an error in the old logic.

Test 7 is a call to nest1:

function nest1() public {
    emit Log1(number, "hi from 1");
    this.nest2();
    increment();
}

function nest2() public {
    increment();
    this.nest3();
    emit Log2(number, 123, "hi from 2");
}

Prior to the call, number (storage slot: 0) is 69, due to call(Counter::setNumberCall { newNumber: U256::from(69) }.abi_encode());

1. nest1 calls nest2
2. nest2 call increment (69 → 70)
3. nest1 calls increment (70 → 71)

[mattsse]

unsure if this should be printed like this @DaniPopes

imo having

│ @ 0: 0

isnt that useful, perhaps even wrong, because nothing changed?

[mattsse]

should we keep all of this as it was and simply filter for changed values ad exclude warmed? I feel like this is separate from the fix

[Wodann]

Reported changes now contain storage warming too, so I'll need to handle it somehow. I can filter out changes with had_value.is_none() which should always and only be true for storage warming. That leaves the snapshots as-is.

Would you prefer that?

[mattsse]

I'd prefer that yes

we can still make a decision on the writer later

[Wodann]

I reverted my changes and instead ignored the storage warming "storage changes".

[Wodann]

@mattsse is there anything else I need to do before we can consider this for merging?