Skip to content

fix collision of content address in case of reorg#116

Open
elv-gilles wants to merge 5 commits intodevelopfrom
fix-qid-collision
Open

fix collision of content address in case of reorg#116
elv-gilles wants to merge 5 commits intodevelopfrom
fix-qid-collision

Conversation

@elv-gilles
Copy link
Contributor

@elv-gilles elv-gilles commented Apr 13, 2023

Relates to: content-fabric/1763

  • use CREATE2 opcode to deploy content
  • MISSING: fix build scripts and generate new version AND tag

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:

* use CREATE2 opcode to deploy content
* MISSING: fic build scripts and generate new version+tag
@lukseven
Copy link
Contributor

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
Copy link
Contributor Author

elv-gilles commented Apr 13, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

lukseven commented Apr 13, 2023

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

elv-preethi and others added 3 commits April 13, 2023 16:37
* to avoid (potential) collision with user's nonce on a young chain
@lukseven
Copy link
Contributor

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

@elv-gilles
Copy link
Contributor Author

@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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants