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

tx/vm/util: throw when provided negative BN #1606

Merged
merged 24 commits into from
Dec 15, 2021

Conversation

gabrocheleau
Copy link
Contributor

@gabrocheleau gabrocheleau commented Dec 12, 2021

This PR resolves #1513 with the following changes:

  • tx package: Throw when constructing a Transaction with a negative BigNumber value (in toBuffer)
  • vm package: Throw when passed a negative call value (throws in the Message constructor)

As mentioned in a comment of the above issue, the toBuffer method also has the side effect of converting negative BN to positive BN. I decided to throw an error in the case of negative BN in that method, since it seemed to me like that behavior isn't intended.

I was not sure about disabling negative BNs at the module level, so omitted that for now.

@codecov
Copy link

codecov bot commented Dec 12, 2021

Codecov Report

Merging #1606 (420328c) into master (d390b9b) will increase coverage by 0.32%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 84.96% <ø> (ø)
blockchain 85.08% <ø> (?)
client 71.27% <ø> (ø)
common 100.00% <ø> (ø)
devp2p 82.82% <ø> (-0.21%) ⬇️
ethash ∅ <ø> (?)
trie 86.09% <ø> (ø)
tx 88.62% <ø> (ø)
util 91.84% <100.00%> (+0.03%) ⬆️
vm 79.89% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@gabrocheleau gabrocheleau marked this pull request as ready for review December 12, 2021 04:44
@gabrocheleau gabrocheleau force-pushed the vm/tx/remove-negative-bn-support branch from 72aa5a9 to 7061f77 Compare December 12, 2021 20:16
@ryanio
Copy link
Contributor

ryanio commented Dec 14, 2021

i'm seeing uses of both "can not" and "cannot" in this PR, let's just use "cannot"

Both cannot and can not are perfectly fine, but cannot is far more common and is therefore recommended, especially in any kind of formal writing.
https://www.merriam-webster.com/words-at-play/cannot-vs-can-not-is-there-a-difference

@ryanio
Copy link
Contributor

ryanio commented Dec 15, 2021

lgtm! will wait if you want to try to add a negative value tx to runTx (using value=0 and tx.value.isubn(1) as jochem suggested)

gabrocheleau and others added 2 commits December 15, 2021 15:23
@ryanio ryanio changed the title tx/vm: throw when provided negative BN tx/vm/util: throw when provided negative BN Dec 15, 2021
gabrocheleau and others added 2 commits December 15, 2021 15:48
Co-authored-by: Ryan Ghods <ryan@ryanio.com>
Co-authored-by: Ryan Ghods <ryan@ryanio.com>
Co-authored-by: Ryan Ghods <ryan@ryanio.com>
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

looks great! can merge after ci passes

@gabrocheleau gabrocheleau merged commit 59f103e into master Dec 15, 2021
@gabrocheleau gabrocheleau deleted the vm/tx/remove-negative-bn-support branch December 15, 2021 21:27
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.

Negative BNs causing weird situations as txs with negative value
3 participants