Using account_create_with_delegation with 0 delegation creates unnecessary objects

[mvandeberg]

PR for #997

[NateBrune]

Might be worth optimizing this using Boost's erase(iterator first,iterator last) if we order by delegated vesting shares, or just erase(const key_type& x) if we keep vesting shares a member in the index.

[mvandeberg]

We do not track that index. This is the safe way to implement the fix. The 0 delegation objects should have no effect on consensus. After the hardfork we can change the 0.17 behavior to match 0.19 and verify that not creating 0 delegation objects has no effect. In which case we can delete this logic entirely.

[theoreticalbts]

You have to be really careful about concurrently modifying collections. Let's use a lower_bound() on the ID to get a completely fresh iterator after that remove().

Let's add a TODO comment noting how this can be simplified after the HF passes.

Whether zero delegations are represented by a zero delegation object, or by the lack of a delegation object, is a non-consensus implementation detail. So I'd personally avoid implementing this as a hardfork. But this implementation is probably a little safer, in case there is some consensus difference I overlooked, it won't split the network.

[mvandeberg]

We use the same pattern here.

[theoreticalbts]

The evaluator check is:

if( hf19 && amount > 0 || !hf19 )

You can simplify any expression of the form (a & b) | !a, it distributes to (a|!a) & (b|!a), the first term is tautological so it simplifies further to b|!a. We cannot always apply this simplification with short circuit logical operators (e.g. when evaluation of b is undefined or will cause side effects when a is false), but we can do so here (o.delegation_amount is always defined). So a clearer test is

if( (amount > 0) || !hf19 )

Also, for( auto itr : remove ) should be for( const delegation_object* object_ptr : remove ) for two reasons:

• Code is clearer if you avoid auto
• These are pointers to objects, not iterators being dereferenced