EIPs icon indicating copy to clipboard operation
EIPs copied to clipboard

eip-2315 example bytecode fix

Open radeksvarz opened this issue 3 years ago • 9 comments
trafficstars

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

radeksvarz avatar Sep 26 '22 10:09 radeksvarz

All tests passed; auto-merging...

(pass) eip-2315.md

classification
updateEIP
  • passed!

eth-bot avatar Sep 26 '22 10:09 eth-bot

Right, my bad. 

holiman avatar Sep 26 '22 15:09 holiman

On another note: the immediate is defined as uint16, so should not be referenced to as '-1'. Also, there needs to be not one but two bytes of immediate data (four nibbles instead of one) . So 0xffff,  65535 rather than -1. 

holiman avatar Sep 26 '22 15:09 holiman

On another note: the immediate is defined as uint16, so should not be referenced to as '-1'.

I believe that this case demonstrates that it should be rather defined as signed int16 - to be consistent with EIP 4200. I raised the relevant question here: https://ethereum-magicians.org/t/eip-2315-simple-subroutines-for-the-evm/3941/137

Also, there needs to be not one but two bytes of immediate data (four nibbles instead of one) . So 0xffff, 65535 rather than -1.

You are right about 0xffff. And similar mistake is for RJUMP as it should be int16. See EIP 4200: The immediate argument relative_offset is encoded as a 16-bit signed (two’s-complement) big-endian value.

I guess the resulting bytecode should be: 0x5c00045e5fffff

radeksvarz avatar Sep 26 '22 16:09 radeksvarz

I'm getting ready to travel, so am out of time to look at this, but you folks seem to have it under control, thanks. EIP-4200 came much, much later than this proposal and I'm not surprised that I failed to fully harmonize them.

gcolvin avatar Sep 26 '22 17:09 gcolvin

On another note: the immediate is defined as uint16, so should not be referenced to as '-1'.

I believe that this case demonstrates that it should be rather defined as signed int16 - to be consistent with EIP 4200. I raised the relevant question here: https://ethereum-magicians.org/t/eip-2315-simple-subroutines-for-the-evm/3941/137

At line 76 the spec already says the immediate is signed:

The relative_offset is relative to the current PC. The address is encoded as a two-byte, twos-complement signed integer, stored MSB-first.

gcolvin avatar Sep 27 '22 03:09 gcolvin

At line 76 the spec already says the immediate is signed:

And at line 69 it says otherwise. It should be int16. Or just removed, as it is redundant.

EDIT: I removed the type declaration. The text is sufficient.

gcolvin avatar Sep 27 '22 05:09 gcolvin

I have updated the PR to reflect the conclusions here.

radeksvarz avatar Oct 02 '22 16:10 radeksvarz

@gcolvin PR merge conflict was corrected, I do not know how to resolve Auto Label server error.

radeksvarz avatar Oct 03 '22 18:10 radeksvarz

This should just need an approval from one of the listed authors.

SamWilsn avatar Oct 21 '22 20:10 SamWilsn

I don't really feel comfortable being listed as one. This eip has changed a lot. 

holiman avatar Oct 21 '22 20:10 holiman

I don't really feel comfortable being listed as one. This eip has changed a lot.

I just made the approval, but it doesn't seem to be merging. I think another PR to remove you from the author list would be better given the troubles we are having with this merge.

From my point of view the basic facility hasn't really changed: it's one instruction to call a subroutine and one to return, using a return stack. The changes to the actual opcode specifications are just the move to immediate arguments, (which we always wanted), relative offsets, (to harmonize with RJUMP) and the removal of BEGINSUB, (which was not in the original proposal anyway, and I regretted adding). The validation I took from EIP-615 and added long ago. I moved it a separate EIP for a while, but it seemed better to have it all in one place. The rest is expanded motivation and rationale, not specification.

So I've vacillated on whether to change numbers, as all of the analysis and discussion so far remains relevant, and the number and the concept are pretty well-known by now. But I'd have no strong objections to renumbering and/or splitting up this proposal if you prefer.

If it's the proposal itself you don't support I'd like to discuss it with you privately first, if you don't mind.

EDIT:

Besides, the Test Cases section we are discussing here remains primarily your work. That alone rates coauthorship.

gcolvin avatar Oct 21 '22 21:10 gcolvin

Thank you so very much! I appreciate all you do! I will be sending blessings of residual significance.

On Fri, Oct 28, 2022, 12:59 PM Sam Wilson @.***> wrote:

Merged #5718 https://github.com/ethereum/EIPs/pull/5718 into master.

— Reply to this email directly, view it on GitHub https://github.com/ethereum/EIPs/pull/5718#event-7693671416, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3H6XK5UDVNALV644NC4SSLWFQIBJANCNFSM6AAAAAAQVVASV4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Cryptonics1 avatar Oct 28 '22 19:10 Cryptonics1