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: fix unmarshaling of ethereum.SyncProgress #24199

Merged
merged 1 commit into from Jan 5, 2022

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Jan 5, 2022

SyncProgress was modified in PR #23576 to add the fields reported for
snap sync. The PR also changed ethclient to use the SyncProgress struct
directly instead of wrapping it for hex-decoding. This broke the
SyncProgress method.

Fix it by putting back the custom wrapper. While here, also put back the
fast sync related fields because SyncProgress is stable API and thus
removing fields is not allowed.

Fixes #24180

SyncProgress was modified in PR ethereum#23576 to add the fields reported for
snap sync. The PR also changed ethclient to use the SyncProgress struct
directly instead of wrapping it for hex-decoding. This broke the
SyncProgress method.

Fix it by putting back the custom wrapper. While here, also put back the
fast sync related fields because SyncProgress is stable API and thus
removing fields is not allowed.
@fjl fjl requested a review from karalabe January 5, 2022 14:13
@fjl fjl added this to the 1.10.15 milestone Jan 5, 2022
@@ -542,3 +542,51 @@ func toCallArg(msg ethereum.CallMsg) interface{} {
}
return arg
}

// rpcProgress is a copy of SyncProgress with hex-encoded fields.
type rpcProgress struct {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these have json tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They didn't have before because all the fields work with the default mapping, and this struct is only used for decoding.

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

@fjl fjl merged commit 0169d57 into ethereum:master Jan 5, 2022
@jclapis
Copy link

jclapis commented Jan 5, 2022

Thanks everyone!

sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Jan 6, 2022
SyncProgress was modified in PR ethereum#23576 to add the fields reported for
snap sync. The PR also changed ethclient to use the SyncProgress struct
directly instead of wrapping it for hex-decoding. This broke the
SyncProgress method.

Fix it by putting back the custom wrapper. While here, also put back the
fast sync related fields because SyncProgress is stable API and thus
removing fields is not allowed.

Fixes ethereum#24180
Fixes ethereum#24176
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
SyncProgress was modified in PR ethereum#23576 to add the fields reported for
snap sync. The PR also changed ethclient to use the SyncProgress struct
directly instead of wrapping it for hex-decoding. This broke the
SyncProgress method.

Fix it by putting back the custom wrapper. While here, also put back the
fast sync related fields because SyncProgress is stable API and thus
removing fields is not allowed.

Fixes ethereum#24180
Fixes ethereum#24176
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.

SyncProgress() fails unmarshalling in Geth v1.10.14
4 participants