BSIP 36 Candidate #832

oxarbitrage · 2018-04-07T21:42:52Z

The fix in db_maint was already introduced in #598 but needed hardfork protection, also there was a small bug on it to delete the correct feeds that now is fixed.

Test cases reproduce the problem before and after hardfork. I created the test in a new file as i was not sure where it could fit. This can be changed.

Problem is present only with smartcoins(mpas that are feeded by the witnesses)

Had to add a bunch of witnesses in the testcases to be able to remove default and control them, there is probably a simpler way to do this(by modifying the genesis?)

Open to all kind of feedback and test cases suggestions.

Issue: #518
BSIP: https://github.com/bitshares/bsips/blob/master/bsip-0036.md
Replaces: #598

abitmore · 2018-04-08T11:10:29Z

Had to add a bunch of witnesses in the testcases to be able to remove default and control them, there is probably a simpler way to do this(by modifying the genesis?)

IMHO using update_feed_producers( bitusd, {sam.id} ); in test cases would be enough. It doesn't harm to test with voted witnesses, it's more complicated.

oxarbitrage · 2018-04-09T13:12:45Z

using update_feed_producers is different and the impact of the bug is minor(there is a way to remove feed).
when feed producers are removed by the issuer(by op) the feed for the account is removed.

still, if the operation is never called feed will stay forever, the after HF fix will delete not needed and expired feeds in this case.

added test case bsip36_update_feed_producers at 10c7e8f with this path.

pmconrad · 2018-04-16T15:57:28Z

libraries/chain/db_maint.cpp

