Pessimistically locked SLOAD

[homakov]

No description provided.

[Arachnid]

Interesting idea!

I'm concerned, though, that as written this could make some useful patterns impossible - for instance, recursive calls to something that appends to an array, or transferring tokens to a contract that then immediately transfers (some of) them to another address.

A couple of alternate suggestions:

• An explicit SLOCK operation
• A change to gas accounting to only charge gas for the net change in storage at the end of a frame.

The latter would make mutexes and other concurrency primitives affordable, as well as other use cases that require temporary storage that persists across calls. Because none of this data gets written to disk, it's affordable for nodes, too.

[homakov]

What is needed to accept it for consideration?

Because i'd like to hear all opinions and try to figure out the best way out of it.

[GNSPS]

I was unsure of changing the default SLOAD behaviour but @Arachnid 's suggestion just lit up my eyes!

A dedicated operation for locking and only accounting for storage changes at the end of the frame is an awesome idea. Where do I sign? 😂

[homakov]

Well, effectively exposing properly SLOADUNSAFE + locked SLOAD would achieve the same result. I.e. if you need the reuse, just use the unsafe version (solidity may expose it as unsafestorage data type for example).

What i'm saying, it should be by default, and old code that people used to write should start working locked. All the use cases for recursive call of the same contract are corner cases and should be using another solidity functions.

If anyone who's handy with eth blockchain, would be great if you could pass a global test whether there is anyone doing recursive calls except dao draining. Maybe I'm missing some case, don't know!

[GNSPS]

@homakov the 0x protocol core contracts are indeed making use of reentrancy, for example. Was that your question?

[MicahZoltu]

I'm very much against this EIP. For a non-trivial dApp, touching one storage variable multiple times in a single transaction is not uncommon. Also, since there are no races in the EVM, the situation is quite different from traditional DB locking as atomicity is already strongly guaranteed due to lack of parallelism.

I'm a fan of netting gas costs for storage updates, but that sounds like a separate EIP from this. I wouldn't be against against an explicit lock opcode, thought I don't know if the value would outweigh the implementation cost.

[homakov]

@GNSPS can you point directly where it's used?

@MicahZoltu enlighten me, please, how non trivial dapps use same storage records recursively? I mean exact use case.

Another idea: SUNLOCK that unlocks particular SLOAD-ed key for future calls, and removes that key from $locked array. I.e. the opposite of SLOCK idea @Arachnid offered.

Secure by default, easily unlockable if needed. Good?

[MicahZoltu]

Nick gave an example already that is pretty common. Tokens are transferred into a contract early on in the transaction and then some portion (sometimes all) are transferred away later on in the transaction. Other examples include mutexes that use a storage variable to prevent reentrancy like the Open Zeppelin one.

Any changes to default behavior of SLOAD will break existing contracts, and therefore should not be included unless there is a really compelling reason to do so. I do not believe that this necessitates a breaking change to existing opcodes.

[homakov]

I still don't get it. Can you explain it with one by one Frame 1, frame 2, frame 3 scenario?

You probably assumed SLOAD is locked inside the same frame? That's pseudocode bug: in fact it is only locked for CALL frames inside current frame, and unlocked for everything else.

[homakov]

P.S. https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/ReentrancyGuard.sol - not how security design works. Things like that must be on by default, otherwise they are never used. This EIP is a live example of a ton of such examples.

[GNSPS]

The 0x example involves several contracts calling each other. Can't exactly recall now but they're here: https://github.com/0xProject/contracts

As @MicahZoltu explains, changing the current implementation of SLOAD would break several existing systems.
Also I don't think that would be the way to go. We can preserve SLOAD's current behaviour and, since no one is currently writing complex smart contracts in raw bytecode (that I know of), implement the default behaviour you want at compiler level in any of the existing HLLs. (edit: have the HLL compiler use SLOCK by default)

So my vote would go only to @Arachnid proposal.

[MicahZoltu]

ERC223 makes it so token transfers call a function on the contract receiving the tokens. This means that an ERC223 token transfer call stack may look like:

MyToken.transfer(FooContract, 1000); // msg.sender = BarContract
FooContract.onTokenReceived(MyToken, BarContract, 1000);
MyToken.transfer(AppleContract, 500);

While you may not like the mechanism used in ReentrancyGuard, it exists in the wild today and will break if this change is implemented.

[homakov]

Now I see, the receiver sends parts of the tokens further to someone else, so they call same contract which has to SLOAD Foo's balance and would throw as it's locked by first frame.

SUNLOCK could be used here just after storage manipulations are done, explicitly.
I.e. before onTokenReceived: balances.unlock(FooContract, BarContract)

That's a good example. But not sure why ReentrancyGuard would break? SLOAD will fail even sooner, so require() will not be executed, but the result will remain the same: fail on reentry.

