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.

Sign up for GitHub

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

Jump to bottom

accounts/abi: fix MakeTopics mutation of big.Int inputs #30785

Merged
merged 3 commits into from
Nov 25, 2024
Merged

accounts/abi: fix MakeTopics mutation of big.Int inputs #30785

merged 3 commits into from
Nov 25, 2024

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Nov 21, 2024

#28764 updated func MakeTopics to support negative *big.Ints. However, it also changed the behavior of the function from just reading the input *big.Int via Bytes(), to leveraging big.U256Bytes which is documented as being destructive:

go-ethereum/common/math/big.go

Lines 175 to 177 in a5fe735

// U256Bytes converts a big Int into a 256bit EVM number.
// This operation is destructive.
func U256Bytes(n *big.Int) []byte {

This was flagged by the race detector in our test suite, due to sharing some fixture variables for "constants" like oneEth. Looking around the codebase, other users of this function make defensive copies, like packNum:

go-ethereum/accounts/abi/pack.go

Line 81 in 2814ee0

return math.U256Bytes(new(big.Int).Set(value.Interface().(*big.Int)))

This PR updates MakeTopics to make a copy in the same way.

MariusVanDerWijden
MariusVanDerWijden approved these changes Nov 25, 2024
View reviewed changes
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.

SGTM

rjl493456442
rjl493456442 approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@rjl493456442 rjl493456442 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM,

can you also fix here? https://github.com/ethereum/go-ethereum/blob/master/signer/core/apitypes/types.go#L679

@jmank88 jmank88 requested a review from holiman as a code owner November 25, 2024 12:13
@jmank88
Copy link
Contributor Author

jmank88 commented Nov 25, 2024

LGTM,

can you also fix here? master/signer/core/apitypes/types.go#L679

Done

holiman
holiman approved these changes Nov 25, 2024
View reviewed changes
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 merged commit 3c754e2 into ethereum:master Nov 25, 2024
2 of 3 checks passed
@holiman holiman added this to the 1.14.13 milestone Nov 25, 2024
gzliudan pushed a commit to gzliudan/XDPoSChain that referenced this pull request Jan 11, 2025
@jmank88 @gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
6233400 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 14, 2025
@gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
21e079c 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 16, 2025
@gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
8c62b11 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 17, 2025
@gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
9a8674e 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 22, 2025
@gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
914d679 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 22, 2025
@gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
1406acb 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 23, 2025
@gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
097a353 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
@fjl fjl modified the milestones: 1.14.13, 1.15.0 Jan 23, 2025
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jan 24, 2025
@gzliudan
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
aa164fb 
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and
also applies the same change in signer/core/apitypes.
tokeyg pushed a commit to tokeyg/go-ethereum that referenced this pull request Feb 6, 2025
@jmank88
accounts/abi: fix MakeTopics mutation of big.Int inputs (ethereum#30785)
967edea 
ethereum#28764 updated `func MakeTopics` to support negative `*big.Int`s.
However, it also changed the behavior of the function from just
_reading_ the input `*big.Int` via `Bytes()`, to leveraging
`big.U256Bytes` which is documented as being _destructive_:

This change updates `MakeTopics` to not mutate the original, and 
also applies the same change in signer/core/apitypes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holiman holiman holiman approved these changes

@jwasinger jwasinger jwasinger approved these changes

@rjl493456442 rjl493456442 rjl493456442 approved these changes

@MariusVanDerWijden MariusVanDerWijden MariusVanDerWijden approved these changes

@gballet gballet Awaiting requested review from gballet gballet is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jmank88 @holiman @jwasinger @rjl493456442 @MariusVanDerWijden @fjl