BSIP35 mitigate rounding issue when matching orders

libraries/chain/asset_evaluator.cpp

@@ -508,33 +508,54 @@ void_result asset_settle_evaluator::do_evaluate(const asset_settle_evaluator::op
 operation_result asset_settle_evaluator::do_apply(const asset_settle_evaluator::operation_type& op)
 { try {
    database& d = db();
-   d.adjust_balance(op.account, -op.amount);
 
    const auto& bitasset = asset_to_settle->bitasset_data(d);
    if( bitasset.has_settlement() )
    {
       const auto& mia_dyn = asset_to_settle->dynamic_asset_data_id(d);
 
-      auto settled_amount = op.amount * bitasset.settlement_price;
+      auto settled_amount = op.amount * bitasset.settlement_price; // round down, in favor of global settlement fund
       if( op.amount.amount == mia_dyn.current_supply )
          settled_amount.amount = bitasset.settlement_fund; // avoid rounding problems
       else
          FC_ASSERT( settled_amount.amount <= bitasset.settlement_fund ); // should be strictly < except for PM with zero outcome
 
-      d.modify( bitasset, [&]( asset_bitasset_data_object& obj ){
-                obj.settlement_fund -= settled_amount.amount;
-                });
+      if( settled_amount.amount == 0 && !bitasset.is_prediction_market )
+      {
+         if( d.get_dynamic_global_properties().next_maintenance_time > HARDFORK_CORE_184_TIME )
+            FC_THROW( "Settle amount is too small to receive anything due to rounding" );
+         else // TODO remove this warning after hard fork core-184
+            wlog( "Something for nothing issue (#184, variant F) occurred at block #${block}", ("block",d.head_block_num()) );
+      }
+
+      asset pays = op.amount;
+      if( op.amount.amount != mia_dyn.current_supply
+            && settled_amount.amount != 0
+            && d.get_dynamic_global_properties().next_maintenance_time > HARDFORK_CORE_342_TIME )
+      {
+         pays = settled_amount.multiply_and_round_up( bitasset.settlement_price );
+      }
 
-      d.adjust_balance(op.account, settled_amount);
+      d.adjust_balance( op.account, -pays );
+
+      if( settled_amount.amount > 0 )
+      {
+         d.modify( bitasset, [&]( asset_bitasset_data_object& obj ){
+            obj.settlement_fund -= settled_amount.amount;
+         });
+
+         d.adjust_balance( op.account, settled_amount );
+      }
 
       d.modify( mia_dyn, [&]( asset_dynamic_data_object& obj ){
-                obj.current_supply -= op.amount.amount;
-                });
+         obj.current_supply -= pays.amount;
+      });
 
       return settled_amount;
    }
    else
    {
+      d.adjust_balance( op.account, -op.amount );
       return d.create<force_settlement_object>([&](force_settlement_object& s) {
          s.owner = op.account;
          s.balance = op.amount;

libraries/chain/db_update.cpp

@@ -245,15 +245,17 @@ bool database::check_for_blackswan( const asset_object& mia, bool enable_black_s
     if( ~least_collateral >= highest  ) 
     {
        wdump( (*call_itr) );
-       elog( "Black Swan detected: \n"
+       elog( "Black Swan detected on asset ${symbol} (${id}) at block ${b}: \n"
              "   Least collateralized call: ${lc}  ${~lc}\n"
            //  "   Highest Bid:               ${hb}  ${~hb}\n"
              "   Settle Price:              ${~sp}  ${sp}\n"
              "   Max:                       ${~h}  ${h}\n",
+            ("id",mia.id)("symbol",mia.symbol)("b",head_block_num())
             ("lc",least_collateral.to_real())("~lc",(~least_collateral).to_real())
           //  ("hb",limit_itr->sell_price.to_real())("~hb",(~limit_itr->sell_price).to_real())
             ("sp",settle_price.to_real())("~sp",(~settle_price).to_real())
             ("h",highest.to_real())("~h",(~highest).to_real()) );
+       edump((enable_black_swan));
        FC_ASSERT( enable_black_swan, "Black swan was detected during a margin update which is not allowed to trigger a blackswan" );
        if( maint_time > HARDFORK_CORE_338_TIME && ~least_collateral <= settle_price )
           // global settle at feed price if possible
@@ -270,14 +272,19 @@ void database::clear_expired_orders()
          //Cancel expired limit orders
          auto head_time = head_block_time();
          auto maint_time = get_dynamic_global_properties().next_maintenance_time;
+
+         bool before_core_hardfork_184 = ( maint_time <= HARDFORK_CORE_184_TIME ); // something-for-nothing
+         bool before_core_hardfork_342 = ( maint_time <= HARDFORK_CORE_342_TIME ); // better rounding
+         bool before_core_hardfork_606 = ( maint_time <= HARDFORK_CORE_606_TIME ); // feed always trigger call
+
          auto& limit_index = get_index_type<limit_order_index>().indices().get<by_expiration>();
          while( !limit_index.empty() && limit_index.begin()->expiration <= head_time )
          {
             const limit_order_object& order = *limit_index.begin();
             auto base_asset = order.sell_price.base.asset_id;
             auto quote_asset = order.sell_price.quote.asset_id;
             cancel_limit_order( order );
-            if( maint_time <= HARDFORK_CORE_606_TIME )
+            if( before_core_hardfork_606 )
             {
                // check call orders
                // Comments below are copied from limit_order_cancel_evaluator::do_apply(...)
@@ -296,9 +303,11 @@ void database::clear_expired_orders()
       asset_id_type current_asset = settlement_index.begin()->settlement_asset_id();
       asset max_settlement_volume;
       price settlement_fill_price;
+      price settlement_price;
+      bool current_asset_finished = false;
       bool extra_dump = false;
 
-      auto next_asset = [&current_asset, &settlement_index, &extra_dump] {
+      auto next_asset = [&current_asset, &current_asset_finished, &settlement_index, &extra_dump] {
          auto bound = settlement_index.upper_bound(current_asset);
          if( bound == settlement_index.end() )
          {
@@ -313,6 +322,7 @@ void database::clear_expired_orders()
             ilog( "next_asset returning true, bound is ${b}", ("b", *bound) );
          }
          current_asset = bound->settlement_asset_id();
+         current_asset_finished = false;
          return true;
       };
 
@@ -368,7 +378,9 @@ void database::clear_expired_orders()
          }
          if( max_settlement_volume.asset_id != current_asset )
             max_settlement_volume = mia_object.amount(mia.max_force_settlement_volume(mia_object.dynamic_data(*this).current_supply));
-         if( mia.force_settled_volume >= max_settlement_volume.amount )
+         // When current_asset_finished is true, this would be the 2nd time processing the same order.
+         // In this case, we move to the next asset.
+         if( mia.force_settled_volume >= max_settlement_volume.amount || current_asset_finished )
          {
             /*
             ilog("Skipping force settlement in ${asset}; settled ${settled_volume} / ${max_volume}",
@@ -386,24 +398,23 @@ void database::clear_expired_orders()
             break;
          }
 
-         auto& pays = order.balance;
-         auto receives = (order.balance * mia.current_feed.settlement_price);
-         receives.amount = (fc::uint128_t(receives.amount.value) *
-                            (GRAPHENE_100_PERCENT - mia.options.force_settlement_offset_percent) / GRAPHENE_100_PERCENT).to_uint64();
-         assert(receives <= order.balance * mia.current_feed.settlement_price);
-
-         price settlement_price = pays / receives;
-
-         // Calculate fill_price with a bigger volume to reduce impacts of rounding
-         // TODO replace the calculation with new operator*() and/or operator/()
          if( settlement_fill_price.base.asset_id != current_asset ) // only calculate once per asset
+            settlement_fill_price = mia.current_feed.settlement_price
+                                    / ratio_type( GRAPHENE_100_PERCENT - mia.options.force_settlement_offset_percent,
+                                                  GRAPHENE_100_PERCENT );
+
+         if( before_core_hardfork_342 )
          {
-            asset tmp_pays = max_settlement_volume;
-            asset tmp_receives = tmp_pays * mia.current_feed.settlement_price;
-            tmp_receives.amount = (fc::uint128_t(tmp_receives.amount.value) *
-                            (GRAPHENE_100_PERCENT - mia.options.force_settlement_offset_percent) / GRAPHENE_100_PERCENT).to_uint64();
-            settlement_fill_price = tmp_pays / tmp_receives;
+            auto& pays = order.balance;
+            auto receives = (order.balance * mia.current_feed.settlement_price);
+            receives.amount = ( fc::uint128_t(receives.amount.value) *
+                                (GRAPHENE_100_PERCENT - mia.options.force_settlement_offset_percent) /
+                                GRAPHENE_100_PERCENT ).to_uint64();
+            assert(receives <= order.balance * mia.current_feed.settlement_price);
+            settlement_price = pays / receives;
          }
+         else if( settlement_price.base.asset_id != current_asset ) // only calculate once per asset
+            settlement_price = settlement_fill_price;
 
          auto& call_index = get_index_type<call_order_index>().indices().get<by_collateral>();
          asset settled = mia_object.amount(mia.force_settled_volume);
@@ -423,10 +434,28 @@ void database::clear_expired_orders()
                break;
             }
             try {
-               settled += match(*itr, order, settlement_price, max_settlement, settlement_fill_price);
+               asset new_settled = match(*itr, order, settlement_price, max_settlement, settlement_fill_price);
+               if( !before_core_hardfork_184 && new_settled.amount == 0 ) // unable to fill this settle order
+               {
+                  if( find_object( order_id ) ) // the settle order hasn't been cancelled
+                     current_asset_finished = true;
+                  break;
+               }
+               settled += new_settled;
+               // before hard fork core-342, `new_settled > 0` is always true, we'll have:
+               // * call order is completely filled (thus itr will change in next loop), or
+               // * settle order is completely filled (thus find_object(order_id) will be false so will break out), or
+               // * reached max_settlement_volume limit (thus new_settled == max_settlement so will break out).
+               //
+               // after hard fork core-342, if new_settled > 0, we'll have:
+               // * call order is completely filled (thus itr will change in next loop), or
+               // * settle order is completely filled (thus find_object(order_id) will be false so will break out), or
+               // * reached max_settlement_volume limit, but it's possible that new_settled < max_settlement,
+               //   in this case, new_settled will be zero in next iteration of the loop, so no need to check here.
             } 
             catch ( const black_swan_exception& e ) { 
-               wlog( "black swan detected: ${e}", ("e", e.to_detail_string() ) );
+               wlog( "Cancelling a settle_order since it may trigger a black swan: ${o}, ${e}",
+                     ("o", order)("e", e.to_detail_string()) );
                cancel_settle_order( order );
                break;
             }

libraries/chain/hardfork.d/CORE_184.hf

@@ -0,0 +1,4 @@
+// bitshares-core issue #184 Fix "Potential something-for-nothing fill bug"
+#ifndef HARDFORK_CORE_184_TIME
+#define HARDFORK_CORE_184_TIME (fc::time_point_sec( 1600000000 ))
+#endif

libraries/chain/hardfork.d/CORE_342.hf

@@ -0,0 +1,5 @@
+// bitshares-core issue #342
+// Mitigate rounding issue when matching orders
+#ifndef HARDFORK_CORE_342_TIME
+#define HARDFORK_CORE_342_TIME (fc::time_point_sec( 1600000000 ))
+#endif

libraries/chain/include/graphene/chain/asset_object.hpp

@@ -145,7 +145,12 @@ namespace graphene { namespace chain {
 
          template<class DB>
          const asset_bitasset_data_object& bitasset_data(const DB& db)const
-         { assert(bitasset_data_id); return db.get(*bitasset_data_id); }
+         {
+            FC_ASSERT( bitasset_data_id.valid(),
+                       "Asset ${a} (${id}) is not a market issued asset.",
+                       ("a",this->symbol)("id",this->id) );
+            return db.get( *bitasset_data_id );
+         }
 
          template<class DB>
          const asset_dynamic_data_object& dynamic_data(const DB& db)const

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

@@ -29,6 +29,8 @@ namespace graphene { namespace chain {
 
    extern const int64_t scaled_precision_lut[];
 
+   struct price;
+
    struct asset
    {
       asset( share_type a = 0, asset_id_type id = asset_id_type() )
@@ -94,6 +96,8 @@ namespace graphene { namespace chain {
          FC_ASSERT( precision < 19 );
          return scaled_precision_lut[ precision ];
       }
+
+      asset multiply_and_round_up( const price& p )const; ///< Multiply and round up
    };
 
    /**
@@ -144,7 +148,7 @@ namespace graphene { namespace chain {
    bool  operator >= ( const price& a, const price& b );
    bool  operator == ( const price& a, const price& b );
    bool  operator != ( const price& a, const price& b );
-   asset operator *  ( const asset& a, const price& b );
+   asset operator *  ( const asset& a, const price& b ); ///< Multiply and round down
 
    price operator *  ( const price& p, const ratio_type& r );
    price operator /  ( const price& p, const ratio_type& r );

libraries/chain/protocol/asset.cpp

@@ -92,6 +92,26 @@ namespace graphene { namespace chain {
          FC_THROW_EXCEPTION( fc::assert_exception, "invalid asset * price", ("asset",a)("price",b) );
       }
 
+      asset asset::multiply_and_round_up( const price& b )const
+      {
+         const asset& a = *this;
+         if( a.asset_id == b.base.asset_id )
+         {
+            FC_ASSERT( b.base.amount.value > 0 );
+            uint128_t result = (uint128_t(a.amount.value) * b.quote.amount.value + b.base.amount.value - 1)/b.base.amount.value;
+            FC_ASSERT( result <= GRAPHENE_MAX_SHARE_SUPPLY );
+            return asset( result.convert_to<int64_t>(), b.quote.asset_id );
+         }
+         else if( a.asset_id == b.quote.asset_id )
+         {
+            FC_ASSERT( b.quote.amount.value > 0 );
+            uint128_t result = (uint128_t(a.amount.value) * b.base.amount.value + b.quote.amount.value - 1)/b.quote.amount.value;
+            FC_ASSERT( result <= GRAPHENE_MAX_SHARE_SUPPLY );
+            return asset( result.convert_to<int64_t>(), b.base.asset_id );
+         }
+         FC_THROW_EXCEPTION( fc::assert_exception, "invalid asset::multiply_and_round_up(price)", ("asset",a)("price",b) );
+      }
+
       price operator / ( const asset& base, const asset& quote )
       { try {
          FC_ASSERT( base.asset_id != quote.asset_id );

tests/common/database_fixture.hpp

@@ -127,7 +127,8 @@ extern uint32_t GRAPHENE_TESTING_GENESIS_TIMESTAMP;
 
 #define PREP_ACTOR(name) \
    fc::ecc::private_key name ## _private_key = generate_private_key(BOOST_PP_STRINGIZE(name));   \
-   public_key_type name ## _public_key = name ## _private_key.get_public_key();
+   public_key_type name ## _public_key = name ## _private_key.get_public_key(); \
+   BOOST_CHECK( name ## _public_key != public_key_type() );
 
 #define ACTOR(name) \
    PREP_ACTOR(name) \

tests/tests/basic_tests.cpp

@@ -242,6 +242,51 @@ BOOST_AUTO_TEST_CASE( price_test )
     BOOST_CHECK( less_than_max2 * ratio_type(1,1) == less_than_max2 );
     BOOST_CHECK( less_than_max2 * ratio_type(5,2) == price(asset(less_than_max2.base.amount*5/2/7),asset(1,asset_id_type(1))) );
 
+    BOOST_CHECK( ( asset(1) * price( asset(1), asset(1, asset_id_type(1)) ) ) == asset(1, asset_id_type(1)) );
+    BOOST_CHECK( ( asset(1) * price( asset(1, asset_id_type(1)), asset(1) ) ) == asset(1, asset_id_type(1)) );
+    BOOST_CHECK( ( asset(1, asset_id_type(1)) * price( asset(1), asset(1, asset_id_type(1)) ) ) == asset(1) );
+    BOOST_CHECK( ( asset(1, asset_id_type(1)) * price( asset(1, asset_id_type(1)), asset(1) ) ) == asset(1) );
+
+    BOOST_CHECK( ( asset(3) * price( asset(3), asset(5, asset_id_type(1)) ) ) == asset(5, asset_id_type(1)) ); // round_down(3*5/3)
+    BOOST_CHECK( ( asset(5) * price( asset(2, asset_id_type(1)), asset(7) ) ) == asset(1, asset_id_type(1)) ); // round_down(5*2/7)
+    BOOST_CHECK( ( asset(7, asset_id_type(1)) * price( asset(2), asset(3, asset_id_type(1)) ) ) == asset(4) ); // round_down(7*2/3)
+    BOOST_CHECK( ( asset(9, asset_id_type(1)) * price( asset(8, asset_id_type(1)), asset(7) ) ) == asset(7) ); // round_down(9*7/8)
+
+    // asset and price doesn't match
+    BOOST_CHECK_THROW( asset(1) * price( asset(1, asset_id_type(2)), asset(1, asset_id_type(1)) ), fc::assert_exception );
+    // divide by zero
+    BOOST_CHECK_THROW( asset(1) * price( asset(0), asset(1, asset_id_type(1)) ), fc::assert_exception );
+    BOOST_CHECK_THROW( asset(1) * price( asset(1, asset_id_type(1)), asset(0) ), fc::assert_exception );
+    // overflow
+    BOOST_CHECK_THROW( asset(GRAPHENE_MAX_SHARE_SUPPLY/2+1) * price( asset(1), asset(2, asset_id_type(1)) ), fc::assert_exception );
+    BOOST_CHECK_THROW( asset(2) * price( asset(GRAPHENE_MAX_SHARE_SUPPLY/2+1, asset_id_type(1)), asset(1) ), fc::assert_exception );
+
+    BOOST_CHECK( asset(1).multiply_and_round_up( price( asset(1), asset(1, asset_id_type(1)) ) ) == asset(1, asset_id_type(1)) );
+    BOOST_CHECK( asset(1).multiply_and_round_up( price( asset(1, asset_id_type(1)), asset(1) ) ) == asset(1, asset_id_type(1)) );
+    BOOST_CHECK( asset(1, asset_id_type(1)).multiply_and_round_up( price( asset(1), asset(1, asset_id_type(1)) ) ) == asset(1) );
+    BOOST_CHECK( asset(1, asset_id_type(1)).multiply_and_round_up( price( asset(1, asset_id_type(1)), asset(1) ) ) == asset(1) );
+
+    // round_up(3*5/3)
+    BOOST_CHECK( asset(3).multiply_and_round_up( price( asset(3), asset(5, asset_id_type(1)) ) ) == asset(5, asset_id_type(1)) );
+    // round_up(5*2/7)
+    BOOST_CHECK( asset(5).multiply_and_round_up( price( asset(2, asset_id_type(1)), asset(7) ) ) == asset(2, asset_id_type(1)) );
+    // round_up(7*2/3)
+    BOOST_CHECK( asset(7, asset_id_type(1)).multiply_and_round_up( price( asset(2), asset(3, asset_id_type(1)) ) ) == asset(5) );
+    // round_up(9*7/8)
+    BOOST_CHECK( asset(9, asset_id_type(1)).multiply_and_round_up( price( asset(8, asset_id_type(1)), asset(7) ) ) == asset(8) );
+
+    // asset and price doesn't match
+    BOOST_CHECK_THROW( asset(1, asset_id_type(3)).multiply_and_round_up( price( asset(1, asset_id_type(2)), asset(1) ) ),
+                       fc::assert_exception );
+    // divide by zero
+    BOOST_CHECK_THROW( asset(1).multiply_and_round_up( price( asset(0), asset(1, asset_id_type(1)) ) ), fc::assert_exception );
+    BOOST_CHECK_THROW( asset(1).multiply_and_round_up( price( asset(1, asset_id_type(1)), asset(0) ) ), fc::assert_exception );
+    // overflow
+    BOOST_CHECK_THROW( asset(GRAPHENE_MAX_SHARE_SUPPLY/2+1).multiply_and_round_up( price( asset(1), asset(2, asset_id_type(1)) ) ),
+                       fc::assert_exception );
+    BOOST_CHECK_THROW( asset(2).multiply_and_round_up( price( asset(GRAPHENE_MAX_SHARE_SUPPLY/2+1, asset_id_type(1)), asset(1) ) ),
+                       fc::assert_exception );
+
     price_feed dummy;
     dummy.maintenance_collateral_ratio = 1002;
     dummy.maximum_short_squeeze_ratio = 1234;