If there's a erc223 style execution, new opcode behavior could break it. So it's better to be introduced for contracts created after the block... or the solution by Nick :)

[MicahZoltu]

Reentrancy guard would break because the lock is set to true before the method executes and then set to false after the method is finished executing. This means the lock is set twice in the same transaction (first to true, then to false).

[homakov]

But there's nothing wrong with setting it twice. Global lock is only enforced in next frames, there's no point in disallowing multiple access to a storage record within same frame.

[pirapira]

Solidity has been using a single storage cell for multiple state variables (two uint128's can fit in one word). So accessing a state variable might lock another. The question is whether this troubles any existing Ethereum contracts.

[GNSPS]

Having a different opcode for it would mean that, in Solidity's specific case, newer contracts making use of this storage key locking feature could have the tight packing of state variables disabled.

[homakov]

@pirapira good catch. is it when solidity stores Structs only or literally two different vars could be stored under same record?

[pirapira]

@homakov the latter. The more agressive one.

[Arachnid]

FWIW, my suggestion was that with net gas metering for storage changes, it wouldn't be necessary to introduce an explicit lock instruction; contracts could affordably implement locking themselves using storage variables - as well as a lot of other useful improvements.

I'm in agreement with the author that looking at transactions like database operations is a good model, thouh.

[GNSPS]

I do think that the explicit lock instruction has indeed got merit, though, since locking state would benefit from being implemented by default at the language level instead of being only a good practice. 🙂

[homakov]

Explicit not touching storage lock with next-to-0 gas (i guess you can't make it 0) - that's good enough for me.

[Arachnid]

@GNSPS it's possible to offer this as a language primitive without a lock instruction.

[androlo]

Net gas calculation seems the simplest and most useful solution. With net gas cost, and modified pricing, it could make (affordable) temporary storage possible. Languages could implement it for example by adding lock and unlock builtins for state variables (and auto generate temporary storage addresses for locks). They could even introduce a new type of variable (transient state variables perhaps) and auto generate code for clearing them at the end of any function where they are used. Also, as pointed out, it would only modify systems that are already in place. No new opcodes or anything like that. Fantastic.

[pirapira]

Please waive the copyright with CC0, as in https://github.com/ethereum/EIPs/blob/master/EIPS/eip-196.md#copyright

[homakov]

Sure - no copyright and CC0

[pirapira]

Category: Core

should appear.

[motafa1320]

thanks

[pirapira]

Please make it explicit that Storage0xA stands for an account's storage.

[pirapira]

When there is an integrity condition among different storage keys, this approach is not guaranteed free of race condition bugs.

[pirapira]

What about SSTORE? I guess SSTORE needs to be modified. If that's not the case, that should be said explicitly I think.

[homakov]

I'm busy with something else and in this threat there were better suggestions by @Arachnid so this EIP isn't probably best way. I cannot keep it up to date - feel free to use create new one.

As of sstore, the safe way would be to throw error if the key is still locked, but most of the time you load before writing, so it would throw anyway.

[pirapira]

@homakov thank you for the response. I marked this PR with looking-for-champion.

[pirapira]

My guess is like 20 more contracts would be affected. I'm interested in the result.

[pirapira]

The biggest obstacle for a merge is that the missing mention on SSTORE in the specification section.

[Arachnid]

This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:

• Frontmatter is now contained between lines with only a triple dash ('---')
• Headers in the frontmatter are now lowercase.

If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR.

In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks.

Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:

---
eip: <num>
title: <title>
author: <author>
type: [Standards Track|Informational|Meta]
category: [Core|Networking|Interface|ERC] (for type: Standards Track only)
status: Draft
created: <date>
---

[jamesray1]

I'll share this with my Drops of Diamond team, working on sharding for Rust, as well as Parity. I'm too busy to work on this myself now, but I'll save it to Pocket.

[jamesray1]

A change to gas accounting to only charge gas for the net change in storage at the end of a frame.
The latter would make mutexes and other concurrency primitives affordable, as well as other use cases that require temporary storage that persists across calls.

This would be particularly useful in the case of synchronous contract calls across shards. http://notes.ethereum.org/s/BJc_eGVFM#cross-shard-communication

[nicksavers]

@homakov The eip-X template has changed a bit. Please update your pull request to this for your draft to be merged. You can rename the file to eip-718.md and use this number in the preample.

[axic]

@homakov are you still pursuing this?

[homakov]

@axic I still think locking database would be very safe to do by default, but already 2 years passed without any changes in the protocol and/or solidity. As said in #718 (comment) I don't pursue it anymore.

It either needs a new champion (didn't happen in 2 years) or be closed.

[github-actions]

There has been no activity on this pull request 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]

This pull request 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.