NethermindEth / juno

Starknet client implementation.
https://juno.nethermind.io
Apache License 2.0
402 stars 170 forks source link

Refactor retrieve state history API #2197

Closed weiihann closed 1 month ago

weiihann commented 1 month ago

Let's start straight away with an example:

Block 0: Alice initially has 10 STRK Block 10: Alice now has 20 STRK Block 25: Alice now has 30 STRK

We store state history (i.e. buckets which are ContractNonceHistory, ContractStorageHistory, ContractClassHashHistory) in the following key-value format:

(prefix, contract address, block height) = old value

In our given example, let's pretend Alice is the contract address, then we will have the following key-value pairs in the database:

(prefix, Alice, 10) = 10
(prefix, Alice, 25) = 20

Ok, now we have the database in mind, let's dive into the code. Look at the func (h *history) valueAt(key []byte, height uint64) method. The first intuition of this method is that it returns the history at a given key with the given height. This intuition is further solidified by the methods that uses it, which are history.ContractStorageAt, history.ContractNonceAt and history.ContractClassHashAt (check the comments). So I would expect something like:

valueAt((prefix+Alice), 10) = 10
valueAt((prefix+Alice), 25) = 20

Now, if you look at the implementation. That's actually not the case. What the method actually does is that it returns the value at given key AFTER the given height. Let me repeat it, it is AFTER a given block height.

The original intention of doing this is that, we can represent the history of a given state for a range of block heights. So something like:

Finally, let's check if the original intention was actually used. Look at the usage of ContractClassHashAt, ContractNonceAt and ContractStorageAt. They have this pattern:

oldValue, err := s.ContractStorageAt(&addr, &key, blockNumber-1)
oldNonce, err = s.ContractNonceAt(&addr, blockNumber-1)
classHash, err = s.ContractClassHashAt(&addr, blockNumber-1) 

So actually, the usage of these methods implies that all we really want to get is just the value of a state at a given key with the given height! So why do we bother with representing the history of a given state for a range of block heights in the first place?

Hence, this PR reimplements the methods to suit their intent.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.65%. Comparing base (702f25b) to head (079afbf).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2197 +/- ## ========================================== + Coverage 78.56% 78.65% +0.08% ========================================== Files 102 102 Lines 9206 9197 -9 ========================================== + Hits 7233 7234 +1 + Misses 1343 1338 -5 + Partials 630 625 -5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

weiihann commented 1 month ago

On a second thought, maybe it just depends on you look at it:

My perspective: When I read through the code, I expect history.valueAt, history.ContractStorageAt etc would give me the old value where changes occur at a given block height, just like how it's portrayed in the database.

The previous implementation: history.valueAt implies the actual value at the given block height. So that's why we do s.ContractStorageAt(&addr, &key, blockNumber-1) because it actually does give me the value at that given block height.

Personally, I prefer the current changes that I have for this PR because its easier to understand how the history is implemented and we do only 1 db lookup instead of 2 db lookups. Furthermore, we currently have no use cases for obtaining a state at some arbitrary block height except latestHeight - 1.

weiihann commented 1 month ago

If we choose to go with the current set of changes, perhaps it's even clearer to change the StateHistoryReader interface methods to:

type StateHistoryReader interface {
    StateReader

    OldContractStorageAt(addr, key *felt.Felt, blockNumber uint64) (*felt.Felt, error)
    OldContractNonceAt(addr *felt.Felt, blockNumber uint64) (*felt.Felt, error)
    OldContractClassHashAt(addr *felt.Felt, blockNumber uint64) (*felt.Felt, error)
    OldContractIsAlreadyDeployedAt(addr *felt.Felt, blockNumber uint64) (bool, error)
}
weiihann commented 1 month ago

...stateSnapshot also implements the StateHistoryReader interface..............