Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions core/deprecatedstate/contract.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,9 @@ func (c *ContractUpdater) UpdateStorage(diff map[felt.Felt]*felt.Felt, cb OnValu

//nolint:staticcheck // Necessary for old state
func ContractStorage(addr, key *felt.Felt, txn db.IndexedBatch) (felt.Felt, error) {
cStorage, err := storage(addr, txn)
if err != nil {
return felt.Felt{}, err
}
return cStorage.Get(key)
return trie.GetLeafPedersen(
txn, db.ContractStorage.Key(addr.Marshal()), ContractStorageTrieHeight, key,
)
}

//nolint:staticcheck // Necessary for old state
Expand Down
7 changes: 4 additions & 3 deletions core/deprecatedstate/contract_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,10 @@ func TestContractStorage(t *testing.T) {
require.NoError(t, err)
require.NoError(t, txn2.Write())

t.Run("returns error when Get() fails on corrupted trie data", func(t *testing.T) {
t.Run("corrupted root key does not affect a direct leaf read", func(t *testing.T) {
txn3 := testDB.NewIndexedBatch()
_, err := ContractStorage(addr, key, txn3)
require.Error(t, err, "should fail on corrupted trie data")
value, err := ContractStorage(addr, key, txn3)
require.NoError(t, err)
require.Equal(t, expectedValue, &value)
})
}
57 changes: 49 additions & 8 deletions core/trie/trie.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,51 @@
return newTrieReader(r, prefix, height, crypto.Poseidon)
}

// maxKeyByHeight[h] is 2^h - 1, the largest key a trie of height h can hold.
var maxKeyByHeight = func() [256]*felt.Felt {
var table [256]*felt.Felt
for h := range table {
x := felt.NewFromUint64[felt.Felt](2)
y := new(big.Int).SetUint64(uint64(h))
maxKey := x.Exp(x, y)
maxKey.Sub(maxKey, &felt.One)
table[h] = maxKey
}
return table
}()

// GetLeafPedersen reads a single leaf value by key from the flat Pedersen trie at prefix
func GetLeafPedersen(
r db.KeyValueReader,
prefix []byte,
height uint8,
key *felt.Felt,
) (felt.Felt, error) {
reader, err := newLeafReaderPedersen(r, prefix, height)
if err != nil {
return felt.Felt{}, err

Check warning on line 71 in core/trie/trie.go

View check run for this annotation

Codecov / codecov/patch

core/trie/trie.go#L71

Added line #L71 was not covered by tests
}
return reader.Get(key)
}

// newLeafReaderPedersen builds a read-only reader that skips loading the root
// key. Only Get is valid on it
func newLeafReaderPedersen(
r db.KeyValueReader,
prefix []byte,
height uint8,
) (TrieReader, error) {
if height > felt.Bits {
return TrieReader{}, fmt.Errorf("max trie height is %d, got: %d", felt.Bits, height)

Check warning on line 84 in core/trie/trie.go

View check run for this annotation

Codecov / codecov/patch

core/trie/trie.go#L84

Added line #L84 was not covered by tests
}
return TrieReader{
readStorage: NewReadStorage(r, prefix),
height: height,
maxKey: maxKeyByHeight[height],
hash: crypto.Pedersen,
}, nil
}

func newTrieReader(
r db.KeyValueReader,
prefix []byte,
Expand All @@ -56,18 +101,13 @@
return TrieReader{}, fmt.Errorf("max trie height is %d, got: %d", felt.Bits, height)
}

// maxKey is 2^height - 1
x := felt.NewFromUint64[felt.Felt](2)
y := new(big.Int).SetUint64(uint64(height))
maxKey := x.Exp(x, y)
maxKey.Sub(maxKey, &felt.One)

readStorage := NewReadStorage(r, prefix)
rootKey, err := readStorage.RootKey()
if err != nil && !errors.Is(err, db.ErrKeyNotFound) {
return TrieReader{}, err
}

maxKey := maxKeyByHeight[height]
return TrieReader{
readStorage: readStorage,
height: height,
Expand Down Expand Up @@ -458,7 +498,7 @@
}

if len(nodes) > 1 { // sibling has a parent
siblingParent := (nodes)[len(nodes)-2]
siblingParent := nodes[len(nodes)-2]

t.replaceLinkWithNewParent(sibling.key, commonKey, siblingParent)
if err := t.storage.Put(siblingParent.key, siblingParent.node); err != nil {
Expand Down Expand Up @@ -865,7 +905,8 @@
rightHash = root.RightHash.String()
}

fmt.Printf("%skey : \"%s\" path: \"%s\" left: \"%s\" right: \"%s\" LH: \"%s\" RH: \"%s\" value: \"%s\" \n",
fmt.Printf(
"%skey : \"%s\" path: \"%s\" left: \"%s\" right: \"%s\" LH: \"%s\" RH: \"%s\" value: \"%s\" \n",

Check warning on line 909 in core/trie/trie.go

View check run for this annotation

Codecov / codecov/patch

core/trie/trie.go#L908-L909

Added lines #L908 - L909 were not covered by tests
strings.Repeat("\t", level),
t.rootKey.String(),
path.String(),
Expand Down
22 changes: 15 additions & 7 deletions rpc/v10/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,22 +105,30 @@
}
defer h.callAndLogErr(stateCloser, "Error closing state reader in getStorageAt")

// This checks if the contract exists because if a key doesn't exist in contract storage,
// the returned value is always zero and error is nil.
_, err := stateReader.ContractClassHash(addressFelt)
value, err := stateReader.ContractStorage(addressFelt, key)
if err != nil {
if errors.Is(err, db.ErrKeyNotFound) {
return nil, rpccore.ErrContractNotFound
}
h.logger.Error("Failed to get contract class hash", zap.Error(err))
h.logger.Error("Failed to get contract storage", zap.Error(err))
return nil, rpccore.ErrInternal.CloneWithData(err)
Comment thread
brbrr marked this conversation as resolved.
}

result.Value, err = stateReader.ContractStorage(addressFelt, key)
if err != nil {
return nil, rpccore.ErrInternal.CloneWithData(err)
// Only the head (latest) reader returns zero for a missing contract, so probe
// the class hash to distinguish "contract absent" from "slot is zero".
// Historical and pre_confirmed readers surface it as ErrKeyNotFound above.
if value.IsZero() && id.IsLatest() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocker — the latest commit (chore: remove emptyStateReader) makes this comment false again and reintroduces the genesis pre_confirmed regression.

This guard only probes when value.IsZero() && id.IsLatest(), relying on the invariant stated in the comment above: "historical and pre_confirmed readers already surface it as ErrKeyNotFound." That held only because emptyStateReader forced the genesis pre_confirmed base to return db.ErrKeyNotFound. The last commit removed it, so baseState now returns the raw empty head state from StateAtBlockHash(&felt.Zero) (sync/preconfirmed/chain_storage.go:220), whose ContractStorage maps ErrKeyNotFound → felt.Zero, nil (core/state/state_reader.go:65-66).

Result: at genesis, starknet_getStorageAt on a pre_confirmed block for a non-existent contract returns 0x0 instead of ErrContractNotFound, because id.IsLatest() is false so the probe never runs — the exact inconsistency @RafaelGranza's option 2 set out to fix.

It also breaks testChainReaderPreConfirmedStateAtGenesisMissingContract (sync/preconfirmed/chain_storage_test.go:1067): its base mock has no ContractStorage expectation, so the pending fall-through (core/pending/state.go:69) now hits base.ContractStorage, which gomock fatals as an unexpected call — go test ./sync/preconfirmed/... goes red.

Please revert the emptyStateReader removal (restore the genesis base returning ErrKeyNotFound), or, if it must go, add id.IsPreConfirmed() back to this guard and update the comment + the genesis test.

Fix this →

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The comment above is now inaccurate, and genesis pre_confirmed returns the wrong result.

The latest commit (chore: remove emptyStateReader) fixes the test failure from the prior revision (the genesis test now asserts 0x0, nil), but the underlying behavioral inconsistency it was guarding against is back:

  • mature pre_confirmed (bottom > 0): baseStateStateAtBlockNumber(bottom-1) → historical reader → missing contract surfaces db.ErrKeyNotFound → mapped to ErrContractNotFound at line 110-112. ✅
  • genesis pre_confirmed (bottom == 0): baseStateStateAtBlockHash(&felt.Zero) → head StateReader, whose ContractStorage maps ErrKeyNotFound → felt.Zero, nil (core/state/state_reader.go:65-66). The pending fall-through (core/pending/state.go:69) forwards straight to it. Since pre_confirmed is not IsLatest(), the probe here never runs → starknet_getStorageAt returns 0x0 instead of ErrContractNotFound.

So the comment "Historical and pre_confirmed readers surface it as ErrKeyNotFound above" is false for the genesis case, and the two pre_confirmed variants now disagree — the exact inconsistency @RafaelGranza's option 2 removed.

Practically the genesis-pre_confirmed path is an extreme edge case (only while block 0 itself is unconfirmed), so this isn't a hard blocker, but it is a real spec deviation + a comment that will mislead the next maintainer. Two ways to resolve:

  1. Restore the emptyStateReader so genesis matches the historical/mature path (option 2), or
  2. Keep the removal but make the guard match reality — add id.IsPreConfirmed() back and reword the comment to note genesis returns zero for a missing contract.

As-is, the two halves are inconsistent.

if _, err := stateReader.ContractClassHash(addressFelt); err != nil {
if errors.Is(err, db.ErrKeyNotFound) {
return nil, rpccore.ErrContractNotFound
}
h.logger.Error("Failed to get contract class hash", zap.Error(err))
return nil, rpccore.ErrInternal.CloneWithData(err)

Check warning on line 126 in rpc/v10/storage.go

View check run for this annotation

Codecov / codecov/patch

rpc/v10/storage.go#L125-L126

Added lines #L125 - L126 were not covered by tests
}
}

result.Value = value

if !flags.IncludeLastUpdateBlock {
return &result, nil
}
Expand Down
58 changes: 37 additions & 21 deletions rpc/v10/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ func TestStorageAt(t *testing.T) {

t.Run("non-existent contract", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(felt.Zero, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, db.ErrKeyNotFound)

blockID := rpc.BlockIDLatest()
Expand Down Expand Up @@ -169,10 +170,29 @@ func TestStorageAt(t *testing.T) {
validateStorageAtJSON(t, result, noFlags.IncludeLastUpdateBlock)
})

t.Run("non-zero value skips the existence probe", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(7)).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)

blockID := rpc.BlockIDFromNumber(7)
result, rpcErr := handler.StorageAt(&targetAddress, &targetSlot, &blockID, noFlags)
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, result.Value)
})

t.Run("zero value on historical reader skips the existence probe", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(8)).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(felt.Zero, nil)

blockID := rpc.BlockIDFromNumber(8)
result, rpcErr := handler.StorageAt(&targetAddress, &targetSlot, &blockID, noFlags)
require.Nil(t, rpcErr)
assert.Equal(t, felt.Zero, result.Value)
})

