Store correct block ID when switching forks #938

abitmore · 2018-05-21T12:22:19Z

Related to #831 .

jmjatlanta

Good find. Are there tests that exercise forking, switching forks, etc? Can a test be developed to demonstrate this issue?

Update: Found it... block_tests::fork_blocks

pmconrad · 2018-05-22T16:31:30Z

Looks good. @jmjatlanta are you working on a test case?

oxarbitrage · 2018-05-22T18:02:16Z

added some dumps at @abitmore code to see the difference and running block_tests::fork_blocks.

as can be seen, the new_block.id is always the same while (*ritr)->id goes changing.

3493398ms th_a       db_management.cpp:153         open                 ] Wiping object_database due to missing or wrong version
3493398ms th_a       object_database.cpp:93        wipe                 ] Wiping object database...
3493398ms th_a       object_database.cpp:95        wipe                 ] Done wiping object databse.
3493398ms th_a       object_database.cpp:106       open                 ] Opening object database from /tmp/graphene-tmp/6763-dedd-4834-4478 ...
3493398ms th_a       object_database.cpp:111       open                 ] Done opening object database.
3493410ms th_a       db_block.cpp:145              _push_block          ] Switching to fork: 0000000ec1a5fb3af98a58ae947eab44eb16e512
3493410ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 11 0000000b93898f176395f57845e7b356eb55b2db
3493410ms th_a       db_block.cpp:160              _push_block          ] new_block.id(): 0000000ec1a5fb3af98a58ae947eab44eb16e512 
3493410ms th_a       db_block.cpp:161              _push_block          ] (*ritr)->id: 0000000b93898f176395f57845e7b356eb55b2db 
3493410ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 12 0000000cfae33417202ce7ad1745b45c393aaca1
3493410ms th_a       db_block.cpp:160              _push_block          ] new_block.id(): 0000000ec1a5fb3af98a58ae947eab44eb16e512 
3493410ms th_a       db_block.cpp:161              _push_block          ] (*ritr)->id: 0000000cfae33417202ce7ad1745b45c393aaca1 
3493410ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 13 0000000d3dee6e52208fff4bebcf43e3a5c32da5
3493410ms th_a       db_block.cpp:160              _push_block          ] new_block.id(): 0000000ec1a5fb3af98a58ae947eab44eb16e512 
3493410ms th_a       db_block.cpp:161              _push_block          ] (*ritr)->id: 0000000d3dee6e52208fff4bebcf43e3a5c32da5 
3493410ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 14 0000000ec1a5fb3af98a58ae947eab44eb16e512
3493410ms th_a       db_block.cpp:168              _push_block          ] exception thrown while switching forks 10 assert_exception: Assert Exception
(skip & skip_merkle_check) || next_block.transaction_merkle_root == next_block.calculate_merkle_root(): 
    {"next_block.transaction_merkle_root":"0000000000000000000000000000000000000000","calc":"e6168d22cd56ae5854c8761377666cbf3bbbd554","next_block":{"previous":"0000000d3dee6e52208fff4bebcf43e3a5c32da5","timestamp":"2015-05-15T14:28:15","witness":"1.6.3","transaction_merkle_root":"0000000000000000000000000000000000000000","extensions":[],"witness_signature":"1f02c7e22da709666e48ff63924bc1c5116368f036704911c0b4f36f9a918e13806d2a907e45e04e0f188f6e175e80c764dfb17a5a10e3db9620411953efdc966e","transactions":[{"ref_block_num":0,"ref_block_prefix":0,"expiration":"1970-01-01T00:00:00","operations":[[0,{"fee":{"amount":0,"asset_id":"1.3.0"},"from":"1.2.0","to":"1.2.0","amount":{"amount":0,"asset_id":"1.3.0"},"extensions":[]}]],"extensions":[],"signatures":[],"operation_results":[]}]},"id":"0000000ec1a5fb3af98a58ae947eab44eb16e512"}
    th_a  db_block.cpp:495 _apply_block

    {"next_block.block_num()":14}
    th_a  db_block.cpp:547 _apply_block
3493411ms th_a       db_block.cpp:145              _push_block          ] Switching to fork: 0000000ec1a5fb3af98a58ae947eab44eb16e512
3493411ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 11 0000000b93898f176395f57845e7b356eb55b2db
3493411ms th_a       db_block.cpp:160              _push_block          ] new_block.id(): 0000000ec1a5fb3af98a58ae947eab44eb16e512 
3493411ms th_a       db_block.cpp:161              _push_block          ] (*ritr)->id: 0000000b93898f176395f57845e7b356eb55b2db 
3493411ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 12 0000000cfae33417202ce7ad1745b45c393aaca1
3493411ms th_a       db_block.cpp:160              _push_block          ] new_block.id(): 0000000ec1a5fb3af98a58ae947eab44eb16e512 
3493411ms th_a       db_block.cpp:161              _push_block          ] (*ritr)->id: 0000000cfae33417202ce7ad1745b45c393aaca1 
3493411ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 13 0000000d3dee6e52208fff4bebcf43e3a5c32da5
3493412ms th_a       db_block.cpp:160              _push_block          ] new_block.id(): 0000000ec1a5fb3af98a58ae947eab44eb16e512 
3493412ms th_a       db_block.cpp:161              _push_block          ] (*ritr)->id: 0000000d3dee6e52208fff4bebcf43e3a5c32da5 
3493412ms th_a       db_block.cpp:155              _push_block          ] pushing blocks from fork 14 0000000ec1a5fb3af98a58ae947eab44eb16e512
3493412ms th_a       db_block.cpp:160              _push_block          ] new_block.id(): 0000000ec1a5fb3af98a58ae947eab44eb16e512 
3493412ms th_a       db_block.cpp:161              _push_block          ] (*ritr)->id: 0000000ec1a5fb3af98a58ae947eab44eb16e512 

