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

ethclient allow empty uncles #228

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Conversation

piersy
Copy link

@piersy piersy commented Sep 18, 2024

The first blocks of the celo chain (till gingerbread) lacked the uncles field, so to support
retrieving them with the ethclient.Client this modification is required.

Discovered through https://github.com/celo-org/celo-blockchain-planning/issues/363

The first blocks of the celo chain lacked the uncles field, so to
support retrieveing them with the ethclient.Client this modification is
required.
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

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.

@piersy piersy merged commit 17790d8 into celo8 Sep 19, 2024
7 checks passed
@piersy piersy deleted the piersy/ethclient-allow-empty-uncles branch September 19, 2024 11:05
karlb pushed a commit that referenced this pull request Oct 11, 2024
The first blocks of the celo chain (till gingerbread) lacked the
uncles field, so to support retrieving them with the
ethclient.Client this modification is required.
piersy added a commit that referenced this pull request Oct 15, 2024
The first blocks of the celo chain (till gingerbread) lacked the
uncles field, so to support retrieving them with the
ethclient.Client this modification is required.
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