t.Run("internal error while retrieving key", func(t *testing.T) {
internalErr := errors.New("some internal error")
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).
Return(felt.Felt{}, internalErr)

Expand All @@ -188,7 +208,6 @@ func TestStorageAt(t *testing.T) {

t.Run("blockID - latest", func(t *testing.T) {
mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)

blockID := rpc.BlockIDLatest()
Expand All @@ -204,7 +223,6 @@ func TestStorageAt(t *testing.T) {

t.Run("blockID - hash", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockHash(&felt.Zero).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)

blockID := rpc.BlockIDFromHash(&felt.Zero)
Expand All @@ -220,7 +238,6 @@ func TestStorageAt(t *testing.T) {

t.Run("blockID - number", func(t *testing.T) {
mockReader.EXPECT().StateAtBlockNumber(uint64(0)).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)

blockID := rpc.BlockIDFromNumber(0)
Expand Down Expand Up @@ -302,7 +319,6 @@ func TestStorageAt(t *testing.T) {
)
mockReader.EXPECT().Height().Return(l1HeadBlockNumber, nil)
mockReader.EXPECT().StateAtBlockNumber(l1HeadBlockNumber).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(felt.Zero, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

blockID := rpc.BlockIDL1Accepted()
Expand All @@ -320,7 +336,6 @@ func TestStorageAt(t *testing.T) {
)
mockReader.EXPECT().Height().Return(chainHeight, nil)
mockReader.EXPECT().StateAtBlockNumber(chainHeight).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&felt.Zero).Return(felt.Zero, nil)
mockState.EXPECT().ContractStorage(gomock.Any(), gomock.Any()).Return(expectedStorage, nil)

blockID := rpc.BlockIDL1Accepted()
Expand All @@ -338,7 +353,6 @@ func TestStorageAt(t *testing.T) {
lastUpdateBlockNum := uint64(3)

mockReader.EXPECT().StateAtBlockNumber(blockNumber).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)
mockState.EXPECT().ContractStorageLastUpdatedBlock(&targetAddress, &targetSlot).
Return(lastUpdateBlockNum, nil)
Expand All @@ -353,7 +367,6 @@ func TestStorageAt(t *testing.T) {
lastUpdateBlockNum := uint64(2)

mockReader.EXPECT().HeadState().Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)
mockState.EXPECT().ContractStorageLastUpdatedBlock(&targetAddress, &targetSlot).
Return(lastUpdateBlockNum, nil)
Expand All @@ -369,7 +382,6 @@ func TestStorageAt(t *testing.T) {
lastUpdateBlockNum := uint64(1)

mockReader.EXPECT().StateAtBlockHash(&blockHash).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)
mockState.EXPECT().ContractStorageLastUpdatedBlock(&targetAddress, &targetSlot).
Return(lastUpdateBlockNum, nil)
Expand All @@ -394,7 +406,6 @@ func TestStorageAt(t *testing.T) {
)
mockReader.EXPECT().Height().Return(l1HeadBlockNumber, nil)
mockReader.EXPECT().StateAtBlockNumber(l1HeadBlockNumber).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)
mockState.EXPECT().ContractStorageLastUpdatedBlock(&targetAddress, &targetSlot).
Return(lastUpdateBlockNum, nil)
Expand Down Expand Up @@ -427,7 +438,6 @@ func TestStorageAt(t *testing.T) {
mockSyncReader.EXPECT().PreConfirmedChain().Return(mustNewChain(t, &preConfirmed), nil)
mockReader.EXPECT().StateAtBlockNumber(preConfirmedBlockNumber-1).
Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).
Return(expectedStorage, nil)
mockState.EXPECT().ContractStorageLastUpdatedBlock(&targetAddress, &targetSlot).
Expand All @@ -439,7 +449,8 @@ func TestStorageAt(t *testing.T) {
assert.Equal(t, expectedStorage, result.Value)
assert.Equal(t, lastUpdateBlockNum, result.LastUpdateBlock)
validateStorageAtJSON(t, result, flags.IncludeLastUpdateBlock)
})
},
)

t.Run(
"with storage update for the target key - returns pre_confirmed block number",
Expand All @@ -452,15 +463,15 @@ func TestStorageAt(t *testing.T) {
mockSyncReader.EXPECT().PreConfirmedChain().Return(mustNewChain(t, &preConfirmed), nil)
mockReader.EXPECT().StateAtBlockNumber(preConfirmedBlockNumber-1).
Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)

preConfirmedID := rpc.BlockIDPreConfirmed()
result, rpcErr := handler.StorageAt(&targetAddress, &targetSlot, &preConfirmedID, flags)
require.Nil(t, rpcErr)
assert.Equal(t, expectedStorage, result.Value)
assert.Equal(t, preConfirmedBlockNumber, result.LastUpdateBlock)
validateStorageAtJSON(t, result, flags.IncludeLastUpdateBlock)
})
},
)

