1

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.

5
  • You cannot reduce its size if it is already empty, check the length is > 0 before reducing it.
    – Ismael
    Jan 21, 2020 at 13:57
  • @Ismael If I add require(array.length > 0); after the for loop, I still see the same behavior: With a return right before the size reduction (but after the require), it doesn't fail. Without it, it fails. So my array has at least 1 element.
    – UTF-8
    Jan 21, 2020 at 14:25
  • Obviously require will cause the whole transaction to revert if the array is empty. If you do not want that you can use if (array.length > 0) { array.pop(); } for solc 0.5.xx. Using loops to iterate the whole array is something I'd not recommend because of possible excesive gas costs.
    – Ismael
    Jan 21, 2020 at 18:32
  • @Ismael I know that that will revert the transaction. The point was to eliminate the array being empty as a possible error source. array.pop() is not an option as I need to delete a specific element that can be anywhere in the array.
    – UTF-8
    Jan 21, 2020 at 18:35
  • If the array is already empty then there's nothing to delete. From your code the array was stored incorrectly or it was deleted accidentaly. (An optimization to delete a position in your array is to replace it with the last item.)
    – Ismael
    Jan 21, 2020 at 18:42

2 Answers 2

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.

2
  • 1
    Wow, I somehow didn't think I could do what you did: Just swap the element to delete out for the last element like you did. Thank you very much for the code!
    – UTF-8
    Jan 21, 2020 at 18:46
  • Thanks for accepting and upvoting. Good luck with it. Jan 21, 2020 at 18:52
3

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

1
  • Thank you. That's indeed a bug. However, it's not the reason for the error message. I by now implemented just creating a new array and copying all of the other elements over. I made the same mistake again but it runs fine. Still, thank you for pointing that out.
    – UTF-8
    Jan 21, 2020 at 18:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.