+ for( auto itr = o.feeds.begin(); itr != o.feeds.end();)
+ {
+ auto feed_time = itr[0].second.first;
+ if( feed_time + (o.options.feed_lifetime_sec) < head_block_time() && o.feeds.size() > o.options.minimum_feeds)


Why leave minimum_feeds in? They will be ignored anyway.

No need to cycle through the rest when the minimum has been reached.

Why leave minimum_feeds in? They will be ignored anyway.

are you sure this is safe? i guess the only thing that matters if all feeds are expired is current_feed.

just confirming before making the change.

Assets start out with no feeds at all and go through a time with less than minimum. I don't see why it shouldn't be safe.

@oxarbitrage please remove && o.feeds.size() > o.options.minimum_feeds check. It's not relevant here.

No need to cycle through the rest when the minimum has been reached.

@pmconrad what did this mean?

Once o.feeds.size() > o.options.minimum_feeds becomes false it will remain false, so once we reach that point we can as well break out of the loop.

Anyway I think this doesn't belong to here, since we want to remove ALL expired price feed entries.

oxarbitrage · 2018-04-24T23:32:08Z

i had the intention initially to remove expired non needed feeds from MPAs that are fed by witnesses or by feed producers as well. However, it seems that this will only be possible on witness fed MPAs(aka smartcoins).

the problem is in non smartcoins the feed producers are actually entries in the feed field of the bitasset options, if removed at expiration the producer will not be valid anymore to post a new one unless the issuer update the producers again by asset_update_feed_producers_operation.

the related code is here:
https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/asset_evaluator.cpp#L546-L558

as you can see witness feed producer are validated different and it doesn't matter if they are already in the feed fields or not while in the last case the feed producer need to be inside the feed field to work.

@pmconrad @abitmore please let me know what do you think because i think we need to change code to only go after smartcoins and leave the other mpas untouched.

abitmore

As mentioned in last comment, please only remove feeds for assets enabled witness_fed_asset or committee_fed_asset.

abitmore · 2018-04-25T09:27:51Z

libraries/chain/db_maint.cpp

+ // Remove all expired feeds
+ for( auto itr = o.feeds.begin(); itr != o.feeds.end();)
+ {
+ auto feed_time = itr[0].second.first;


itr[0] here looks strange. Why not use itr->?

abitmore · 2018-04-25T09:29:27Z

libraries/chain/db_maint.cpp

+ for( auto itr = o.feeds.begin(); itr != o.feeds.end();)
+ {
+ auto feed_time = itr[0].second.first;
+ if( feed_time + (o.options.feed_lifetime_sec) < head_block_time() && o.feeds.size() > o.options.minimum_feeds)


@oxarbitrage please remove && o.feeds.size() > o.options.minimum_feeds check. It's not relevant here.

No need to cycle through the rest when the minimum has been reached.

@pmconrad what did this mean?

abitmore · 2018-04-25T09:38:23Z

libraries/chain/db_maint.cpp

+ modify( d, [this](asset_bitasset_data_object& o) {
+ o.force_settled_volume = 0;
+ // Remove all expired feeds
+ for( auto itr = o.feeds.begin(); itr != o.feeds.end();)


Please use a reverse iterator for better performance while erasing multiple entries.

oxarbitrage · 2018-04-26T23:27:54Z

In the last commit(101a02c) the bsip code is changed to only affect smartcoins.

A few considerations:

A new index tag by_bitasset_id was added to the asset asset_index. This can be of use in other parts of code, specifically if you have the bitasset_id and want to get what asset it belongs to.
In db_maint.cpp: const auto& idx = get_index_type<asset_index>().indices().get<by_bitasset_id>(); is done inside lambda and by that inside a for loop. It will be more efficient to add this line before the loop but that will not compile as the indexes are protected. I am not sure how bad in performance is doing it like it is now. Suggestions are appreciated.
We can't exit the feeds loop before all feeds are checked, this is because the feeds are not sorted by date but by witness account id. this means, expired feed can be at any position of the map. Can be seen in bsip36_additional test case.
@abitmore please explain some more about how reverse_itr will increase performance in this scenario as i was not able to get it.
TODO: Add test for asset fed by committee.

thanks.

pmconrad · 2018-04-27T12:12:41Z

A new index tag by_bitasset_id was added to the asset asset_index.

Instead of this, you can find the original asset_id from o.feeds.begin()->settlement_price.base.asset_id . (If o.feeds is empty there's nothing to do anyway.)

oxarbitrage · 2018-04-27T14:28:21Z

Instead of this, you can find the original asset_id from o.feeds.begin()->settlement_price.base.asset_id . (If o.feeds is empty there's nothing to do anyway.)

thanks, i had this idea in a telegram chat and then i forgot about it. implemented now in 63199d0

abitmore · 2018-04-27T18:16:22Z

please explain some more about how reverse_itr will increase performance in this scenario as i was not able to get it.

@oxarbitrage you're removing elements from a flat_map, every removal will cost linear time depends on how many elements are after it since need to move them all. By using a reverse iterator, less elements will need to be moved when there are more than 1 element to be removed.

abitmore · 2018-04-27T18:20:15Z

libraries/chain/db_maint.cpp

+ o.force_settled_volume = 0;
+
+ // Check if asset is smartcoin
+ const auto& settlement_price = o.feeds.begin()->second.second.settlement_price;


This is not safe. Before dereferencing begin(), please check whether feeds is empty.

silly me, fixed here f60a953 thank you.

abitmore · 2018-04-27T18:25:24Z

libraries/chain/db_maint.cpp

@@ -925,7 +925,29 @@ void database::perform_chain_maintenance(const signed_block& next_block, const g
 // Reset all BitAsset force settlement volumes to zero
 for( const auto& d : get_index_type<asset_bitasset_data_index>().indices() )
 {
- modify( d, [](asset_bitasset_data_object& o) { o.force_settled_volume = 0; });
+ if(head_block_time() < HARDFORK_CORE_518_TIME)


For better performance, please move this check out of the for loop.

abitmore · 2018-04-27T18:27:51Z

libraries/chain/db_maint.cpp

+
+ // Check if asset is smartcoin
+ const auto& settlement_price = o.feeds.begin()->second.second.settlement_price;
+ if(!settlement_price.is_null()) {


This check is incorrect. The first feed is null doesn't mean all feeds are null.

#832 (comment)
you think that is wrong ? makes sense to me.

waiting for more comments from @pmconrad maybe.

abitmore · 2018-04-27T18:31:41Z

libraries/chain/db_maint.cpp

+ if ((flags & witness_fed_asset) || (flags & committee_fed_asset)) { // if smartcoin
+ for (auto itr = o.feeds.begin(); itr != o.feeds.end();) { // loop feeds
+ auto feed_time = itr->second.first;
+ if (feed_time + (o.options.feed_lifetime_sec) < head_block_time())


For better performance, please calculate head_block_time()-feed_lifetime_sec out of the for loop (and be careful about Integer overflow or underflow) and only check feed_time while looping.

done this one, checked for overflow only as we are dealing with all unsigned ints so underflow will not be possible. overflow could occur with a feed_lifetime_sec is too big. hard to happen in practice as when we are here we are in a smartcoin but check was added anyways.

abitmore · 2018-04-27T21:51:53Z

libraries/chain/db_maint.cpp

 // Reset all BitAsset force settlement volumes to zero
 for( const auto& d : get_index_type<asset_bitasset_data_index>().indices() )
 {
- modify( d, [](asset_bitasset_data_object& o) { o.force_settled_volume = 0; });
+ if(!hf518)


I prefer this structure:

if ( hf ) for else for

changed to it.

abitmore · 2018-04-28T10:40:41Z

libraries/chain/db_maint.cpp

+ }
+ });
+ if (d.has_settlement())
+ process_bids(d);


Hmm, this is a bit ugly (duplicate code). But I guess it's better to live with it at this moment.

I'd rather avoid the duplicate code. Why not pull the HF check into the lambda?

@pmconrad it's a side-effect of one of my earlier requests, since hard fork time check is independent, for slightly better performance, moved it to out of for.

abitmore

Actually I think it's better to move the whole code to a new function since it's getting a bit too long.

abitmore · 2018-04-28T10:43:21Z

libraries/chain/db_maint.cpp

+ }
+ else {
+ for( const auto& d : get_index_type<asset_bitasset_data_index>().indices() ) {
+ modify(d, [](asset_bitasset_data_object &o) { o.force_settled_volume = 0; });


Better leave the "Reset all BitAsset force settlement volumes to zero" comment here.

oxarbitrage · 2018-04-28T14:09:15Z

moved code to new function.

oxarbitrage · 2018-04-29T00:23:32Z

as discussed in telegram i now added asset_id to the bitasset object in this commit: 09b7ffb

will need replay of course as this is added on object creation.

abitmore

Thanks. Looks good to me, except the coding style (most code elsewhere has { in a new line). Approving anyway.

@pmconrad what's your opinion?

pmconrad · 2018-05-01T09:49:25Z

libraries/chain/db_maint.cpp

+ }
+ });
+ if (d.has_settlement())
+ process_bids(d);


I'd rather avoid the duplicate code. Why not pull the HF check into the lambda?

pmconrad · 2018-05-01T10:02:29Z

libraries/chain/db_maint.cpp

+ if ((flags & witness_fed_asset) || (flags & committee_fed_asset)) // if smartcoin
+ {
+ // check overflow
+ if (std::numeric_limits<uint32_t>::max() - o.options.feed_lifetime_sec > head_block_time().sec_since_epoch())


I think this is wrong. Two lines below you calculate (head_block_time - feed_lifetime), so what you want to avoid is feed_lifetime > head_block_time.
Also, please move this check into the if two lines above.

(Good thinking to put in the check though.)

@pmconrad i think you are right and making sure feed_lifetime < head is enough. changed here 960bfe9 and moved check to the previous if. thanks.

pmconrad · 2018-05-01T10:03:20Z

libraries/chain/hardfork.d/CORE_518.hf

+// bitshares-core issue #518 Clean up bitasset_data during maintenance
+#ifndef HARDFORK_CORE_518_TIME
+#define HARDFORK_CORE_518_TIME (fc::time_point_sec( 1600000000 ))
+#endif


Please add a newline.

pmconrad · 2018-05-01T10:04:38Z

tests/tests/smartcoin_tests.cpp

+{
+ try
+ {
+ /* Bug affects only smartcoions(market pegged assets feeded by active witnesses or committee members).


It's not a bug, it's an issue. :-)

BTW smartcoions is typo.

abitmore · 2018-05-03T13:04:29Z

libraries/chain/db_maint.cpp

@@ -778,20 +778,19 @@ void database::process_bitassets()

 const auto &asset = get(o.asset_id);
 auto flags = asset.options.flags;
- if ((flags & witness_fed_asset) || (flags & committee_fed_asset)) // if smartcoin
+ if ( (flags & witness_fed_asset || flags & committee_fed_asset) &&


To make sure & will be done before ||, it's not harm to leave the ()s.
BTW as @jmjatlanta mentioned, perhaps better to use ( flag & ( witness_fed_asset | committee_fed_asset ) )

abitmore · 2018-05-03T13:12:16Z

Please resolve conflicts.

abitmore · 2018-05-07T09:07:47Z

Resolved conflicts and merged into a new bsip36 branch. Will develop in that branch.

initial bsip35

2e1f07d

oxarbitrage mentioned this pull request Apr 7, 2018

clean feeds at maint time #598

Closed

abitmore added this to the 201805 - Consensus Changing Release milestone Apr 8, 2018

abitmore added the hardfork label Apr 8, 2018

add update_feed_producers test

10c7e8f

xeroc approved these changes Apr 13, 2018

View reviewed changes

pmconrad suggested changes Apr 16, 2018

View reviewed changes

abitmore mentioned this pull request Apr 25, 2018

Clear price feed data after updated a bitAsset's backing asset ID #868

Closed

abitmore requested changes Apr 25, 2018

View reviewed changes

change bsip code to only affect smartcoins

101a02c

get asset_id from first feed instead of index tag

63199d0

abitmore reviewed Apr 27, 2018

View reviewed changes

fix performance and some errors

f60a953

abitmore reviewed Apr 27, 2018

View reviewed changes

change hardfork structure

67e1ec4

abitmore reviewed Apr 28, 2018

View reviewed changes

move code of bsip to new function

c93913c

add asset_id to bitasset object

09b7ffb

abitmore approved these changes Apr 30, 2018

View reviewed changes

change coding style

bcea865

pmconrad suggested changes May 1, 2018

View reviewed changes

oxarbitrage added 2 commits May 2, 2018 19:42

typo fix

81bdfa3

change overflow test

960bfe9

abitmore reviewed May 3, 2018

View reviewed changes

abitmore changed the base branch from hardfork to bsip36 May 7, 2018 08:49

abitmore merged commit 960bfe9 into bitshares:bsip36 May 7, 2018

abitmore mentioned this pull request May 7, 2018

Bsip36: clear expired feeds on maintenance interval #889

Merged

pmconrad mentioned this pull request Feb 19, 2019

Buggy feeds (may) break MPAs #1602

Closed

BSIP 36 Candidate #832

BSIP 36 Candidate #832

Conversation

oxarbitrage commented Apr 7, 2018

abitmore commented Apr 8, 2018

oxarbitrage commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Apr 24, 2018

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Apr 26, 2018

pmconrad commented Apr 27, 2018

oxarbitrage commented Apr 27, 2018

abitmore commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Apr 28, 2018

oxarbitrage commented Apr 29, 2018

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitmore commented May 3, 2018

abitmore commented May 7, 2018