Skip to content

Commit

Permalink
partial hash check for mock_sentry.go (#11964)
Browse files Browse the repository at this point in the history
> I encountered a problem in the way we run execution-spec-tests, and
need more eyes on it:
some execution spec tests have a failure associated with a block. An
example is: a 7702 tx with empty auth list should cause block rejection
because such a tx is invalid.
now when the BlockTest#Run executes, it tries to "insertChain", after
which mock_sentry does block.HashCheck() before calling the el to insert
block.
one of the check in HashCheck() is that if receiptRoot is emptyRootHash
and block has any tx, it rejects the block right there, even before
sending it to el (lots if not all execution-spec-tests wherein block
should be rejected have receiptRoot=emptyRootHash)
The execution-spec-test in the above scenario passes, since the block
was "rejected". But this can (and does) lead to false positive
test-success, as the block is never executed, so we don't actually check
if the block is rejected "for correct reasons". So for example, recently
Mario added a test to check "tx should not be nil in 7702 tx" in latest
spec tests. This is not done by our code, but the test still passes
because the rejection happens at block.HashCheck()


to counter these false positives, I add a `fullCheck` flag for
`HashCheck` which bypasses the empty receipt check for mock_sentry.
  • Loading branch information
sudeepdino008 authored Sep 12, 2024
1 parent a5dd9d8 commit 372e62c
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 8 deletions.
11 changes: 8 additions & 3 deletions core/types/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -1412,13 +1412,18 @@ func (b *Block) SanityCheck() error {
}

// HashCheck checks that transactions, receipts, uncles, withdrawals, and requests hashes are correct.
func (b *Block) HashCheck() error {
func (b *Block) HashCheck(fullCheck bool) error {
if hash := DeriveSha(b.Transactions()); hash != b.TxHash() {
return fmt.Errorf("block has invalid transaction hash: have %x, exp: %x", hash, b.TxHash())
}

if len(b.transactions) > 0 && b.ReceiptHash() == EmptyRootHash {
return fmt.Errorf("block has empty receipt hash: %x but it includes %x transactions", b.ReceiptHash(), len(b.transactions))
if fullCheck {
// execution-spec-tests contain such scenarios where block has an invalid tx, but receiptHash is default (=EmptyRootHash)
// the test is to see if tx is rejected in EL, but in mock_sentry.go, we have HashCheck() before block execution.
// Since we want the tx execution to happen, we skip it here and bypass this guard.
if len(b.transactions) > 0 && b.ReceiptHash() == EmptyRootHash {
return fmt.Errorf("block has empty receipt hash: %x but it includes %x transactions", b.ReceiptHash(), len(b.transactions))
}
}

if len(b.transactions) == 0 && b.ReceiptHash() != EmptyRootHash {
Expand Down
2 changes: 1 addition & 1 deletion p2p/sentry/sentry_multi_client/sentry_multi_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ func (cs *MultiClient) newBlock66(ctx context.Context, inreq *proto_sentry.Inbou
if err := request.SanityCheck(); err != nil {
return fmt.Errorf("newBlock66: %w", err)
}
if err := request.Block.HashCheck(); err != nil {
if err := request.Block.HashCheck(true); err != nil {
return fmt.Errorf("newBlock66: %w", err)
}

Expand Down
2 changes: 1 addition & 1 deletion polygon/sync/blocks_verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func VerifyBlocks(blocks []*types.Block) error {
return err
}

if err := block.HashCheck(); err != nil {
if err := block.HashCheck(true); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion turbo/app/import_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ func insertPosChain(ethereum *eth.Ethereum, chain *core.ChainPack, logger log.Lo
}

for i := posBlockStart; i < chain.Length(); i++ {
if err := chain.Blocks[i].HashCheck(); err != nil {
if err := chain.Blocks[i].HashCheck(true); err != nil {
return err
}
}
Expand Down
4 changes: 2 additions & 2 deletions turbo/stages/mock/mock_sentry.go
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ func (ms *MockSentry) insertPoWBlocks(chain *core.ChainPack) error {
return nil
}
for i := 0; i < chain.Length(); i++ {
if err := chain.Blocks[i].HashCheck(); err != nil {
if err := chain.Blocks[i].HashCheck(false); err != nil {
return err
}
}
Expand Down Expand Up @@ -767,7 +767,7 @@ func (ms *MockSentry) insertPoSBlocks(chain *core.ChainPack) error {

ctx := context.Background()
for i := n; i < chain.Length(); i++ {
if err := chain.Blocks[i].HashCheck(); err != nil {
if err := chain.Blocks[i].HashCheck(false); err != nil {
return err
}
}
Expand Down

0 comments on commit 372e62c

Please sign in to comment.