*** No errors detected
root@oxarbitrage ~/bitshares/dev/pull938/bitshares-core # ./tests/chain_test -t block_tests/fork_blocks

i think this is the expected behavior, please check and confirm.

jmjatlanta · 2018-05-22T22:01:08Z

I was not working on a test case, but I will.

jmjatlanta · 2018-05-23T15:00:52Z

tests/tests/block_tests.cpp

+         {
+            fc::optional<signed_block> curr_block = db1.fetch_block_by_number( curr_block_num );
+            BOOST_CHECK( curr_block.valid() );
+            BOOST_CHECK_EQUAL( curr_block->previous, previous_block->id() );


This test should fail with the old code, but is not.

fetch_block_by_number here will read data from _fork_db, thus will pass the check.

I think we can add more tests in the end: close db, reopen db, run this check again.

Unfortunately, the original problem still does not show up. I haven't dug too deeply, but I'm guessing the reindexing that happens when the database opens fixes things.

Oh, reopen at this point doesn't work. Try generate 30 more blocks to push the 2 blocks out of fork_db.

Oh, seems due to empty blocks. I'm trying to update the test case.

My comment earlier about empty block is incorrect.

Actually PUSH_BLOCK( db1, good_block ); made all blocks stored in db1 correct. I'm trying to reproduce.

abitmore · 2018-05-24T11:50:54Z

@pmconrad @jmjatlanta @oxarbitrage test case fixed, please review again.

pmconrad

Good job!

Store correct block ID when switching forks

2d6b831

abitmore added the security label May 21, 2018

abitmore added this to the 201806 - Non-Consensus-Changing Release milestone May 21, 2018

abitmore requested review from jmjatlanta and pmconrad May 21, 2018 12:22

jmjatlanta reviewed May 21, 2018

View reviewed changes

oxarbitrage self-requested a review May 22, 2018 16:16

abitmore assigned jmjatlanta May 23, 2018

Testing for invalid block ids, test should fail

ec48416

jmjatlanta reviewed May 23, 2018

View reviewed changes

abitmore added 2 commits May 24, 2018 07:10

Testing for invalid block ids

8c779a7

Update logging and variables used

72bee83

abitmore requested a review from jmjatlanta May 24, 2018 11:49

pmconrad approved these changes May 24, 2018

View reviewed changes

abitmore merged commit 6b69b6a into develop May 24, 2018

abitmore deleted the fix-block-storing branch May 25, 2018 07:47

pmconrad mentioned this pull request Jan 21, 2019

Fix block storing soundac/SounDAC-Source#41

Merged

pmconrad mentioned this pull request Feb 1, 2019

Merge improvements from BitShares soundac/SounDAC-Source#26

Open

24 tasks

This was referenced Sep 5, 2019

[GRPH-73] fix block id forks peerplays-network/peerplays#110

Merged

fork resolution fixes peerplays-network/peerplays#118

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store correct block ID when switching forks #938

Store correct block ID when switching forks #938

abitmore commented May 21, 2018

jmjatlanta left a comment •

edited

Loading

pmconrad commented May 22, 2018

oxarbitrage commented May 22, 2018

jmjatlanta commented May 22, 2018

jmjatlanta May 23, 2018

abitmore May 23, 2018

jmjatlanta May 23, 2018 •

edited

Loading

abitmore May 24, 2018

abitmore May 24, 2018

abitmore May 24, 2018 •

edited

Loading

abitmore commented May 24, 2018

pmconrad left a comment

Store correct block ID when switching forks #938

Store correct block ID when switching forks #938

Conversation

abitmore commented May 21, 2018

jmjatlanta left a comment • edited Loading

Choose a reason for hiding this comment

pmconrad commented May 22, 2018

oxarbitrage commented May 22, 2018

jmjatlanta commented May 22, 2018

jmjatlanta May 23, 2018

Choose a reason for hiding this comment

abitmore May 23, 2018

Choose a reason for hiding this comment

jmjatlanta May 23, 2018 • edited Loading

Choose a reason for hiding this comment

abitmore May 24, 2018

Choose a reason for hiding this comment

abitmore May 24, 2018

Choose a reason for hiding this comment

abitmore May 24, 2018 • edited Loading

Choose a reason for hiding this comment

abitmore commented May 24, 2018

pmconrad left a comment

Choose a reason for hiding this comment

jmjatlanta left a comment •

edited

Loading

jmjatlanta May 23, 2018 •

edited

Loading

abitmore May 24, 2018 •

edited

Loading