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

Question on EIP3: CALLDEPTH #25

Closed
janx opened this issue Nov 20, 2015 · 6 comments
Closed

Question on EIP3: CALLDEPTH #25

janx opened this issue Nov 20, 2015 · 6 comments
Labels

Comments

@janx
Copy link
Member

janx commented Nov 20, 2015

link: https://eips.ethereum.org/EIPS/eip-3

IMO it's a good practice to check return value of callee if consequential operations is dependent and critical, if so what's the advantage of checking CALLDEPTH instead of return value?

Another point is, even if you use CALLDEPTH to guard stack overflow, you may still need return value guard in case there's other errors, thus leads to more verbose code?

@nmushegian
Copy link

CALLDEPTH seems like it will ruin encapsulation in a lot of ways. msg.sender is an elegant abstraction

It seems CALLDEPTH is trying to solve a problem that could be solved more simply by some kind of error bit in the EVM state. Maybe OOG can be retrofitted to use an error bit as well.

@frozeman frozeman added the EIP label Nov 23, 2015
@ebuchman
Copy link

I similarly don't see the value in using CALLDEPTH instead of an error bit, but I did advocate for something similar, for different reasons, a year ago, and still would now: https://www.reddit.com/r/ethereum/comments/2hu2fq/proposal_for_new_opcodes_callstack_and/

@axic
Copy link
Member

axic commented Sep 5, 2018

The current version is reachable at: https://eips.ethereum.org/EIPS/eip-3

@holiman it seems you have never addressed the questions above.

Is it still relevant? If not, should it be marked "Rejected"? If yes, what would it take to implement it?

While not fully relevant, it does help in the context of ewasm implementing evm2wasm (see ewasm/design#138)

@Earlz
Copy link

Earlz commented Nov 4, 2018

The CALLDEPTH opcode would make it trivial to implement a guard preventing uncontrolled reentrancy into the contract while still allowing self-calls. With the new net gas metering for storage, I think it'd even be fairly cheap

Basically just a storage variable expectedDepth. expectedDepth would be incremented when calling itself, and each function needing a reentrancy guard would check expectedDepth matches CALLDEPTH. When the self-call function returns, decrement expectedDepth. If implemented properly, expectedDepth is never anything but 0 upon contract termination (except in case of error, where this state would be reverted anyway). It would need some special case checking for when the contract is initially called with CALLDEPTH > 0, potentially another storage variable or flag saying "execInProgress" or similar, but should be easily doable.

The only other existing way to implement such a guard is not suitable for all cases. It can be done by checking sender == origin, but this prevents contract called contracts from preventing reentrancy as well as self-calls. It's also possible to use a simple storage mutex, but that also prevents self-calls.

I see no way to have this kind of reentrancy guard functionality without CALLDEPTH and I don't really understand why this EIP is deferred with very little discussion and with only one narrow use case in mind

@github-actions
Copy link

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Jan 19, 2022
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this as completed Feb 2, 2022
RaphaelHardFork pushed a commit to RaphaelHardFork/EIPs that referenced this issue Jan 30, 2024
…plementation. (ethereum#25)

* Revised version of ERC-6123 including reference implementation for OTC-Derivatives and according Unit Tests

* Revised Doc, renamed SDC in SDCAbstract

* Revised Readme

* Changed Status to "DRAFT" - here we go again.

* Fixed some typos.

* Fixed some typos.

* Fixed typos. Fixed typo in file name.

* Removed unused variables.

* Reverted named change. Refactor rename SDCAbstract to SDC

* Changed the instruction to run out-of-the-box (clean checkout) wit

* Used view, where possible. Used local variable.

* Commenting our unused parameter.

* Fixed reference to image. Fixed blank lines after headlines.

* Fixed blank lines before code.

* Fixed headline (duplicate/wrong).

* Added note on NPM version numbers.

* Used ERC-20 in place of EIP-20

* Added abstract. Used Specification as headline.

* Fixed link to eip-20.md

* Minor fixed to README.md

* Minor fixes to README.md

* Simplified instruction to run unit tests, added package.json and hardhat.config.js

* Simplified instruction to run unit tests, added package.json and hardhat.config.js

* Removed duplicated resulted from manual merge with new ERC repo.

* Fixed link: eip-20.md -> erc-20.md

* Fixed Bug in SDCPledgedBalance.sol (afterTransfer), minor rewriting in markdown doc.

* Minor fixes (typos).

* Linking to eip instead of erc (see remark 7 ii on migrating pull requests to ERC repo).

* Linking to eip instead of erc (see remark 7 ii on migrating pull requests to ERC repo).

* Removed external link upon request.

* Update ERCS/erc-6123.md

Co-authored-by: Sam Wilson <[email protected]>

* Update ERCS/erc-6123.md

Co-authored-by: Sam Wilson <[email protected]>

* Update ERCS/erc-6123.md

Co-authored-by: Sam Wilson <[email protected]>

---------

Co-authored-by: Peter KL <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
bumblefudge pushed a commit to bumblefudge/EIPs that referenced this issue Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants