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

core, trie, triedb: port changes from the snapshot integration #30599

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Oct 15, 2024

This pull request ports some non-important changes from the #29754 , including interface rename, refactoring, etc.

@@ -558,7 +556,4 @@ func (h *handler) enableSyncedFeatures() {
log.Info("Snap sync complete, auto disabling")
h.snapSync.Store(false)
}
if h.chain.TrieDB().Scheme() == rawdb.PathScheme {
Copy link
Member Author

Choose a reason for hiding this comment

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

The buffer flushing can be performed in the background, see #30464 for more details.

This buffer adjustment is not relevant anymore

@rjl493456442 rjl493456442 force-pushed the snapshot-integration-p1 branch from 36e3eb9 to 2d59745 Compare October 15, 2024 08:32
triedb/pathdb/buffer.go Outdated Show resolved Hide resolved
triedb/pathdb/nodes.go Outdated Show resolved Hide resolved
@rjl493456442 rjl493456442 force-pushed the snapshot-integration-p1 branch 2 times, most recently from 2b84983 to ed975de Compare October 15, 2024 12:37
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.

These changes seem fairly straight-forward to me.

@holiman
Copy link
Contributor

holiman commented Oct 15, 2024

This pull request ports some non-important changes from the #29754 , including interface rename, refactoring, etc.

Don't you mean changes from #30159 ?

return err
}
log.Debug("Journaled pathdb diff layer", "root", dl.root, "parent", dl.parent.rootHash(), "id", dl.stateID(), "block", dl.block, "nodes", len(dl.nodes))
log.Debug("Journaled pathdb diff layer", "root", dl.root, "parent", dl.parent.rootHash(), "id", dl.stateID(), "block", dl.block)
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Github won't let me add comments to stuff you didn't change, Just wanted to add a note that the usage of disk.buffer.layers in Journal might be a bit racy, but its okay since its for logging output only

Copy link
Member Author

Choose a reason for hiding this comment

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

Which line? We don't use disk.buffer.layers here?

Copy link
Contributor

Choose a reason for hiding this comment

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

a bit racy, but its okay since its for logging output only

No, actually that's not okay. If it's racy, then it's a problem (?)

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden Oct 16, 2024

Choose a reason for hiding this comment

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

Line 237 and Line 239
There we use disk.buffer.layers without acquiring the journal lock first. All other instance where disk.buffer is used, we acquire the locks first before using it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, 237 and 239 is in the Journal function, which is only called when the Geth is terminated.

I will try to fix it in a following PR

@rjl493456442 rjl493456442 force-pushed the snapshot-integration-p1 branch from ed975de to ef42ebb Compare October 16, 2024 06:43
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden 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 added this to the 1.14.12 milestone Oct 18, 2024
@holiman holiman merged commit b6c62d5 into ethereum:master Oct 18, 2024
3 checks passed
holiman pushed a commit that referenced this pull request Nov 19, 2024
This change ports some non-important changes from #30159, including interface renaming and some trivial refactorings.
zfy0701 pushed a commit to sentioxyz/go-ethereum that referenced this pull request Dec 3, 2024
…#30599)

This change ports some non-important changes from ethereum#30159, including interface renaming and some trivial refactorings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants