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

Zero pad from calldata #1605

Merged
merged 9 commits into from
Oct 23, 2019

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Sep 8, 2019

What I did

Increase clarity and efficiency of zero-padder. Currently overlaps with #1611 - fixes #1599, #1610. See the discussion in those issues as well as #1603 (comment).

How I did it

CALLDATACOPY writes zero bytes into memory when the location in calldata is >= calldatasize.

How to verify it

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@fubuloubu fubuloubu added the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Sep 10, 2019
@charles-cooper charles-cooper changed the title WIP Zero pad from calldata Zero pad from calldata Sep 21, 2019
@charles-cooper charles-cooper removed the work in progress Work on this PR or issue is not yet complete but reviewers are free to add their input for guidance. label Sep 21, 2019
@fubuloubu
Copy link
Member

Run against ETH2.0 deposit contract

Copy link
Contributor

@jacqueswww jacqueswww left a comment

Choose a reason for hiding this comment

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

Brilliant usages of calldatacopy, this makes very happy. :) The gas savings will be insane for anybody that uses bytes.

The semantics of calldatacopy is that copying data from past-the-end
writes zero bytes to memory. So we can avoid the loop, making the code
more gas-efficient and easier to reason about.
(no longer needed with calldatacopy zeroer)
maxlen was confusingly named. maxlen referred to the static size of the
entire packed log. instead, the maxlen of the current item should be
passed to zero_pad.
It's a red herring. byte arrays should always be padded to the runtime
length and not maxlen.
Callers should make this optimization
@charles-cooper
Copy link
Member Author

I ran the eth2.0 tests and they passed, although I'm not sure that's a great indicator because they also pass under master and 0.1.0b12.

@jacqueswww
Copy link
Contributor

@charles-cooper @fubuloubu I think this can be merged?

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Are some test cases missing in our test suite, or does this refactor not implement a logical change?

@jacqueswww
Copy link
Contributor

jacqueswww commented Oct 23, 2019

Only replaces existing plumbing, with better pipes ;)

@fubuloubu fubuloubu merged commit 757b625 into vyperlang:master Oct 23, 2019
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.

Off-by-one error in zero_pad
3 participants