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

struggling with travis #2662

Merged
merged 3 commits into from
May 19, 2020
Merged

struggling with travis #2662

merged 3 commits into from
May 19, 2020

Conversation

gcolvin
Copy link
Contributor

@gcolvin gcolvin commented May 19, 2020

another try at getting this past travis

@eip-automerger eip-automerger merged commit 2661ee5 into master May 19, 2020
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.

Bad automerger!

```
Error: at pc=0, op=BEGINSUB: invalid subroutine entry
```
Consumed gas: `26`
Copy link
Contributor

Choose a reason for hiding this comment

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

@gcolvin Why did you revert this? I had updated these testcases

@holiman
Copy link
Contributor

holiman commented May 20, 2020

@gcolvin please make sure you make your changes based off the most recent master version. You just removed all the updated testcases I had fixed yesterday, plus some clarifications from #2656

@matkt
Copy link

matkt commented May 20, 2020

When you say "JUMPSUB be low, and RETURNSUB be verylow" Do you mean JUMPSUB = 5 and RETURNSUB = 3? Because I see that you have changed JUMPSUB but RETURNSUB is equal to 2 on the tests

@holiman
Copy link
Contributor

holiman commented May 20, 2020

This PR made some random changes here and there, but the PR description does not even mention what the intended changes were -- I don't know which ones were intentional and which ones were somehow just accidents during merge :/

@holiman
Copy link
Contributor

holiman commented May 20, 2020

From the YP:

Gbase 2    Amount of gas to pay for operations of the setWbase.
Gverylow 3    Amount of gas to pay for operations of the setWverylow.
Glow 5    Amount of gas to pay for operations of the setWlow.
Gmid 8    Amount of gas to pay for operations of the setWmid.

So jumpsub should be 5, according to that sentence. IMO it should be at MID=8, since that's what a JUMP is at. A JUMPSUB is more resource intensive than a JUMP since it requires an internal stack push, which JUMP doesn't.

@matkt
Copy link

matkt commented May 20, 2020

From the YP:

Gbase 2    Amount of gas to pay for operations of the setWbase.
Gverylow 3    Amount of gas to pay for operations of the setWverylow.
Glow 5    Amount of gas to pay for operations of the setWlow.
Gmid 8    Amount of gas to pay for operations of the setWmid.

So jumpsub should be 5, according to that sentence. IMO it should be at MID=8, since that's what a JUMP is at. A JUMPSUB is more resource intensive than a JUMP since it requires an internal stack push, which JUMP doesn't.

That makes sense

pizzarob pushed a commit to pizzarob/EIPs that referenced this pull request Jun 12, 2020
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
@axic axic deleted the struggle branch August 9, 2020 23:46
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
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.

4 participants