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

Fix #1761: potentially insufficient gas stipend for precompiled #1771

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Fix #1761: potentially insufficient gas stipend for precompiled #1771

merged 1 commit into from
Dec 17, 2019

Conversation

daejunpark
Copy link
Contributor

What I did

Fix #1761

How I did it

Put all available gas (i.e., ['gas']) for calls to precompiled contracts, especially:

  • ECREC (1)
  • ID (4)
  • ECADD (6)
  • ECMUL (7)

Note that calls to SHA256 (2) are already given the full gas.

How to verify it

Check if the changes affect only (and all of) the calls to precompiled.

Description for the changelog

Fix potentially insufficient gas stipend for precompiled contract calls

Cute Animal Picture

@fubuloubu
Copy link
Member

LGTM

Applies cleanly to b13: https://github.com/vyperlang/vyper/tree/1761-HOTFIX-v0.1.0-beta.13

NOTE: This branch will not be maintained further.

@fubuloubu
Copy link
Member

cc @djrtwo ^

@daejunpark
Copy link
Contributor Author

@fubuloubu Awesome! Just to make sure, are you going to release the hotfix branch with a version name, like 0.1.0b13.hotfix1761 so that it can be installed using pip (https://github.com/ethereum/eth2.0-specs/blob/dev/deposit_contract/requirements-testing.txt#L2)?

@jacqueswww
Copy link
Contributor

LGTM; yeah sure I think we can do that - you'd just have to pin that version in your requirements.

@fubuloubu fubuloubu merged commit 6808ecc into vyperlang:master Dec 17, 2019
@fubuloubu
Copy link
Member

fubuloubu commented Dec 17, 2019

@daejunpark an alternative is to specify this in your file:

https://github.com/vyperlang/vyper/archive/1761-HOTFIX-v0.1.0-beta.13.tar.gz

I've protected that branch so it doesn't get deleted. I would prefer this to uploading it to pypi.

@djrtwo
Copy link

djrtwo commented Dec 17, 2019

Thank you @fubuloubu. We're an inch away from finishing this formal verification and expect stability from here on out.

Really appreciate the help :)

@fubuloubu
Copy link
Member

No problem! Thank you for using Vyper for what is arguably the most important contract in the ecosystem :)

@MrChico
Copy link

MrChico commented Dec 18, 2019

@daejunpark an alternative is to specify this in your file:

https://github.com/vyperlang/vyper/archive/1761-HOTFIX-v0.1.0-beta.13.tar.gz

I've protected that branch so it doesn't get deleted. I would prefer this to uploading it to pypi.

I think it could be important to make a release as well. For services like etherscan that will need to verify the source code of this contract, it would be valuable to have this version available as a tarball.

Another thing to note is that there was no version bump included in this PR. Something like 0.1.0b13.hotfix1761 could be nice

@fubuloubu
Copy link
Member

@MrChico, that's fair

TBH this is the kind of thing I was afraid of happening, but seems unavoidable if we want the deposit contract to be more visible and widely referenced

@djrtwo
Copy link

djrtwo commented Dec 18, 2019

I'm having trouble pip installing from the tar.gz url.

Tried against a number of vyper branches and all failed. Is this an installation that has worked for anyone else?

@charles-cooper
Copy link
Member

@djrtwo maybe try this

pip install "git+https://github.com/vyperlang/vyper@1761-HOTFIX-v0.1.0-beta.13"

@fubuloubu
Copy link
Member

@djrtwo sorry, I had looked that up but I typically do it the way @charles-cooper laid out

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.

Potentially insufficient gas stipend for precompiled contract calls
6 participants