Bug: elements of _items_vector are not updated when multi_index is moved to new instance

[smlu]

https://github.com/EOSIO/eos/blob/develop/contracts/eosiolib/multi_index.hpp#L270 The elements of _items_vector are not not updated to the address of new instance when multi_index is moved causing certain operations to fail. Affected member functions are iterator_to, erase and modify. All three functions fill throw an assert exception with message object passed to <member_function> is not in multi_index on cached items when index was moved to new instance.

The core of this problem is that all successful query operations carried against multi_index, cache loaded items. Cached Items store the address of multi_index instance in __idx member variable for identification purposes. The __idx variable is never updated when index is moved to the new instance.

Items are cached here: https://github.com/EOSIO/eos/blob/develop/contracts/eosiolib/multi_index.hpp#L590 and here: https://github.com/EOSIO/eos/blob/develop/contracts/eosiolib/multi_index.hpp#L1696

Example of a bug:

multi_index_t get_multi_index_of(uint64_t key)
{
    multi_index_1 mi1;
    if(mi1.find(key) != mi1.end()) {
        return mi1; // returns mi1 via RVO, no deep copying involved
    }

    multi_index_2 mi2;
     ......
}

///////////////////////////////////////////////////////

auto mi = get_multi_index_of(some_key);
auto it = mi.find(some_key);
mi.modify(it, payer, [](auto& obj){...}) // Error

Possible solutions:

• Replace member variable __idx with unique identifier which is not tied to the address of index instance. Since __idx is not used for other than identifying parent index, it's safe to replace it with short UID.
• Explicitly delete move constructor / move assignment operator. I would also not recommend this solution because there are cases where you want to move multi_index from one object to another.
• Make custom move constructor / move assignment operator and do not move _items_vector to new instance. The problem with this solution is, that new instance looses all cached items and has to fetch them again from db.

Other possible solution:

• Make custom move constructor / move assignment operator and update items of _items_vector to the new instance before move. This solution is very expensive in terms of CPU cycles so I wouldn't consider it.

[taokayan]

I don't think copying the entire multi_index table is a good practice because of several reason:

• you can't copy _items_vector, because there's a std::unique_ptr inside each of its element owning the corresponding cache
• changing from std::unique_ptr to std::shared_ptr to allow copying is inefficient, which will only result in more memory and time overhead.

workaround: you can put your multi_index instances as member of your class, so that your function get_multi_index_of should only return the pointer of your multi_index instance.

[smlu]

Thank you for the suggestion but I think this issue should not be closed yet since move and RVO interface of multi_index is broken. No deep copying ever occurs because copy constructor is implicitly deleted due to member of item_ptr being type of std::unique_ptr. https://github.com/EOSIO/eos/blob/develop/contracts/eosiolib/multi_index.hpp#L265

Please note I've update the code example with a comment so it's more clear what it does. @taokayan @tbfleming

[tbfleming]

It's a confirmed bug (#4577). For efficiency reasons we're more likely to delete copy and move.

[smlu]

Would you maybe consider making explicitly defined move constructor where _items_vector is not moved to the new instance? It's a quick fix and it works. The only drawback is that cached items are not preserved.

[tbfleming]

When most standard containers move, iterators and references to their content remain valid. Not keeping that semantic invites bugs.

[smlu]

I agree, but in this case are only cached items which would not be available to the new instance. The state of _items_vector in both instances (old and new) remains valid so nothing breaks. The only drawback is that the instance index moved to has to (lazily) load again already cached items. Its temporarily solution but it would fix current broken interface.