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

trie: remove owner and binary marshaling from stacktrie #28291

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Oct 9, 2023

This is a (potential) follow-up to #28233. This change

  1. removes the owner-notion from a stacktrie; the owner is only ever needed for comitting to the database, but the commit-function, the writeFn is provided by the caller, so the caller can just set the owner into the writeFn instead of having it passed through the stacktrie.
  2. Removes the encoding.BinaryMarshaler/encoding.BinaryUnmarshaler interface from stacktrie. We're not using it, and I doubt anyone downstream is either.

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

Left nitpick comments

eth/protocols/snap/sync.go Outdated Show resolved Hide resolved
eth/protocols/snap/sync.go Outdated Show resolved Hide resolved
eth/protocols/snap/sync.go Outdated Show resolved Hide resolved
eth/protocols/snap/sync.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member

Again, we should run snap sync before merging, just ensure.

@holiman
Copy link
Contributor Author

holiman commented Oct 10, 2023

running this on bench07 now

@holiman holiman added this to the 1.13.3 milestone Oct 10, 2023
@holiman holiman merged commit 8976a0c into ethereum:master Oct 11, 2023
2 checks passed
@holiman holiman deleted the refactor_stacktrie_pt2 branch October 11, 2023 07:23
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
This change
  - Removes the owner-notion from a stacktrie; the owner is only ever needed for comitting to the database, but the commit-function, the `writeFn` is provided by the caller, so the caller can just set the owner into the `writeFn` instead of having it passed through the stacktrie.
  - Removes the `encoding.BinaryMarshaler`/`encoding.BinaryUnmarshaler` interface from stacktrie. We're not using it, and it is doubtful whether anyone downstream is either.
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 16, 2023
This change
  - Removes the owner-notion from a stacktrie; the owner is only ever needed for comitting to the database, but the commit-function, the `writeFn` is provided by the caller, so the caller can just set the owner into the `writeFn` instead of having it passed through the stacktrie.
  - Removes the `encoding.BinaryMarshaler`/`encoding.BinaryUnmarshaler` interface from stacktrie. We're not using it, and it is doubtful whether anyone downstream is either.
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This change
  - Removes the owner-notion from a stacktrie; the owner is only ever needed for comitting to the database, but the commit-function, the `writeFn` is provided by the caller, so the caller can just set the owner into the `writeFn` instead of having it passed through the stacktrie.
  - Removes the `encoding.BinaryMarshaler`/`encoding.BinaryUnmarshaler` interface from stacktrie. We're not using it, and it is doubtful whether anyone downstream is either.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Dec 19, 2023
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