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.

Sign up for GitHub

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

Jump to bottom

ethclient allow empty uncles #228

Merged
merged 2 commits into from
Sep 19, 2024
Merged

ethclient allow empty uncles #228

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
693e7f4
Allow ethclient to fetch blocks with empty uncles
piersy Sep 18, 2024
30aeff9
Merge branch 'celo8' into piersy/ethclient-allow-empty-uncles
piersy Sep 19, 2024
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
5 changes: 4 additions & 1 deletion ethclient/ethclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,10 @@ func (ec *Client) getBlock(ctx context.Context, method string, args ...interface
if head.UncleHash == types.EmptyUncleHash && len(body.UncleHashes) > 0 {
return nil, errors.New("server returned non-empty uncle list but block header indicates no uncles")
}
if head.UncleHash != types.EmptyUncleHash && len(body.UncleHashes) == 0 {
// In celo before the ginerbread hardfork, blocks had no uncles hash and no
// uncles, so in those cases it is legitimate to have an empty uncles hash.
var emptyHash common.Hash
Copy link

Choose a reason for hiding this comment

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

Should we allow this only in a specific range of block numbers?

Copy link
Author

Choose a reason for hiding this comment

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

That would be nice but I don't see any way to do it without some fairly invasive changes to the interface. Let me know if you see a way to do this neatly.

Copy link

@palango palango Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I though of something like this, just to limit the scope of the check:

	var emptyHash common.Hash
	if head.UncleHash != emptyHash  && len(body.UncleHashes) == 0 && head.Number.Cmp(big.NewInt(12345)) < 0 {
		return nil, errors.New("server returned empty uncle list but block header indicates uncles")
	}

That would also require that we know in which block the first uncle is written.

Copy link
Author

Choose a reason for hiding this comment

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

So yeah, that is the thing getting the knowledge of the block (gingerbread block) to the right place, as I see it either you pass it through the constructor or as a parameter to the method, but either way, you'd be looking at a lot of changes.

Copy link

Choose a reason for hiding this comment

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

Ok, now I get it.

The first blocks of the celo chain lacked the uncles field

It sounded like it was only for the first couple of blocks, not until gingerbread.

if head.UncleHash != emptyHash && head.UncleHash != types.EmptyUncleHash && len(body.UncleHashes) == 0 {
return nil, errors.New("server returned empty uncle list but block header indicates uncles")
}
if head.TxHash == types.EmptyTxsHash && len(body.Transactions) > 0 {
Expand Down