Skip to content

core: Check whether zero uncles check in verifyUncles is redundant#34014

Closed
kevaundray wants to merge 2 commits into
ethereum:masterfrom
kevaundray:kw/check-extra-uncle-guard
Closed

core: Check whether zero uncles check in verifyUncles is redundant#34014
kevaundray wants to merge 2 commits into
ethereum:masterfrom
kevaundray:kw/check-extra-uncle-guard

Conversation

@kevaundray
Copy link
Copy Markdown
Contributor

Pulling this out from comment thread here

@kevaundray kevaundray changed the title chore(do not merge): Check whether zero uncles check in verifyUncles is redundant core: Check whether zero uncles check in verifyUncles is redundant Mar 16, 2026
@kevaundray
Copy link
Copy Markdown
Contributor Author

@rjl493456442 Since Test (1.24) passes*, I believe it shows that the check is not needed in ValidateBody. I also ran a unit test locally to check this (that VerifyHeader catches the empty uncle hash and validateBody checks that they match).

Would delegate to you as to how you think we should handle this.

*appveyor fails on master, so I'm assuming we can just ignore it

Copy link
Copy Markdown
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Just checked the coverage and it turns out that this file has 0% coverage 🤯 so just passing the tests isn't enough.

@kevaundray
Copy link
Copy Markdown
Contributor Author

Just checked the coverage and it turns out that this file has 0% coverage 🤯 so just passing the tests isn't enough.

I had assumed that this would be tested via EEST, though I also asked claude to make a test case locally while CI was working:

func TestPostMergeUncleRejection(t *testing.T) {
	// Generate a small post-merge chain
	gspec := &Genesis{Config: params.TestChainConfig}
	engine := beacon.New(ethash.NewFaker())
	_, blocks, _ := GenerateChainWithGenesis(gspec, engine, 2, func(i int, gen *BlockGen) {
		gen.SetPoS()
	})

	options := DefaultConfig().WithStateScheme(rawdb.HashScheme)
	chain, err := NewBlockChain(rawdb.NewMemoryDatabase(), gspec, engine, options)
	if err != nil {
		t.Fatal(err)
	}
	defer chain.Stop()

	// Insert the first block so we have a parent
	if _, err := chain.InsertChain(blocks[:1]); err != nil {
		t.Fatal(err)
	}
	goodBlock := blocks[1]
	uncle := &types.Header{
		ParentHash: blocks[0].Hash(),
		Number:     big.NewInt(2),
		Time:       goodBlock.Time(),
	}
	bodyWithUncle := types.Body{
		Transactions: goodBlock.Transactions(),
		Uncles:       []*types.Header{uncle},
		Withdrawals:  goodBlock.Withdrawals(),
	}

	// Case 1: Header with non-empty uncle hash.
	// VerifyHeader should reject because UncleHash != EmptyUncleHash post-merge.
	badHeader := *goodBlock.Header()
	badHeader.UncleHash = types.CalcUncleHash(bodyWithUncle.Uncles)
	if err := engine.VerifyHeader(chain, &badHeader); err == nil {
		t.Error("VerifyHeader accepted post-merge block with non-empty uncle hash")
	}

	// Case 2: Block with uncles in body BUT EmptyUncleHash in header (dishonest header).
	// ValidateBody should reject because CalcUncleHash(uncles) != header.UncleHash.
	dishonestBadBlock := goodBlock.WithBody(bodyWithUncle)
	if dishonestBadBlock.Header().UncleHash != types.EmptyUncleHash {
		t.Fatal("test setup: expected EmptyUncleHash in dishonest block header")
	}
	if err := chain.validator.ValidateBody(dishonestBadBlock); err == nil {
		t.Error("ValidateBody accepted post-merge block with uncles in body but EmptyUncleHash in header")
	}
}

Then played around with the code to ensure I wasn't missing anything

@kevaundray
Copy link
Copy Markdown
Contributor Author

Closing alongside #34007

@kevaundray kevaundray closed this Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants