Skip to content
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

Add state consistency check during witness generation #13412

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
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
4 changes: 4 additions & 0 deletions erigon-lib/commitment/hex_patricia_hashed.go
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,10 @@ func (c *cell) String() string {
return s
}

func (hph *HexPatriciaHashed) GetPatriciaContext() PatriciaContext {
return hph.ctx
}

func (hph *HexPatriciaHashed) PrintGrid() {
fmt.Printf("GRID:\n")
for row := 0; row < hph.activeRows; row++ {
Expand Down
62 changes: 62 additions & 0 deletions turbo/jsonrpc/eth_call.go
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,55 @@ func verifyExecResult(execResult *core.EphemeralExecResult, block *types.Block)
return nil
}

// verify the state consistency between the history state reader and the hph.Ctx reader
func verifyStateConsistency(touchedPlainKeys [][]byte, stateReader state.StateReader, hph *commitment.HexPatriciaHashed) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

add notice that this is for debug. in general ctx and state reader should have same view, so it's just comparison with another reader which may or may not share same view with hph.ctx. So when every detail with reading is cleaned out, this comparison is a self-comparison and waste of resources during witness calculation

ctx := hph.GetPatriciaContext()
for _, key := range touchedPlainKeys {
if len(key) == 20 { //account address
Copy link
Member

@shohamc1 shohamc1 Jan 14, 2025

Choose a reason for hiding this comment

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

Should throw err if len<20?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that will never happen. The keys will either be account keys (20 bytes), or storage keys (20+32=52 bytes).

Copy link
Member

Choose a reason for hiding this comment

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

that would happen during tests at least

address := libcommon.Address(key)
accountInStateReader, err := stateReader.ReadAccountData(address)
if err != nil {
return false, err
}
accountDataInTrie, err := ctx.Account(key)
if err != nil {
return false, err
}
accountInTrie := accounts.Account{
Nonce: accountDataInTrie.Nonce,
Balance: accountDataInTrie.Balance,
Root: accountDataInTrie.Storage,
CodeHash: accountDataInTrie.CodeHash,
Incarnation: accountInStateReader.Incarnation, // these 2 are copied over since they don't affect the RLP encoding
PrevIncarnation: accountInStateReader.PrevIncarnation,
}
if !accountInStateReader.Equals(&accountInTrie) {
err := fmt.Errorf("accountInStateReader(%+v) != accountInPatriciaContext(%+v)", accountInStateReader, accountInTrie)
return false, err
}

} else {
address := libcommon.Address(key[:20])
storageKey := libcommon.Hash(key[20:])
storageValInStateReader, err := stateReader.ReadAccountStorage(address, 0, &storageKey)
if err != nil {
return false, err
}

storageDataInTrie, err := ctx.Storage(storageKey[:])
if err != nil {
return false, err
}
storageValInPatriciaContext := storageDataInTrie.Storage[:]
if !bytes.Equal(storageValInStateReader, storageDataInTrie.Storage[:]) {
return false, fmt.Errorf("storageValInStateReader(%x) != storageValInPatriciaContext(%x)", storageValInStateReader, storageValInPatriciaContext)
}

}
}
return true, nil
}

func (api *BaseAPI) getWitness(ctx context.Context, db kv.RoDB, blockNrOrHash rpc.BlockNumberOrHash, txIndex hexutil.Uint, fullBlock bool, maxGetProofRewindBlockCount int, logger log.Logger) (hexutility.Bytes, error) {
roTx, err := db.BeginRo(ctx)
if err != nil {
Expand Down Expand Up @@ -574,6 +623,19 @@ func (api *BaseAPI) getWitness(ctx context.Context, db kv.RoDB, blockNrOrHash rp
touchedPlainKeys, touchedHashedKeys := store.Tds.GetTouchedPlainKeys()
codeReads := store.Tds.BuildCodeTouches()

// Verify that consistency of account and storage data for touched keys between the state reader
// and the patricia context (hph.ctx)
// If there is any inconsistency there will inevitably be a root hash mismatch later on when
// we try to construct the witness trie
isStateConsistent, err := verifyStateConsistency(touchedPlainKeys, store.Tds.StateReader, hph)
Copy link
Member

Choose a reason for hiding this comment

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

id suppose to make code block like

{
 // Verify that ....
 ...
}

so it could catch an eye that this section actually is for debugging and may be omitted.

if err != nil {
return nil, err
}

if !isStateConsistent {
return nil, fmt.Errorf("state is inconsistent: %w", err)
}

// define these keys as "updates", but we are not really updating anything, we just want to load them into the grid,
// so this is just to satisfy the current hex patricia trie api.
updates := commitment.NewUpdates(commitment.ModeDirect, sdCtx.TempDir(), hph.HashAndNibblizeKey)
Expand Down
Loading