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

refactor!(core/types): Block method WithBody(Body) signature #110

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

qdm12
Copy link
Collaborator

@qdm12 qdm12 commented Jan 30, 2025

Breaking change refactoring core/types.Block.WithBody() method from signature

WithBody(transactions []*Transaction, uncles []*Header) *Block

to signature

WithBody(body Body) *Block

such that block and body extras can be used within WithBody.

Note

geth made the same change in method signature so the deltas introduced in this PR will disappear once we sync.

@qdm12 qdm12 force-pushed the qdm12/core/types/withbody branch from 801e855 to c0f677c Compare January 31, 2025 14:45
@qdm12 qdm12 changed the title feat(core/types): add BlockWithBodyOption variadic argument to Block.WithBody feat(core/types): block WithExtra hook and InspectDatabase variadic options Jan 31, 2025
core/rawdb/database.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/core/types/withbody branch from c0f677c to 16cfcdf Compare February 10, 2025 16:59
@qdm12 qdm12 changed the title feat(core/types): block WithExtra hook and InspectDatabase variadic options feat(core/types): block WithBody hook Feb 11, 2025
@ARR4N ARR4N changed the title feat(core/types): block WithBody hook refactor!(core/types): Block.WithBody(*Body) signature Feb 11, 2025
@qdm12 qdm12 force-pushed the qdm12/core/types/withbody branch from 16cfcdf to 9c584ab Compare February 11, 2025 10:36
@qdm12 qdm12 changed the base branch from qdm12/core/types/block-hooks-rlp to main February 11, 2025 10:36
@qdm12 qdm12 marked this pull request as ready for review February 11, 2025 10:36
cmd/evm/internal/t8ntool/block.go Outdated Show resolved Hide resolved
ethclient/ethclient.go Outdated Show resolved Hide resolved
ethclient/ethclient.go Outdated Show resolved Hide resolved
eth/downloader/downloader.go Outdated Show resolved Hide resolved
eth/downloader/downloader.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/core/types/withbody branch from aef72be to 9f1862b Compare February 11, 2025 11:26
eth/fetcher/block_fetcher.go Outdated Show resolved Hide resolved
eth/handler_eth_test.go Outdated Show resolved Hide resolved
eth/downloader/downloader.go Outdated Show resolved Hide resolved
@qdm12
Copy link
Collaborator Author

qdm12 commented Feb 11, 2025

  1. Commit 9f1862b brings the code closer to geth master branch
  2. The last commit b117d9a brings the code EVEN CLOSER to geth master branch, modifying WithBody to copy the body withdrawals and to remove some usage of WithWithdrawals (still used twice in other irrelevant places) Never mind this needs the slices package to reduce diffs, which isn't available in Go 1.20, so dropping this.

Should we keep that last commit or drop it? 🤔 Reverted it

@qdm12 qdm12 requested a review from ARR4N February 11, 2025 11:34
@qdm12
Copy link
Collaborator Author

qdm12 commented Feb 11, 2025

geth made the same change in method signature so the deltas introduced in this PR will disappear once we sync.

Thanks for this, somehow I totally omitted to write this which is kind of a big deal.

ARR4N added a commit that referenced this pull request Feb 11, 2025
## Why this should be merged

#110 introduces changes that we want to revert as part of the next Geth
sync. This label will be applied to issues that track similar tasks.

## How this works

Magic 🧙 

## How this was tested

Inspecting [CI dry run, which creates the new
label](https://github.com/ava-labs/libevm/actions/runs/13261362433/job/37018526246?pr=129#step:3:99).

Signed-off-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
@@ -843,7 +843,12 @@ func ReadBadBlock(db ethdb.Reader, hash common.Hash) *types.Block {
}
for _, bad := range badBlocks {
if bad.Header.Hash() == hash {
return types.NewBlockWithHeader(bad.Header).WithBody(bad.Body.Transactions, bad.Body.Uncles).WithWithdrawals(bad.Body.Withdrawals)
block := types.NewBlockWithHeader(bad.Header)
if bad.Body != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(remark) This suggests that they thought the old version could have a nil-pointer dereference.

eth/fetcher/block_fetcher.go Outdated Show resolved Hide resolved
Co-authored-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
Signed-off-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@qdm12 qdm12 changed the title refactor!(core/types): Block.WithBody(*Body) signature refactor!(core/types): Block method WithBody(Body) signature Feb 11, 2025
@qdm12 qdm12 enabled auto-merge (squash) February 11, 2025 14:24
@qdm12 qdm12 merged commit 67f879a into main Feb 11, 2025
4 checks passed
@qdm12 qdm12 deleted the qdm12/core/types/withbody branch February 11, 2025 14:29
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