Bitshares-core: Replace all flat_index with generic_index

[abitmore]

To avoid further potential issues.

[oxarbitrage]

Before replacing here is the idea plan i have on how to proceed in this issue. I will like you guys to let me know what do you think.

In the current bitshares core there is only 2 indexes of type flat_index. this are:

• This is the one that gave us the problems of corrupted data yesterday: https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/include/graphene/chain/asset_object.hpp#L231

This was already replaced by @bytemaster at pull: https://github.com/bitshares/bitshares-core/pull/324

• Then we have flat_index being used at one more object in here:
  https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/db_init.cpp#L211
  And here:
  https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/db_init.cpp#L244

• There was a flat_index in the worker_object in the past that was replaced by a generix_index as well, commented line is here:
  https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/include/graphene/chain/worker_object.hpp#L156
  This is maybe due to a similar problem in the past ?

So the idea will be to replace the flat_index around the block_summary_object with a generic index and test that the index still works as before.

If everything is ok i think we may want to remove this file too:
https://github.com/bitshares/bitshares-core/blob/master/libraries/db/include/graphene/db/flat_index.hpp
As there will be no more flat indexes in our code.

I created a "develop" branch from master here:
https://github.com/bitshares/bitshares-core/tree/develop

Starting from now pull requests should be made there instead of master directly(unless very special circumstances). More info about this will be added to a wiki document as i don't want to divert the attention of this particular issue.

Please let me know what do you think in order to proceed.

[abitmore]

• size of block_summary_object will never be unchanged after initiated, so I think we don't need to change it. (also OK if we change it)
• perhaps we need to remove simple_index as well?

[pmconrad]

I think I've got the root cause.

Like I suspected, the failed proposal 1.10.3598 attempts to create a bitasset but fails, which leads to the created objects being rolled back. flat_index is not made for rollbacks, nor for removing anything at all: https://github.com/bitshares/bitshares-core/blob/master/libraries/db/include/graphene/db/flat_index.hpp#L33

What happens is that bitasset_data 2.4.104 is created and then removed. This is the first time in the history of our chain when flat_index::remove is called. As you can see at https://github.com/bitshares/bitshares-core/blob/master/libraries/db/include/graphene/db/flat_index.hpp#L73 , the object in question is not really removed but zeroed out (overwritten with T()).

Later, in the maintenance interval, at https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/db_maint.cpp#L877, all bitasset_data objects are modified to have their settlement_volume set to 0. The iterator of flat_index walks over all objects in the index, including the zeroed out item at index 104: https://github.com/bitshares/bitshares-core/blob/master/libraries/db/include/graphene/db/flat_index.hpp#L104 . The object at index 104 has the incorrect id 0. That leads to modify() actually attempting to modify object 0.0.0, which triggers an assertion failure.
(Actually I'm not quite sure about the last part, but this is where disaster strikes.)

Conclusion: flat_index is not made for having items removed. Proposals can always lead to items being removed, which means that flat_index is not suitable for the job. Like BM suggested, it should be replaced in all places.

[oxarbitrage]

that was awesome @pmconrad , very well done.

linking the test unit you also did, ill test that and comment you there:

https://github.com/bitshares/bitshares-core/pull/326

[oxarbitrage]

i am unsure if we should change the flat_index of block_summary_object . i don't think items will be removed from there but not totally sure, what do you think @pmconrad ?

[pmconrad]

Well. Usually I'd prefer to only change what is absolutely necessary.
OTOH flat_index has a bug, and my feeling is that this will kill us at some point in the future if we don't get rid of it. So unless someone points out a convincing advantage of flat_index over simple_index (or some other index type) I vote to replace flat_index where it is used and remove flat_index.hpp from the codebase.

[compumatrix]

Are the revisions on the web wallets? The latest release is still 170148.

[pmconrad]

https://github.com/bitshares/bitshares-ui/releases
Please post general questions in the forum.

[oxarbitrage]

i merged the patch from @pmconrad to remove the flat_index fully. @xeroc approved and i did too. i will leave the issue open 24 hours more if someone want to take a look before closing as resolved.