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

refactor: optimize calldatasize check #3104

Merged

Conversation

emc415
Copy link
Contributor

@emc415 emc415 commented Sep 23, 2022

What I did

This wip commit addresses the first half of inefficient selector table checks (issue #3037)

How I did it

I added a function which tests whether a contract’s function has a selector with trailing zeroes (wip: at the moment it catches all selectors which contain 0)
It then changes the IR created in def _runtime_ir() to only have a line check in IR for calldatasize >= 4 if there is a zero selector function in the contract.

How to verify it

Commit message

Addresses the first half of inefficient selector table checks (issue #3037) by only running a check for calldatasize >= 4 if a zero selector function is present.

Description for the changelog

only check for calldatasize >= 4 if zero selector function is present.

Cute Animal Picture

which is the animal? :D

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

Addresses the first half of inefficient selector table checks (issue vyperlang#3037) by only running a check for calldatasize >= 4 if a zero selector function is present.
(wip: currently checks on all selectors which contain a 0)
@emc415
Copy link
Contributor Author

emc415 commented Sep 23, 2022

I figured out why my zero selector function wasn't getting pulled (and why I hacked it by finding any zero instead of trailing zeroes): it's because str(f._metadata["signature"]) is not pulling the abi_signature, just the signature. So for example, "def BlockHashAskewLimitary():" is getting hashed instead of "BlockHashAskewLimitary". Currently working on finding the correct function in the codebase to convert it to an abi signature.

@emc415 emc415 marked this pull request as draft September 23, 2022 15:59
fixed by passing the correct abi_sig instead of a FunctionSignature object.
now correctly generates method_id and identifies zero selector functions
(any function where selector starts with a 0)
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

thanks for taking this on, nice work so far -- left a few comments in the review

vyper/codegen/module.py Outdated Show resolved Hide resolved
vyper/codegen/module.py Outdated Show resolved Hide resolved
vyper/codegen/module.py Outdated Show resolved Hide resolved
vyper/codegen/module.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Merging #3104 (703021d) into master (61deca9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3104   +/-   ##
=======================================
  Coverage   88.43%   88.44%           
=======================================
  Files          88       88           
  Lines       11046    11049    +3     
  Branches     2338     2339    +1     
=======================================
+ Hits         9769     9772    +3     
  Misses        822      822           
  Partials      455      455           
Impacted Files Coverage Δ
vyper/codegen/module.py 94.39% <ø> (ø)
.../codegen/function_definitions/external_function.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…lang#3037)

changed import and associated function calls
updated check for zero selector functions
removed/updated comments
…g#3037)

Only performs a calldatasize == 4 check when there are trailing 0s in selector

wip: figure out whether to check hex, int, or bytecode of selector
streamlined creation of runtime list in _runtime_ir()
@emc415 emc415 marked this pull request as ready for review October 7, 2022 02:12
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 29, 2022

This pull request introduces 1 alert when merging b8ba472 into b13595f - view on LGTM.com

new alerts:

  • 1 for Unused import

@fubuloubu fubuloubu marked this pull request as draft October 31, 2022 18:12
@fubuloubu fubuloubu changed the title wip: feat: optimize calldatasize check (#3037) feat: optimize calldatasize check (#3037) Oct 31, 2022
@fubuloubu
Copy link
Member

@emc415 just convert back from Draft when ready (it's on the right hand side)

@emc415 emc415 force-pushed the feat/optimize_calldatasize_check branch from a723860 to a79edba Compare November 7, 2022 19:34
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

this is looking pretty good. will try to merge in later this or next week.

@charles-cooper charles-cooper changed the title feat: optimize calldatasize check (#3037) feat: optimize calldatasize check Nov 22, 2022
@charles-cooper
Copy link
Member

looking like there's a test failure in ctor assembly

@charles-cooper
Copy link
Member

The failing test case is a little weird; it depends on JUMP being in the opcodes output to demarcate the initcode from the runtime code. I would instead use the assembly output which is a little more structured.

@emc415 emc415 marked this pull request as ready for review December 30, 2022 22:00
@charles-cooper
Copy link
Member

looks decent. @fubuloubu what do you think?

@charles-cooper
Copy link
Member

also tagging @wadealexc in case you want to leave any feedback.

@fubuloubu fubuloubu changed the title feat: optimize calldatasize check refactor: optimize calldatasize check Jan 3, 2023
@charles-cooper charles-cooper merged commit 02339df into vyperlang:master Jan 20, 2023
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