t.Run(
"new deployed contract with storage update - returns pre_confirmed block number",
Expand Down Expand Up @@ -488,7 +499,8 @@ func TestStorageAt(t *testing.T) {
assert.Equal(t, expectedStorage, result.Value)
assert.Equal(t, preConfirmedBlockNumber, result.LastUpdateBlock)
validateStorageAtJSON(t, result, flags.IncludeLastUpdateBlock)
})
},
)

t.Run(
"new deployed contract with NO storage diffs - returns 0 block number",
Expand All @@ -515,14 +527,14 @@ func TestStorageAt(t *testing.T) {
assert.Equal(t, felt.Zero, result.Value)
assert.Equal(t, uint64(0), result.LastUpdateBlock)
validateStorageAtJSON(t, result, flags.IncludeLastUpdateBlock)
})
},
)
})

t.Run("no history entry (storage never updated)", func(t *testing.T) {
blockNumber := uint64(4)

mockReader.EXPECT().StateAtBlockNumber(blockNumber).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, nil)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(expectedStorage, nil)
mockState.EXPECT().ContractStorageLastUpdatedBlock(&targetAddress, &targetSlot).
Return(uint64(0), nil)
Expand All @@ -539,7 +551,7 @@ func TestStorageAt(t *testing.T) {
dbErr := errors.New("db error")

mockReader.EXPECT().StateAtBlockNumber(blockNumber).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, dbErr)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).Return(felt.Felt{}, dbErr)

