Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

Implement eip1380 (call-to-self) in LegacyVM#5752

Merged
gumb0 merged 8 commits intomasterfrom
eip1380-callself
Oct 2, 2019
Merged

Implement eip1380 (call-to-self) in LegacyVM#5752
gumb0 merged 8 commits intomasterfrom
eip1380-callself

Conversation

@halfalicious
Copy link
Copy Markdown
Contributor

@halfalicious halfalicious commented Sep 20, 2019

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1380.md

Charge a reduced gas fee (40 vs 700) for calls (call/callcode/delegatecall/staticcall) to functions within the same contract.

Add unit tests for both call-to-self and non-call-to-self cases (for call/callcode/delegatecall/staticcall)

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 20, 2019

Codecov Report

Merging #5752 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5752      +/-   ##
==========================================
+ Coverage    63.9%   64.01%   +0.11%     
==========================================
  Files         359      359              
  Lines       30667    30770     +103     
  Branches     3410     3417       +7     
==========================================
+ Hits        19597    19698     +101     
- Misses       9841     9843       +2     
  Partials     1229     1229

@halfalicious
Copy link
Copy Markdown
Contributor Author

Need to move changes to Berlin schedule once #5754 is merged

@halfalicious halfalicious requested review from chfast and gumb0 and removed request for gumb0 September 22, 2019 21:48
@halfalicious halfalicious marked this pull request as ready for review September 22, 2019 21:48
@halfalicious halfalicious changed the title Implement eip1380 in LegacyVM Implement eip1380 (call-to-self) in LegacyVM Sep 22, 2019
@halfalicious halfalicious force-pushed the eip1380-callself branch 3 times, most recently from 317a70e to 4a1ee51 Compare September 25, 2019 05:19
@halfalicious
Copy link
Copy Markdown
Contributor Author

cc @gumb0

@halfalicious
Copy link
Copy Markdown
Contributor Author

Marking as in-progress since I'm adding call tests (non-call-to-self cases) as a part of this PR

@halfalicious halfalicious force-pushed the eip1380-callself branch 3 times, most recently from 9c893f1 to 62c1bc2 Compare September 29, 2019 21:16
@halfalicious
Copy link
Copy Markdown
Contributor Author

Rebased + squashed some commits

@halfalicious
Copy link
Copy Markdown
Contributor Author

@gumb0 / @chfast These changes are ready for review again. Some things to call out:

  • Calls-to-self which include a value transfer are charged the value-transfer gas cost (currently 9000) - the call stipend (currently 2300). This seems wrong to me since I'd expect this to be a NOOP at the VM level and therefore I don't think there should be any gas cost for this but this is existing behavior so I'm not going to change it (since that would cause consensus issues).
  • There's a bit of code duplication in the call tests - I don't think addressing this is high priority, especially since I've spent a good chunk of time on the tests already. I could potentially address this in the future if we decide cleaning up the test code is important.

Charge a reduced gas fee for calls to functions within the same contract
The tests exercise CALL/CALLCODE/DELEGATECALL/STATICCALL
Remove need for eip1380 schedule flag

Simplify gas adjustment for call-self case

Make minor updates to call self VM test
@halfalicious
Copy link
Copy Markdown
Contributor Author

Rebased + moved EIP to Berlin hardfork

vm->exec(gas, extVm, onOp);
// Subtract the call stipend gas since there's no code at the address being called
unsigned int gasExpected =
extVm.evmSchedule().callValueTransferGas - extVm.evmSchedule().callStipend;
Copy link
Copy Markdown
Member

@gumb0 gumb0 Oct 1, 2019

Choose a reason for hiding this comment

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

I'm a bit lost about this. Can you explain how it can cost less than transfer gas?

Copy link
Copy Markdown
Contributor Author

@halfalicious halfalicious Oct 2, 2019

Choose a reason for hiding this comment

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

@gumb0 Yes - according to the definition of the call stipend (gcallstipend) in the YP, the call stipend is subtracted from the value transfer gas (for a non-zero value transfer). Additionally, the call stipend is added to the gas being passed to the call and its purpose is to ensure that there's just enough gas for the contract's fallback function to perform some basic steps (e.g. log something), but not enough for it to do something nefarious. Also, gas not used in a contract call is refunded to the caller. So, since there's no code at the addresses being called in my test cases, there's no fallback function to execute and therefore the stipend is refunded

@gumb0 gumb0 merged commit 33cc6cc into master Oct 2, 2019
@gumb0 gumb0 deleted the eip1380-callself branch October 2, 2019 10:51
auto const destinationAddr = asAddress(m_SP[1]);
if (callParams->staticCall && isPrecompiledContract(destinationAddr))
m_runGas = toInt63(m_schedule->precompileStaticCallGas);
m_runGas += toInt63(m_schedule->precompileStaticCallGas);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@halfalicious I just noticed that this became +=, why?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants