Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for existing tests after hf1270 #1493

Merged

Conversation

Projects
2 participants
@oxarbitrage
Copy link
Member

commented Dec 24, 2018

By comments in #1324 (comment) the flag for existing tests was added to database fixture.

This pull is not to merge(at least not as it is) but it is for discussion of some issues found in the existing tests after the hardfork.

Trying to give support for the following existing test cases:

  • hardfork_core_338_test
  • hardfork_core_453_test
  • hardfork_core_625_big_limit_order_test
  • target_cr_test_limit_call
  • target_cr_test_call_limit

The first 3 tests pass after hardfork, only mod needed is on hardfork_core_338_test that use call price to do some checks. This is not allowed after HF1270, order matching is as expected.

In the last 2 there are some different at matching orders, specifically i modified a bit target_cr_test_limit_call to show this differences:

This is how before HF match the orders before and after the big sell order:

NAME               FOR SALE         FOR WHAT PRICE (S/W) 1/PRICE (W/S)
======================================================================
seller              7 USDBIT         78 BTS      0.08974   11.14286 
buyer3            111 BTS           10 USDBIT   11.10000    0.09009 
buyer2          33000 BTS         3000 USDBIT   11.00000    0.09091 
buyer              80 BTS           10 USDBIT    8.00000    0.12500 
NAME               FOR SALE         FOR WHAT PRICE (S/W) 1/PRICE (W/S)
======================================================================
seller              7 USDBIT         78 BTS      0.08974   11.14286 
buyer2          12056 BTS         1096 USDBIT   11.00000    0.09091 
buyer              80 BTS           10 USDBIT    8.00000    0.12500 

And this is what after HF will do:

NAME               FOR SALE         FOR WHAT PRICE (S/W) 1/PRICE (W/S)
======================================================================
seller              7 USDBIT         78 BTS      0.08974   11.14286 
buyer3            111 BTS           10 USDBIT   11.10000    0.09009 
buyer2          33000 BTS         3000 USDBIT   11.00000    0.09091 
buyer              80 BTS           10 USDBIT    8.00000    0.12500 
NAME               FOR SALE         FOR WHAT PRICE (S/W) 1/PRICE (W/S)
======================================================================
seller              7 USDBIT         78 BTS      0.08974   11.14286 
buyer2          24310 BTS         2210 USDBIT   11.00000    0.09091 
buyer              80 BTS           10 USDBIT    8.00000    0.12500 

@abitmore i will like to have your word about this when you can.

@abitmore

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

@oxarbitrage could you print data of the call orders? There should be a difference.

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2018

before HF call orders:

NAME             TYPE             DEBT           COLLAT  CALL PRICE(D/C) ~CALL PRICE(C/D)        SWAN(D/C)        SWAN(C/D)
======================================================================
borrower        1000 USDBIT        15000 BTS          8.57143          0.11667          0.06667         15.00000 
borrower2       1000 USDBIT        15500 BTS          8.85714          0.11290          0.06452         15.50000 
borrower3       1000 USDBIT        25000 BTS         14.28571          0.07000          0.04000         25.00000 
NAME             TYPE             DEBT           COLLAT  CALL PRICE(D/C) ~CALL PRICE(C/D)        SWAN(D/C)        SWAN(C/D)
======================================================================
borrower         615 USDBIT        10765 BTS         10.00232          0.09998          0.05713         17.50407 
borrower2        499 USDBIT         9989 BTS         11.43888          0.08742          0.04995         20.01804 
borrower3       1000 USDBIT        25000 BTS         14.28571          0.07000          0.04000         25.00000 

After:

NAME             TYPE             DEBT           COLLAT  CALL PRICE(D/C) ~CALL PRICE(C/D)        SWAN(D/C)        SWAN(C/D)
======================================================================
borrower        1000 USDBIT        15000 BTS          1.00000          1.00000          0.06667         15.00000 
borrower2       1000 USDBIT        15500 BTS          1.00000          1.00000          0.06452         15.50000 
borrower3       1000 USDBIT        25000 BTS          1.00000          1.00000          0.04000         25.00000 
NAME             TYPE             DEBT           COLLAT  CALL PRICE(D/C) ~CALL PRICE(C/D)        SWAN(D/C)        SWAN(C/D)
======================================================================
borrower3       1000 USDBIT        25000 BTS          1.00000          1.00000          0.04000         25.00000 
@abitmore

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

The issue is fixed in 1270-fix-call-price-cache branch. Thanks.

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Dec 26, 2018

thank you @abitmore . Everything passing now, i removed the prints at 3aec53d . Can be merged as it is.

Do you think we should do more HF1270 checks inside rounding_tests, bitassets_tests, swan tests?

@abitmore

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

Yes, we need swan_tests (including revivals). I guess we need bitasset_tests as well, but perhaps don't need to update rounding tests. Thanks.

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2018

I added after hf1270 tests to swan_tests.

There is an issue with order matching in 1 of them, at black_swan_issue_346

Before HF:

./tests/chain_test -t swan_tests/black_swan_issue_346
...
1291336ms th_a       db_update.cpp:249             check_for_blackswan  ] *call_ptr: {"id":"1.8.0","borrower":"1.2.18","collateral":1000,"debt":100,"call_price":{"base":{"amount":40,"asset_id":"1.3.0"},"quote":{"amount":7,"asset_id":"1.3.2"}}} 
1291336ms th_a       db_update.cpp:259             check_for_blackswan  ] Black Swan detected on asset USDBIT0X (1.3.2) at block 1: 
   Least collateralized call: 10.00000000000000000  0.10000000000000001
   Settle Price:              50.00000000000000000  0.02000000000000000
   Max:                       50.00000000000000000  0.02000000000000000

1291336ms th_a       db_update.cpp:260             check_for_blackswan  ] enable_black_swan: true 
1291337ms th_a       swan_tests.cpp:230            operator()           ] feed.max_short_squeeze_price(): {"base":{"amount":2,"asset_id":"1.3.3"},"quote":{"amount":75,"asset_id":"1.3.0"}} 
1291338ms th_a       swan_tests.cpp:230            operator()           ] feed.max_short_squeeze_price(): {"base":{"amount":7,"asset_id":"1.3.3"},"quote":{"amount":500,"asset_id":"1.3.0"}} 
1291338ms th_a       db_market.cpp:1069            check_call_orders    ] black swan detected on asset USDBIT1X (1.3.3) at block 1
1291338ms th_a       db_market.cpp:1070            check_call_orders    ] enable_black_swan: true 
NAME               FOR SALE         FOR WHAT PRICE (S/W) 1/PRICE (W/S)
======================================================================
seller             39 USDBIT1X       2000 BTS      0.01950   51.28205 

After HF:

./tests/chain_test -t swan_tests/black_swan_issue_346_hf1270
...
1339306ms th_a       swan_tests.cpp:230            operator()           ] feed.max_short_squeeze_price(): {"base":{"amount":2,"asset_id":"1.3.2"},"quote":{"amount":15,"asset_id":"1.3.0"}} 
1339306ms th_a       swan_tests.cpp:230            operator()           ] feed.max_short_squeeze_price(): {"base":{"amount":1,"asset_id":"1.3.2"},"quote":{"amount":75,"asset_id":"1.3.0"}} 
1339306ms th_a       db_update.cpp:249             check_for_blackswan  ] *call_ptr: {"id":"1.8.0","borrower":"1.2.18","collateral":1000,"debt":100,"call_price":{"base":{"amount":1,"asset_id":"1.3.0"},"quote":{"amount":1,"asset_id":"1.3.2"}}} 
1339307ms th_a       db_update.cpp:259             check_for_blackswan  ] Black Swan detected on asset USDBIT0X (1.3.2) at block 4: 
   Least collateralized call: 10.00000000000000000  0.10000000000000001
   Settle Price:              50.00000000000000000  0.02000000000000000
   Max:                       75.00000000000000000  0.01333333333333333

1339307ms th_a       db_update.cpp:260             check_for_blackswan  ] enable_black_swan: true 
1339307ms th_a       swan_tests.cpp:230            operator()           ] feed.max_short_squeeze_price(): {"base":{"amount":2,"asset_id":"1.3.3"},"quote":{"amount":75,"asset_id":"1.3.0"}} 
1339308ms th_a       swan_tests.cpp:230            operator()           ] feed.max_short_squeeze_price(): {"base":{"amount":7,"asset_id":"1.3.3"},"quote":{"amount":500,"asset_id":"1.3.0"}} 
1339308ms th_a       db_update.cpp:249             check_for_blackswan  ] *call_ptr: {"id":"1.8.1","borrower":"1.2.18","collateral":5000,"debt":100,"call_price":{"base":{"amount":1,"asset_id":"1.3.0"},"quote":{"amount":1,"asset_id":"1.3.3"}}} 
1339308ms th_a       db_update.cpp:259             check_for_blackswan  ] Black Swan detected on asset USDBIT1X (1.3.3) at block 4: 
   Least collateralized call: 50.00000000000000000  0.02000000000000000
   Settle Price:              47.61904761904762040  0.02100000000000000
   Max:                       50.00000000000000000  0.02000000000000000

1339308ms th_a       db_update.cpp:260             check_for_blackswan  ] enable_black_swan: true 
NAME               FOR SALE         FOR WHAT PRICE (S/W) 1/PRICE (W/S)
======================================================================
seller             40 USDBIT1X       2000 BTS      0.02000   50.00000 
seller             39 USDBIT1X       2000 BTS      0.01950   51.28205 
/home/alfredo/CLionProjects/1270_existing_testcases/tests/tests/swan_tests.cpp(284): error: in "swan_tests/black_swan_issue_346_hf1270": check db.find_object( oid_020 ) == nullptr has failed

The rest of them seems to be ok. @abitmore let me know what do you think about this one when you can.

Thanks.

@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

@oxarbitrage that's an old test case which tests behaviors before any recent hard fork. I guess the behavior has changed since 2.0.180612, perhaps change the test case to that date and see what will happen? If it doesn't pass as well, I think we can skip it.

