eluv-io / contracts

Eluvio Content Management Smart Contracts
MIT License
10 stars 0 forks source link

fix collision of content address in case of reorg #116

Open elv-gilles opened 1 year ago

elv-gilles commented 1 year ago

Relates to: content-fabric/1763

Current code use the CREATE opcode to deploy contents, which uses the address of the caller and its nonce. Since the caller is always the BaseContentFactory of the space, this results in creating the same content address for distinct users, should the users do the call in parallel. In case of reorg, one of the users wins and the other is left with the address of a content which he does not own.

Unit-tests TestCreateContentMultipleUsers* in qluvio/content-fabric#2397 show the problem and/or the fix.

Instead, using the CREATE2 opcode (see eip-1014 allows using a 'salt' parameter which we can craft to depend on the actual user (with address tx.origin) requesting the content creation.

    // saltFor computes a salt for the given address: 'address++nonce'.
    // block.number is used as nonce if the provided nonce is zero.
    // The function is public for testing purposes.
    function saltFor(address addr, uint64 nonce) public view returns (uint256) {
        uint256 ret;
        ret = uint256(uint160(addr));
        ret <<= 96;
        if (nonce == 0) {
            // block.number has to fit into a uint64
            // see go-ethereum@v1.10.19/core/types/block.go in Header.SanityCheck
            nonce = uint64(block.number);
        }
        return uint256(ret) + nonce;
    }

~~also tried to get rid of the copy/pasted currentObjBin in ContentFactoryHelper by using type(BaseContent).creationCode but this always resulted in revert during content creation. May someone have a closer look at this (or give a hint) ?~~

some links:

lukseven commented 1 year ago

I think address++block.number is not sufficient - like that you can only create a single content per block and won't be able to do bulk creation. Not sure whether this is an issue right now - do any of @elv-marc's ingest tools create multiple contents in a single block? Anyway, we should include an additional item in the salt. The user's nonce would be best, but that's apparently not available, right Gilles? Is there anything else we can use?

elv-gilles commented 1 year ago

user's nonce not available

not from solidity.

Instead of block.number, we could use a simple precompile returning a random uint64 or use a state variable in the factory.

elv-gilles commented 1 year ago

We could also require the user to send a value (clients would take the user's nonce per default).

This would make an API change, so we could add a function, the old one calling the new one with nonce at zero. And in saltFor if nonce is zero, use block.number.

newContent(address payable lib, address payable content_type, uint64 nonce)
lukseven commented 1 year ago

Adding a new function and having the user submit the nonce sounds good to me - like that we enable bulk operations, but also remain backwards-compatible.

The salt should always include the block.Number, even if a nonce (or some other value...) is submitted: salt=address++block.number++nonce

lukseven commented 8 months ago

@elv-gilles how do we deploy this? Doesn't this require a new space?

elv-gilles commented 8 months ago

@elv-gilles how do we deploy this? Doesn't this require a new space?

No, I believe this is not required, 'just' updating the space factories. But that would need to be tested and SS/Preethi know more (I think) about updating factories and the pitfalls.