blockID := rpc.BlockIDFromNumber(blockNumber)
_, rpcErr := handler.StorageAt(
Expand All @@ -563,8 +575,10 @@ func TestStorageAt(t *testing.T) {
t.Run("error: contract not found", func(t *testing.T) {
blockNumber := uint64(99)

// Historical reader surfaces a missing contract directly from ContractStorage.
mockReader.EXPECT().StateAtBlockNumber(blockNumber).Return(mockState, nopCloser, nil)
mockState.EXPECT().ContractClassHash(&targetAddressFelt).Return(felt.Felt{}, db.ErrKeyNotFound)
mockState.EXPECT().ContractStorage(&targetAddressFelt, &targetSlot).
Return(felt.Felt{}, db.ErrKeyNotFound)

blockID := rpc.BlockIDFromNumber(blockNumber)
_, rpcErr := handler.StorageAt(
Expand Down Expand Up @@ -938,7 +952,8 @@ func TestStorageProof(t *testing.T) {
require.Equal(t, classHash, ld.ClassHash)
require.Equal(t, &expectedStorageRoot, ld.StorageRoot,
"StorageRoot should be the contract's storage trie root, not the global contracts trie root")
})
},
)
t.Run("contract storage trie address does not exist in a trie", func(t *testing.T) {
mockReader.EXPECT().BlockHeaderByNumber(blockNumber).
Return(headBlock.Header, nil)
Expand Down Expand Up @@ -1488,7 +1503,8 @@ func TestStorageProof_StorageRoots(t *testing.T) {
handler := rpc.New(bc, nil, nil, logger)
blockID := rpc.BlockIDLatest()
result, rpcErr := handler.StorageProof(
&blockID, nil, []felt.Felt{*expectedContractAddress}, nil)
&blockID, nil, []felt.Felt{*expectedContractAddress}, nil,
)
require.Nil(t, rpcErr)

expectedResult := rpc.StorageProofResult{
Expand Down
Loading
Loading