@abitmore abitmore added this to In Development in Protocol Upgrade Release (3.0.0) via automation Jan 16, 2019

Show resolved Hide resolved tests/tests/bitasset_tests.cpp Outdated
Show resolved Hide resolved tests/tests/bitasset_tests.cpp
BOOST_CHECK(!db.find<limit_order_object>(sell_id));
}
// go beyond hard fork 1270
blocks += generate_blocks(HARDFORK_CORE_1270_TIME, true, skip);

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 24, 2019

Member

To be more accurate, here should be

         blocks += generate_blocks(HARDFORK_CORE_1270_TIME - mi, true, skip);
         blocks += generate_blocks(db.get_dynamic_global_properties().next_maintenance_time, true, skip);

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Jan 24, 2019

Author Member

thank you for the suggestion, i actually tried to make it like that everywhere but there are some problems not sure why.

in the case of this particular test(hf_935_test) when i make the generate block chains i get error:

83898ms th_a       bitasset_tests.cpp:1150       test_method          ] 10 assert_exception: Assert Exception
prev: popping block would leave head block null
    {}
    th_a  fork_database.cpp:41 pop_block

    {}
    th_a  db_block.cpp:477 pop_block

This comment has been minimized.

Copy link
@abitmore

abitmore Jan 24, 2019

Member

Make sure you increased blocks here, aka blocks += generate_blocks(...).

Show resolved Hide resolved tests/tests/market_tests.cpp Outdated
Show resolved Hide resolved tests/tests/market_tests.cpp Outdated
Show resolved Hide resolved tests/tests/market_tests.cpp Outdated
Show resolved Hide resolved tests/tests/market_tests.cpp Outdated
Show resolved Hide resolved tests/tests/market_tests.cpp Outdated
Show resolved Hide resolved tests/tests/swan_tests.cpp
@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

By the way, similar code added in previous PR need to be updated as well, E.G.

BOOST_AUTO_TEST_CASE(mcr_bug_increase_before1270)
{ try {

   generate_blocks(HARDFORK_CORE_453_TIME);

   generate_block();

and also mcr_bug_increase_after1270, mcr_bug_decrease_before1270 and mcr_bug_decrease_after1270.

To be more accurate, should generate blocks until the maintenance time but not the hf time.

Actually, theoretically some test cases will fail if hf453 and hf1270 happen in the same maintenance interval (in reality we know they aren't, so it's fine).

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

Block generation for market and swan tests changed for accuracy at a2d5fac

The only one with issues when doing this change is bitasset_tests/hf_935_test. Ill try to see what is the problem there and send a fix.

@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

The bitasset_tests/hf_935_test is a bit weird to me, maybe i am not understanding it correctly but HARDFORK_CORE_868_890_TIME, HARDFORK_CORE_935_TIME, HARDFORK_CORE_343_TIME are all the same date.
Independently from changes of this pull request, with all those hardforks at the same time the block that tests beyond hard fork 890 and before hard fork 935 was never happening.

In this commit: 8466e19 i removed that test and use the spot to test hardfork 1270. I am unsure if it is the right thing to do, can revert back.

Show resolved Hide resolved tests/tests/bitasset_tests.cpp Outdated
Show resolved Hide resolved tests/tests/bitasset_tests.cpp Outdated
Show resolved Hide resolved tests/tests/bitasset_tests.cpp Outdated
@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

The logic around i is as follows (code executes in order):

  • the 1st pass, no block generation at the beginning, do something related to MCR, go beyond hf343, do something, go beyond hf 935, do something, go beyond hf1270, pop all blocks (roll back all changes)
  • the 2nd pass, mostly same as 1st, the only difference is to test MSSR but not MCR, after this all changes are rolled back
  • the 3rd pass, generate blocks to the time right after hf343, do something related to MCR, then go beyond hf935 and hf1270, pop blocks until the time right after hf343, so at this stage the chain is after hf343
  • the 4th pass, we're now after hf343, do tests related to MSSR, go beyond hf935 and hf1270, roll back to hf343
  • the 5th pass, generate blocks to hf935, test MCR, roll back to the time right after hf 935
  • the 6th pass, test MSSR, roll back to hf935
  • the 7th pass, generate blocks to hf1270, test MCR, roll back to the time right after hf1270
  • the 8th pass, test MSSR, roll back to hf1270
@oxarbitrage

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

thank you, with the logic described i should be able to make it better.

@oxarbitrage oxarbitrage referenced this pull request Jan 29, 2019

Closed

Test Cases for PR 1270 (MCR Bugfix) #1423

6 of 10 tasks complete
Show resolved Hide resolved tests/tests/bitasset_tests.cpp Outdated
Show resolved Hide resolved tests/tests/bitasset_tests.cpp Outdated
Show resolved Hide resolved tests/tests/market_tests.cpp Outdated

Protocol Upgrade Release (3.0.0) automation moved this from In Development to In Testing Jan 30, 2019

@abitmore abitmore merged commit 6b22b91 into bitshares:1270-fix-call-price-cache Jan 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Protocol Upgrade Release (3.0.0) automation moved this from In Testing to Done Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.