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

eth/downloader, core/types: take withdrawals-size into account in downloader queue #30276

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

psogv0308
Copy link
Contributor

This PR addresses an issue where the size of the downloader queue was not accurately measured. Previously, the size of withdrawals was not included in the queue size calculation, leading to an accumulation of more items than originally intended. This simple fix ensures that withdrawals are now considered, providing an accurate measurement of the queue size.

@@ -42,6 +43,10 @@ type withdrawalMarshaling struct {
Amount hexutil.Uint64
}

func (w Withdrawal) Size() common.StorageSize {
return common.StorageSize(unsafe.Sizeof(w))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid the use of package unsafe here. Since withdrawals are defined by the protocol and will never change, we can just hard-code the returned size here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var withdrawalSize = common.StorageSize(reflect.TypeOf(Withdrawal{}).Size())

func (w Withdrawal) Size() common.StorageSize {
	return common.StorageSize(withdrawalSize)
}

What do think about handling it like this, similar to the code in other parts?

@psogv0308 psogv0308 requested a review from fjl August 7, 2024 16:12
Comment on lines 388 to 390
for _, withdrawal := range result.Withdrawals {
size += withdrawal.Size()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since withdrawals are constant size, perhaps put Size on Withdrawals instead, and do

size += common.StorageSize(result.Withdrawals.Size())

(yes, make the conversion to StorageSize here, not in the Size-function)

Then the change in types boils down to

var withdrawalSize = int(reflect.TypeOf(Withdrawal{}).Size())

func (s Withdrawals) Size() int {
	return withdrawalSize * len(s)
}

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman changed the title eth/downloader, core/types: adding withdrawal size to downloader queue eth/downloader, core/types: take withdrawal size into account for downloader queue size Aug 8, 2024
@holiman holiman changed the title eth/downloader, core/types: take withdrawal size into account for downloader queue size eth/downloader, core/types: take withdrawals-size into account in downloader queue Aug 8, 2024
@holiman holiman added this to the 1.14.8 milestone Aug 8, 2024
@holiman holiman merged commit ebe31df into ethereum:master Aug 8, 2024
3 checks passed
rjl493456442 pushed a commit to rjl493456442/go-ethereum that referenced this pull request Aug 13, 2024
…nloader queue (ethereum#30276)

Fixes a slight miscalculation in the downloader queue, which was not accurately taking block withdrawals into account when calculating the size of the items in the queue
leeren pushed a commit to storyprotocol/story-geth that referenced this pull request Aug 16, 2024
…nloader queue (ethereum#30276)

Fixes a slight miscalculation in the downloader queue, which was not accurately taking block withdrawals into account when calculating the size of the items in the queue
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.

4 participants