Bsip 0018 #340

pmconrad · 2017-07-29T08:59:02Z

This is my initial implementation of BSIP-0018.

…val after price recovery

Use writable fee parameters for account_create only Fixed fee calculation

Added missing reflections

pmconrad · 2017-07-29T12:06:43Z

Resolves #216

xeroc · 2017-07-29T18:31:37Z

libraries/app/database_api.cpp

+{ try {
+   FC_ASSERT( limit <= 100 );
+   const asset_object& swan = asset_id(_db);
+   FC_ASSERT( swan.is_market_issued() );


should we not also assert that swan has a settlement fund > 0

This is just an accessor method. I think it will simplify things if we're lenient here. The call will return an empty list if the requested asset is not in global settlement. (This is technically correct.)

xeroc · 2017-07-29T18:37:05Z

libraries/chain/asset_evaluator.cpp

+         if( !bad.current_feed.settlement_price.is_null()
+             && ~price::call_price(asset(mia_dyn.current_supply, o.asset_id),
+                                   asset(bad.settlement_fund, bad.options.short_backing_asset),
+                                   bad.current_feed.maintenance_collateral_ratio ) < bad.current_feed.settlement_price )


are we 100% sure about the < at this place? is this correct for any comvination of assetid and short backing asset?

The feed price is defined to be debt/collateral, see asset.hpp. The call price is computed as the inverse of debt/collateral*ratio. So if the inverted call price is lower than the feed, it means that we have less debt per collateral than required.

xeroc · 2017-07-29T18:42:06Z

libraries/chain/db_market.cpp

+   auto to_short = bdd.current_supply;
+   auto collateral = bad.settlement_fund;
+   if( collateral > 0 )
+      adjust_balance( bitasset.issuer, asset( collateral, bad.options.short_backing_asset ) );


why does it say bitasset.issuer here?

xeroc · 2017-07-29T18:43:16Z

libraries/chain/db_market.cpp

+   auto to_short = bdd.current_supply;
+   auto collateral = bad.settlement_fund;
+   if( collateral > 0 )
+      adjust_balance( bitasset.issuer, asset( collateral, bad.options.short_backing_asset ) );


oh, I assume after settlement all supply is 'shorted' by the issuer .. correct?

Right, this is the part where auto-revival happens after the feed has recovered. A call_order is created from debt and collateral, and this call_order belongs to the issuer.

This means the issuer will get the 75% extra collateral for free, which didn't worth that much when the black swan occurred.
Usually this won't happen, because usually bidders will cover the debt before the collateral ratio raised so high, asset holders may also have settled, unless the debt amount is too huge to cover, and the holders forgot to settle or lost the key, etc.
I suggest that we fulfill the bids first rather than sending the whole collateral to the issuer.

xeroc · 2017-07-29T18:48:24Z

libraries/chain/db_market.cpp

+   remove(bid);
+}
+
+void database::execute_bid( const collateral_bid_object& bid, share_type debt_covered, share_type collateral_from_fund, const price_feed& current_feed )


assuming this is a virtual operation, why does it not have the if condition for 'create_virtual_ops'?

I think I left it out because I always want to create the virtual op.

xeroc · 2017-07-29T18:54:29Z

libraries/chain/market_evaluator.cpp

+
+   if( o.additional_collateral.amount > 0 )
+   {
+      FC_ASSERT( d.get_balance(*_paying_account, _bitasset_data->options.short_backing_asset(d)) >= o.additional_collateral,


shouldnt fee be in here as well?

Fees are handled globally, in evaluator.cpp, not separately in each evaluator.

xeroc · 2017-07-29T18:57:48Z

libraries/chain/market_evaluator.cpp

+   const collateral_bid_index& bids = d.get_index_type<collateral_bid_index>();
+   const auto& index = bids.indices().get<by_account>();
+   const auto& bid = index.find( boost::make_tuple( o.debt_covered.asset_id, o.bidder ) );
+   if( bid != index.end() )


i am not sure i understand this.
How can i bid if this evaluator asserts that i can update an existing bid? am i reading this wrong?

o.debt_covered.amount == 0 means that we only want to cancel an existing bid (which means a bid must already exist).
o.debt_covered.amount > 0 means create-or-update.
If you invert the logic you arrive at "if bid exists then fine else assert o.debt_covered.amount > 0".

xeroc · 2017-07-29T18:59:05Z

libraries/chain/market_evaluator.cpp

+   if( o.debt_covered.amount == 0 ) return void_result();
+
+   d.adjust_balance( o.bidder, -o.additional_collateral  );
+


do i not need to credit for the previous bid?

That already happened in d.cancel_bid (called on line 321).

xeroc

Looks good to me

abitmore · 2017-08-01T09:49:00Z

libraries/app/database_api.cpp

+   const auto& idx = _db.get_index_type<collateral_bid_index>();
+   const auto& aidx = idx.indices().get<by_price>();
+   auto start = aidx.lower_bound( boost::make_tuple( asset_id, price::max(back.id, asset_id), collateral_bid_id_type(GRAPHENE_DB_MAX_INSTANCE_ID) ) );
+   auto end = aidx.lower_bound( boost::make_tuple( asset_id, price::min(back.id, asset_id), collateral_bid_id_type(GRAPHENE_DB_MAX_INSTANCE_ID) ) );


It's strange that both start and end use GRAPHENE_DB_MAX_INSTANCE_ID, although technically it's not a big deal.

abitmore · 2017-08-01T10:44:45Z

libraries/chain/db_market.cpp

+   auto to_short = bdd.current_supply;
+   auto collateral = bad.settlement_fund;
+   if( collateral > 0 )
+      adjust_balance( bitasset.issuer, asset( collateral, bad.options.short_backing_asset ) );


This means the issuer will get the 75% extra collateral for free, which didn't worth that much when the black swan occurred.
Usually this won't happen, because usually bidders will cover the debt before the collateral ratio raised so high, asset holders may also have settled, unless the debt amount is too huge to cover, and the holders forgot to settle or lost the key, etc.
I suggest that we fulfill the bids first rather than sending the whole collateral to the issuer.

abitmore · 2017-08-01T10:48:48Z

libraries/chain/db_market.cpp

+   const asset_bitasset_data_object& bad = bitasset.bitasset_data(*this);
+   FC_ASSERT( bad.has_settlement() );
+   const asset_dynamic_data_object& bdd = bitasset.dynamic_asset_data_id(*this);
+   FC_ASSERT( shorter != account_id_type() );


account_id_type() is committee-account which should be allowed here. When this function is called from asset_publish_feeds_evaluator::do_apply(), for example if the asset is bitUSD, the issuer is committee-account.

By the way, why we need this shorter parameter when revive_bitasset() will only be called in asset_publish_feeds_evaluator which always use the issuer as the parameter?

Will remove the shorter parameter and the assertion. It's a leftover from an earlier version. revive_bitasset is only used for auto-revival after the feed has recovered, and it always creates a call_order that belongs to the issuer.

abitmore · 2017-08-01T15:36:25Z

libraries/chain/market_evaluator.cpp

+{ try {
+   database& d = db();
+
+   FC_ASSERT( d.head_block_time() >= HARDFORK_CORE_216_TIME, "Not yet!" );


I think > is better here. Be consistent.

By the way, do we need to check for asset white-listing/black-listing?

Will fix >.

I don't think white/blacklisting are relevant here, because you can only bid BACK that you already have, and you'll only ever get BACK out again (possibly more than you put in). You can't transfer or trade BACK or SWAN here, and you cannot get SWAN out.

pmconrad · 2017-08-01T20:35:35Z

Thanks @abitmore !

Note that in revive_bitasset the issuer receives the collateral, but only to use it up in the call_order_update a few lines further down. The idea here is that the issuer will probably have the interests of the asset holders in mind, and will hopefully leave the collateral alone. Bidders usually have their own interests in mind and are likely to take their profits and leave.

Now that I'm writing this I think revive_bitasset should be modified to call execute_bid instead of using adjust_balance and call_order_update_operation. Will think about this some more.

…mmon code into _cancel_bids_and_revive_mpa

oxarbitrage · 2017-09-01T23:11:50Z

libraries/app/api.cpp

@@ -268,150 +268,6 @@ namespace graphene { namespace app {
       return *_debug_api;
    }

-    vector<account_id_type> get_relevant_accounts( const object* obj )


good job removing this from api.cpp, no needed at all.

oxarbitrage · 2017-09-04T14:49:20Z

tests/tests/swan_tests.cpp

+ * Black swan occurs when price feed falls, triggered by settlement
+ * order.
+ */
+BOOST_AUTO_TEST_CASE( black_swan_issue_346 )


unsure about the reference to issue 346 here, it seems to not be related in bitshares-core or cryptonomex repos.

This was moved here from the original https://github.com/bitshares/bitshares-core/blob/master/tests/tests/operation_tests.cpp#L287 , I didn't check what the 346 refers to. It seems that it indeed references the cryptonomes repo - cryptonomex/graphene@9d5a5dd

you are right it actually came from here cryptonomex/graphene#346, at first saw i thought it was unrelated but going lower into the issue bytemaster explains. thanks.

oxarbitrage · 2017-09-04T16:05:44Z

the pull adds 14 test cases for black swan scenarios. they are at tests/swan_tests.cpp
all passing.

can run them all by:

./chain_test -t swan_tests -l all

or individually:

./chain_test -t swan_tests/recollateralize -l all

oxarbitrage · 2017-09-04T16:10:57Z

libraries/chain/include/graphene/chain/protocol/fee_schedule.hpp

@@ -34,6 +34,49 @@ namespace graphene { namespace chain {
   };
   typedef transform_to_fee_parameters<operation>::type fee_parameters;

+   template<typename Operation>
+   class fee_helper {


unsure about why the fee_helper was needed to be implemented, sure it haves a reason but can you explain a bit further this and about why the account_create_operation and the new created bid_collateral_operation are special ?

The challenge here was to create a fee that defaults to the value of another fee, as opposed to a static default. For this I needed to specialize the function template, and trying to do that I ran into the trap that partial specialization of certain templates is not allowed. The solution was to introduce a new class fee_helper that can be specialized as needed.

account_create is special because it is the only operation that uses the non-const get().
bid_collateral is special because the fee defaults to the current value of call_order_update.

got it, thanks!

oxarbitrage · 2017-09-04T19:45:28Z

error cant push more blocks in a new chain with this pull code, part of the output:

2475243ms th_a       application.cpp:551           handle_block         ] Got block: #170000 time: 2015-10-19T15:20:06 latency: 59286069243 ms from: fox  irreversible: 169975 (-25)
2475969ms th_a       db_market.cpp:576             check_call_orders    ] Feed protected margin call executing (HARDFORK_436_TIME not here yet)
2475969ms th_a       db_market.cpp:577             check_call_orders    ] *call_itr: {"id":"1.8.524","borrower":"1.2.91277","collateral":40419011,"debt":8425,"call_price":{"base":{"amount":161676044,"asset_id":"1.3.0"},"quote":{"amount":58975,"asset_id":"1.3.121"}}} 
2475969ms th_a       db_market.cpp:578             check_call_orders    ] *limit_itr: {"id":"1.7.988","expiration":"2020-10-19T16:46:54","seller":"1.2.22517","for_sale":500000,"sell_price":{"base":{"amount":500000,"asset_id":"1.3.121"},"quote":{"amount":1377715550,"asset_id":"1.3.0"}},"deferred_fee":0} 
2475969ms th_a       db_market.cpp:576             check_call_orders    ] Feed protected margin call executing (HARDFORK_436_TIME not here yet)
2475969ms th_a       db_market.cpp:577             check_call_orders    ] *call_itr: {"id":"1.8.1018","borrower":"1.2.91890","collateral":575746364,"debt":120000,"call_price":{"base":{"amount":143936591,"asset_id":"1.3.0"},"quote":{"amount":52500,"asset_id":"1.3.121"}}} 
2475969ms th_a       db_market.cpp:578             check_call_orders    ] *limit_itr: {"id":"1.7.988","expiration":"2020-10-19T16:46:54","seller":"1.2.22517","for_sale":491575,"sell_price":{"base":{"amount":500000,"asset_id":"1.3.121"},"quote":{"amount":1377715550,"asset_id":"1.3.0"}},"deferred_fee":0} 
2475969ms th_a       db_market.cpp:576             check_call_orders    ] Feed protected margin call executing (HARDFORK_436_TIME not here yet)
2475970ms th_a       db_market.cpp:577             check_call_orders    ] *call_itr: {"id":"1.8.819","borrower":"1.2.91572","collateral":40607101,"debt":8457,"call_price":{"base":{"amount":162428404,"asset_id":"1.3.0"},"quote":{"amount":59199,"asset_id":"1.3.121"}}} 
2475970ms th_a       db_market.cpp:578             check_call_orders    ] *limit_itr: {"id":"1.7.988","expiration":"2020-10-19T16:46:54","seller":"1.2.22517","for_sale":371575,"sell_price":{"base":{"amount":500000,"asset_id":"1.3.121"},"quote":{"amount":1377715550,"asset_id":"1.3.0"}},"deferred_fee":0} 
2479627ms th_a       db_block.cpp:202              _push_block          ] Failed to push new block:
10 assert_exception: Assert Exception
itr != parameters.end(): 
    {}
    th_a  fee_schedule.hpp:43 cget

    {}
    th_a  evaluator.cpp:51 start_evaluate

    {"op":[40,{"fee":{"amount":1500000,"asset_id":"1.3.0"},"inputs":[{"commitment":"029d7014fe9b718cfe8d08c27981ea8a7564cdd8d3b7ff12274372fecd01bae858","owner":{"weight_threshold":1,"account_auths":[],"key_auths":[["BTS6ZQ8hdkRwu2D6qjo47KtEHiYWw3z83vqpE3o74pc6dgZU5vZgE",1]],"address_auths":[]}}],"outputs":[{"commitment":"031f515f9fdacc067114d8ea05f72135741a773b80478aac1a643905500afc16e4","range_proof":"4019800b7b290590afcd4f450c12a6816869de02f001754c158cc87a259747d70325b8b2d334f82bebe585d78653a8ea15ef068fcf6899abbf1bafb636ad6b6142a4b52f9c44a3852d3a8855b912e23546c5ac4119b4afa175fb1890a767231efbb76968763b9e34ac77af8ae91b50e87dd9585718173c73f98c8f94be64d8fe5396023fa44f97a2431909b2660e4fe6c04f2e8fcb

this is after block 170000, seems to be some problem with the fees. do i need to change some hardfork date ? this was just started with everything default.

pmconrad · 2017-09-04T20:19:37Z

WTF? Will check.

oxarbitrage · 2017-09-04T20:44:09Z

hold on, i messed up with the code of another node ... edit: not really, i am in the correct place. it seems to be failing there.

pmconrad · 2017-09-05T20:02:57Z

Found and fixed this one, but ran into another special case. Still working on it.

pmconrad · 2017-09-06T05:32:21Z

Replayed successfully with hf date in the future and in the past as well

oxarbitrage · 2017-09-06T20:12:00Z

the new fee tests look good, the chain now replays fine too.

i was checking a bit the api calls and wallet, we have 1 node new api call get_collateral_bids. all empty of course in the main net.

against cny:

alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull340/bitshares-core/tests $ curl --data '{"jsonrpc": "2.0", "params": ["database", "get_collateral_bids", ["1.3.113", 100, 0]], "method": "call", "id": 10}' http://localhost:8090/rpc --silent | jq
{
  "id": 10,
  "jsonrpc": "2.0",
  "result": []
}
alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull340/bitshares-core/tests $

against ALTCAP.X:

alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull340/bitshares-core/tests $ curl --data '{"jsonrpc": "2.0", "params": ["database", "get_collateral_bids", ["1.3.985", 100, 0]], "method": "call", "id": 10}' http://localhost:8090/rpc --silent | jq
{
  "id": 10,
  "jsonrpc": "2.0",
  "result": []
}
alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull340/bitshares-core/tests $

against non MPA assert:

"database","get_collateral_bids",["1.3.1439",100,0]]}}]}}}alfredo@alfredo-Inspiron-5559 ~/CLionProjects/pull340/bitshares-core/tests $ curl --data '{"jsonrpc": "2.0", "100, 0]], "method": "call", "id": 10}' http://localhost:8090/rpc --silent | jq
{
  "id": 10,
  "jsonrpc": "2.0",
  "error": {
    "code": 1,
    "message": "Assert Exception: swan.is_market_issued(): ",
...

Then in the cli_wallet we have 2 calls. First we have get_collateral_bids just exposing the previous node call and then we have bid_collateral. this is expected to do what described in "The operation works as follows:" section of the BSIP.

new >>> get_collateral_bids ALTCAP.X 100 1
get_collateral_bids ALTCAP.X 100 1
[]
new >>> 
new >>> bid_collateral oxarbitrage.a699 100 ALTCAP.X 100 false
bid_collateral oxarbitrage.a699 100 ALTCAP.X 100 false
{
  "ref_block_num": 26794,
  "ref_block_prefix": 554938416,
  "expiration": "2017-09-06T19:51:57",
  "operations": [[
      45,{
        "fee": {
          "amount": 1213,
          "asset_id": "1.3.0"
        },
        "bidder": "1.2.152768",
        "additional_collateral": {
          "amount": "10000000000",
          "asset_id": "1.3.103"
        },
        "debt_covered": {
          "amount": "10000000000",
          "asset_id": "1.3.985"
        },
        "extensions": []
      }
    ]
  ],
  "extensions": [],
  "signatures": []
}
new >>>

We need to make sure the fees are charged only after the operation checks pass, in the list of the BSIP it says it will be the first thing to do.

It is important to note that anyone will be able to do a revive, i am ok with this.

Next step is to send the code to a testnet and revive MPAs in blackswan there to check the process.

pmconrad · 2017-09-06T21:18:09Z

Great, thanks for your work!
Please merge this one and the other three HF PRs into develop, then I'll prepare a testnet release.

oxarbitrage · 2017-09-06T21:50:11Z

merged everything, now checking if develop compiles and replies(it should).

oxarbitrage · 2017-09-07T01:53:46Z

new develop compiled and replayed fully.

pmconrad and others added 11 commits July 27, 2017 13:50

Added test case for settlement bug described in cryptonomex#216

5c5424d

Fixed settlement bug cryptonomex#216

894d5b6

Allow price feed (for smartcoins) after a black swan

5bd8cc4

Implemented revival of empty assets + revival when price recovers

55664d2

Refactored swan-related tests into separate file, added test for revi…

cf048e9

…val after price recovery

Refactored common code into swan_fixture

1fe819e

Removed unused duplicate code

93f01ff

Implemented bid_collateral_operation + unit test

1bdb651

Use writable fee parameters for account_create only Fixed fee calculation

Fixed compiler warning

eff35c6

Implemented bid processing + test case

4f8253a

Implemented wallet api calls "bid_collateral" and "get_collateral_bids"

cbb2e55

Added missing reflections

xeroc reviewed Jul 29, 2017

View reviewed changes

xeroc approved these changes Jul 30, 2017

View reviewed changes

abitmore reviewed Aug 1, 2017

View reviewed changes

pmconrad added 3 commits August 1, 2017 20:54

Be consistent wrt HF time

e861a4f

Removed redundant parameter from revive_bitasset

24252e2

Fixed lower bound in get_collateral_bids

6cfb4d4

pmconrad added 3 commits August 2, 2017 11:51

Fixed collateral asset id in execute_bid_operation

4f8cffa

Rewrote revive_bitasset in terms of create+execute_bid, refactored co…

025050b

…mmon code into _cancel_bids_and_revive_mpa

Added tests for some edge cases and handle rounding issues properly

8bc5498

pmconrad added the hardfork label Aug 20, 2017

oxarbitrage mentioned this pull request Aug 31, 2017

added new asset permission flags pay_fees_core_only and exchange_restricted #380

Open

pmconrad mentioned this pull request Aug 31, 2017

database close unexpected exception #385

Closed

oxarbitrage self-requested a review September 1, 2017 22:37

oxarbitrage reviewed Sep 1, 2017

View reviewed changes

oxarbitrage reviewed Sep 4, 2017

View reviewed changes

pmconrad added 3 commits September 5, 2017 19:56

Updated hardfork date for testing

c774987

Added test case to verify correct handling of unset fees

a7d7dcb

Fixed handling of unset fees

52f6347

Added test case + fix for prediction market with 0 outcome

36e8b77

oxarbitrage approved these changes Sep 6, 2017

View reviewed changes

oxarbitrage merged commit dd8da84 into bitshares:develop Sep 6, 2017

oxarbitrage mentioned this pull request Sep 19, 2017

Process to Reset a SmartCoin after a Blackswan Event #216

Closed

		if( o.debt_covered.amount == 0 ) return void_result();

		d.adjust_balance( o.bidder, -o.additional_collateral );

Bsip 0018 #340

Bsip 0018 #340

Conversation

pmconrad commented Jul 29, 2017

pmconrad commented Jul 29, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeroc 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

pmconrad commented Aug 1, 2017

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 Sep 4, 2017

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 Sep 4, 2017

pmconrad commented Sep 4, 2017

oxarbitrage commented Sep 4, 2017 • edited Loading

pmconrad commented Sep 5, 2017

pmconrad commented Sep 6, 2017

oxarbitrage commented Sep 6, 2017

pmconrad commented Sep 6, 2017

oxarbitrage commented Sep 6, 2017

oxarbitrage commented Sep 7, 2017

oxarbitrage commented Sep 4, 2017 •

edited

Loading