Nonreentrant modifier breaks backward compatibility in storage allocation between versions 0.2 / 0.3

[pavlovdog]

Version Information

• vyper Version: 0.3.0
• OS: osx
• Python Version: Using docker image as a plugin for Hardhat

What's your issue about?

After switching to the 0.3.1 compiler, I've found that the storage layout has been changed. Now, the first variable is not stored in the first slot, but in the second; the second variable is stored in the third slot, and so on.

In my project, we're using Vyper contracts (link) with a Transparent Upgradeable Proxy pattern (link). The current implementation is built with 0.2.12 compiler, so it's impossible to upgrade the compiler version without breaking everything.

What is also important, is that I've reached the contract size limit, so to deploy a newer implementation I need better optimization, which can be achieved only by upgrading to the 0.3 compiler.

Repro

I've built a simple repro contract and tested it against two compiler versions. Test code is the same, and contracts differ only in the @version line. It's pretty clear, that the slots allocation is different.

FooBar.vy

Change the @version line to see the difference.

# @version 0.3.1

foo: public(uint256)
bar: public(uint256)

@external
def __init__():
    self.foo = 1
    self.bar = 2

@external
@nonreentrant("incrementBar")
def incrementFoo():
    self.foo += 1

@external
def incrementBar():
    self.bar += 1

Test code

describe('Test slots allocation', async () => {
    it('Deploy dummy contract, check public functions and storage slots', async () => {
        const FooBar = await ethers.getContractFactory("FooBar");
        const fooBar = await FooBar.deploy();

        console.log(await fooBar.foo());
        console.log(await fooBar.bar());

        console.log(await ethers.provider.getStorageAt(fooBar.address, 0));
        console.log(await ethers.provider.getStorageAt(fooBar.address, 1));
        console.log(await ethers.provider.getStorageAt(fooBar.address, 2));
    });
});

Test output

0.2.12

  Test slots allocation
BigNumber { _hex: '0x01', _isBigNumber: true }
BigNumber { _hex: '0x02', _isBigNumber: true }
0x0000000000000000000000000000000000000000000000000000000000000001
0x0000000000000000000000000000000000000000000000000000000000000002
0x0000000000000000000000000000000000000000000000000000000000000000
    ✓ Deploy dummy contract, check public functions and storage slots (6866ms)

0.3.0

  Test slots allocation
BigNumber { _hex: '0x01', _isBigNumber: true }
BigNumber { _hex: '0x02', _isBigNumber: true }
0x0000000000000000000000000000000000000000000000000000000000000000
0x0000000000000000000000000000000000000000000000000000000000000001
0x0000000000000000000000000000000000000000000000000000000000000002
    ✓ Deploy dummy contract, check public functions and storage slots (5695ms)

How can it be fixed?

For me, it seems like unexpected behavior and breaks backward compatibility between compiler minor releases.

I've done the test without the nonreentrant modifier against both compiler versions and it fixed the problem. So my guess is that the problem is caused by slot allocation for the reentrancy flag.

Happy X mas ✌️ 🎅

[fubuloubu]

Hello @pavlovdog, first I would say that this is indeed the case that a breaking change was introduced. as per our versioning guideline, minor versions may contain breaking changes (we do not conform to semver):
https://vyper.readthedocs.io/en/stable/versioning.html#minor-release-x-y-0

However, you may be in luck. @charles-cooper was looking to create a VIP describing a compiler input file for the storage slots that would override the compiler-chosen values for the user-chosen ones. He is in the process of writing that proposal, and when it is created we will link it here so you can monitor it's progress.

If you are looking for a short term remedy, I would say that your options are limited to looking for ways to migrate your deployed project to the 0.3.x series if possible.

[charles-cooper]

@pavlovdog I was actually thinking about being able to manually override compiler generated storage slots, you gave me the impetus to write it up here #2572. It's not a terribly difficult feature to add actually, a motivated new contributor could probably do it with not too much time. In any case I could get around to it in a few weeks but if you need it sooner we would welcome your contribution!

[pavlovdog]

@charles-cooper @fubuloubu I would like to contribute, but for me, your solution seems too tricky.

1. Overriding compiler-chosen slots is a complex task. It requires deep knowledge for a developer, is prone to mistake and I think it won't be used often.
2. Storage layout is not obvious right now. You know, the first variable is not on the first slot and so on. Vyper is great because it puts simplicity and clarity first and this thing does not comply with this principle.

For me, compiler-inserted variables like the reentrancy flag should be located at some standard slot, something similar to the EIP 1967.

What do you think?

[charles-cooper]

@pavlovdog alright, well in the past we have held that the storage layout is subject to change (this could be for many reasons, but the main reasons would be optimization or bugfixes, for instance, see #1556, #2308). We can revisit this policy, but given that the storage layout could change sometimes, what do you think could be a good resolution for your issue?

[charles-cooper]

It might not be a huge deal to go back to nonreentrant variables starting after all other variables. But even so, the allocated storage size of a lot of things have changed as of v0.3.1. @fubuloubu thoughts?

[fubuloubu]

I think pain is already created, there's no sense in going back now when other changes have been made, and other changes will happen in the future as well.

making storage slot locations a part of the guarantees of the compiler would require a VIP to define exactly what that means

[charles-cooper]

I'm going to say we are not going to guarantee that different versions of vyper have the same storage layout - closing since we now provide #2593 if you need to manually provide storage compatibility between different vyper versions. @fubuloubu @pavlovdog please reopen for discussion if you have conflicting ideas about this.