Invalid opcode exception when trying to resize a storage array

Question

I wrote this function:

function removeClaim(uint256 _claimId) public returns (bool success) {
    require(msg.sender == owner || msg.sender == claims[_claimId].issuer);

    // Emit event and store burned signature before deleting to save gas for copy.
    IdentityContractLib.Claim storage claim = claims[_claimId];
    emit ClaimRemoved(_claimId, claim.topic, claim.scheme, claim.issuer, claim.signature, claim.data, claim.uri);
    burnedSignatures[claim.signature] = true; // Make sure that this same claim cannot be added again.

    // Delete entries of helper directories.
    uint256[] storage array = topics2ClaimIds[claim.topic];
    uint32 positionInArray = 0;
    while(_claimId != array[positionInArray]) {
        positionInArray++;
    }

    for(uint32 i = positionInArray; i < array.length - 1; i++) {
        array[i] = array[i+1];
    }

    array.length = array.length - 1;

    // Delete the actual directory entry.
    claim.topic = 0;
    claim.scheme = 0;
    claim.issuer = address(0);
    claim.signature = "";
    claim.data = "";
    claim.uri = "";

    return true;
}

I'm trying to reduce the size of topics2ClaimIds[claim.topic] which I aliased to array. But changing array.length = array.length - 1; to topics2ClaimIds[claim.topic].length = array.length - 1; doesn't change the problem.

If I add a return right before array.length = array.length - 1;, the function executes normally. However, if I add a return right after this line (or none at all letting the function execute till the end), I get this error:

Error: Returned error: VM Exception while processing transaction: invalid opcode

If I understand the documentation correctly, every storage array is a dynamically sized array:

For dynamically-sized arrays (only available for storage), this member can be assigned to resize the array. Accessing elements outside the current length does not automatically resize the array and instead causes a failing assertion. Increasing the length adds new zero-initialised elements to the array. Reducing the length performs an implicit :ref:delete on each of the removed elements. If you try to resize a non-dynamic array that isn’t in storage, you receive a Value must be an lvalue error.

(https://solidity.readthedocs.io/en/v0.5.3/types.html)

In case this is wrong, please let me know.

The mapping I use and therefore the array in question is declared as follows in the header of the contract:

mapping (uint256 => uint256[]) topics2ClaimIds;

Btw. I realize that arrays are a terrible data structure for what I'm trying to do to begin with. If Solidity has better data structures available (like a binary heap for starters), please let me know.

Answer 1

I got a little lost in it, in part because we can't see the whole thing and in part because the data structure is leading to convoluted code. You eluded to that.

Btw. I realize that arrays are a terrible data structure for what I'm trying to do to begin with. If Solidity has better data structures available (like a binary heap for starters), please let me know.

Solidity does not have built-in structures like that, but one can create them.

Delete from array (set) seems to be causing the headaches. Have a look at this for a description of the technique: https://medium.com/robhitchens/solidity-crud-part-2-ed8d8b4f74ec

As mentioned in the blog, the technique is still valid but the original code is a little outdated. It's available as a library to offload the set management concerns. https://medium.com/robhitchens/solidity-crud-epilogue-e563e794fde

The actual repo: https://github.com/rob-Hitchens/UnorderedKeySet

I found it less time-consuming to just implement my interpretation of your intent instead of fishing around for the bug in your code.

pragma solidity 0.5.16;

import "./HitchensUnorderedKeySet.sol";

contract Claims {

    using HitchensUnorderedKeySetLib for HitchensUnorderedKeySetLib.Set;

    struct Claim {
        uint scheme;
        address issuer;
        bytes signature;
        bytes data;
        bytes url;
    }    

    struct Topic {
        HitchensUnorderedKeySetLib.Set claimIdSet;
    }

    HitchensUnorderedKeySetLib.Set topicIdSet;

    mapping(bytes32 => Topic) topics; // can't be public because the compiler doesn't know who to handle the Set type
    mapping(bytes32 => Claim) public claims;
    mapping(bytes32 => bool) public usedSignatureHashes;

    function newTopic(bytes32 topicId) public {
        topicIdSet.insert(topicId);
    }

    function removeTopic(bytes32 topicId) public {
        Topic storage t = topics[topicId];
        require(t.claimIdSet.count() == 0, "Cannot delete topic with claims. Remove the claims first.");
        delete topics[topicId];
        topicIdSet.remove(topicId);
    }

    function newClaim(bytes32 topicId, bytes32 claimId, uint scheme, address issuer, bytes memory signature, bytes memory data, bytes memory url) public {
        require(topicIdSet.exists(topicId), "Topic ID not found.");
        require(!usedSignatureHashes[keccak256(signature)], "Claim signature was used before.");
        usedSignatureHashes[keccak256(signature)] = true;
        Topic storage t = topics[topicId];
        t.claimIdSet.insert(claimId); // this will not allow a duplicate
        Claim storage c = claims[claimId];
        c.scheme = scheme;
        c.issuer = issuer;
        c.signature = signature;
        c.data = data;
        c.url = url;
    }

    function removeClaim(bytes32 topicId, bytes32 claimId) public {
        Topic storage t = topics[topicId];
        t.claimIdSet.remove(claimId); // will revert if claim ID does not exist in topic
        delete claims[claimId];
    }

    // inspectors
    function topicClaimCount(bytes32 topicId) public view returns(uint) {
        return topics[topicId].claimIdSet.count();
    }

    function topicClaimIdAtIndex(bytes32 topicId, uint index) public view returns(bytes32) {
        return topics[topicId].claimIdSet.keyAtIndex(index); // must exist
    }

    // convenience function for Remix
    function arbitraryKey() public view returns(bytes32 key) {
        key = keccak256(abi.encodePacked(block.number));
    }

}

It's admittedly just a scribble, so no warranty, but superficial testing seemed to confirm it's doing the job.

Hope it helps.

Answer 2

An invalid opcode exception typically implies either one of the following two scenarios:

• An assert(cond) statement with cond == false
• An array[i] expression with i >= array.length

You don't have any assert statements in your code, so you should start by looking into every x[y] statement.

However, some of those statements in your code are with x being a mapping rather than an array, and regretfully, you haven't posted all the information required for the reader in order to determine whether a given such statement refers to an array or to a mapping.

So I'll assume that the following statements refer to a mapping, and if they're not, then feel free to check them too:

• msg.sender == claims[_claimId].issuer
• claim = claims[_claimId]
• burnedSignatures[claim.signature] = true
• array = topics2ClaimIds[claim.topic]

Now we are left with the following piece of code (which includes all other x[y] statements):

    uint32 positionInArray = 0;
    while(_claimId != array[positionInArray]) {
        positionInArray++;
    }

    for(uint32 i = positionInArray; i < array.length - 1; i++) {
        array[i] = array[i+1];
    }

IMO, the immediate suspect here is array[positionInArray], and you should definitely change this:

    while(_claimId != array[positionInArray])

To This:

    while(positionInArray < array.length && _claimId != array[positionInArray])

More generally, in order to actually achieve your purpose (and not only resolve this bug), you should decide how you want to handle the case where you do not find a value of positionInArray such that _claimId == array[positionInArray], and then implement it in your code (right after the loop which searches for this value).