Start block state creation early #6167

heifner · 2018-10-26T20:06:56Z

Change Description

controller::push_block() now takes a future to a block state that can be created by controller::create_state_block_future(). This allows the creation of the block state to begin earlier (before abort block) including the verfication of block signee.
Added a controller::replay_push_block for use during replay since futures would only slow that process down (block signee is not verified during normal replay).
Capture weak_ptr for futures so they do not keep shared_ptr alive

brianjohnson5972 · 2018-10-29T02:05:30Z

libraries/chain/controller.cpp

      try {
+         auto& b = bs->block;


Could this be made "const auto&"? (quick perusal looks like it could, but hard to search with variable "b")

brianjohnson5972 · 2018-10-29T02:52:58Z

libraries/chain/block_header_state.cpp

    if( !trust ) {
-       EOS_ASSERT( result.block_signing_key == result.signee(), wrong_signing_key, "block not signed by expected key",
-                  ("result.block_signing_key", result.block_signing_key)("signee", result.signee() ) );
+       result.verify_signee();


If my brain is following this, I think it would be better that this block assert that !block_signing_key_future.valid() and call verify_signee(signee()) (see comment on verify_signee() )

brianjohnson5972 · 2018-10-29T02:53:14Z

libraries/chain/block_header_state.cpp

+     if( block_signing_key_future.valid() ) {
+        // if block signing verification delayed for light validation then verify it here
+        auto signee = block_signing_key_future.get();
+        EOS_ASSERT( block_signing_key == signee, wrong_signing_key, "block signed by wrong key",


Can we make this check a function verify_signee(... signee) { ...}?

And make verify_signee do an assert on block_signing_key_future.valid() and make this method just do the future signature processing, see other comment above.

We should handle case where signee == decltype(block_signing_key_future){} differently, since we don't know if the block is signed by wrong key, we just don't have the header state anymore to know who the signee is.

brianjohnson5972 · 2018-10-29T15:16:09Z

libraries/chain/controller.cpp

@@ -1263,7 +1274,15 @@ struct controller_impl {
         EOS_ASSERT( s != controller::block_status::incomplete, block_validate_exception, "invalid block status for a completed block" );
         emit( self.pre_accepted_block, b );
         bool trust = !conf.force_all_checks && (s == controller::block_status::irreversible || s == controller::block_status::validated);
-         auto new_header_state = fork_db.add( b, trust );
+         auto new_header_state = fork_db.add( b, trust || conf.block_validation_mode == validation_mode::LIGHT );


I don't like that a flag that we are calling trust in fork_database::add and block_state::next is really "trust or light validation", we either need to come up with a name that means that or give it a horrible name like trust_or_light_validation so that someone is expected to look at what is set into that flag before they can correctly use it.

heifner · 2018-10-30T16:29:45Z

Tieing block signee delayed verification to light validation is the opposite of what this should be tied to. Light validation implies you are doing the same validation as a light client. Block signature verification is not something you want to postpone in that mode.

brianjohnson5972

Changes were requested, see @heifner 's comment

create_block_state_futre. This allows the creation of the new block_state and verification of block signee while current block is aborted. - replay_push_block added which provides efficient version of push_block for purpose of replay only.

… block_state_ptr)

arhag · 2018-10-31T22:06:59Z

libraries/chain/controller.cpp

-         bool trust = !conf.force_all_checks && (s == controller::block_status::irreversible || s == controller::block_status::validated);
-         auto new_header_state = fork_db.add( b, trust );
+
+         fork_db.add( new_header_state );


Calling fork_database::add( block_state_ptr ) here instead of the original fork_database::add( signed_block_ptr bool ) skips some checks that were done in the original version.

create_block_state_future replicates those fork_database::add( signed_block_ptr, bool) checks, but it does so at a different time (and therefore potentially with different state of the fork database) than the call to fork_db.add here.

One of the two necessary checks, checking for duplicate blocks, is already handled in fork_database::add( block_state_ptr ) with the assertion that inserted.second is true.

The other check that is necessary to again here (so it could be done in fork_database::add( block_state_ptr )) is to ensure that the block still links to an existing block in the fork database. Without that check, it is possible to add a block into the fork database that does not link. For example, a user of controller could in theory:

Call create_block_state_future on two different blocks A and B where both currently link to existing blocks in the fork database and where B builds off of the current head.

Then calls push_block with the future of block A which causes: a fork switch, LIB advancement, and purging of orphaned branches which may include the block that the block B builds off of.

Then calls push_block with the future of block B.

The current code does not have more than one create_block_state_future in play at a time. It is single threaded from producer plugin on_incoming_block to push_block. The create_block_state_future creation of block_state which includes digest, merkle, and signee verification can run at the same time as abort_block but the rest of the order is all single threaded.

Now if we updated the code so that more than one create_block_state_future could be in flight at a time then that could certainly be a problem. That would take some work as it does verify previous is in the fork_db. Could create a version of create_block_state_future that takes the previous block so that a whole chain of blocks could be processed at once and then verify in fork_db add that the previous is there. However, that would require changing net_plugin to receive more than one block at a time or queue up blocks which would increase latency which is what this is attempting to reduce.

That said the extra check doesn't hurt and should be quick, so I'll go ahead and add it.

heifner requested a review from wanderingbort October 29, 2018 12:44

brianjohnson5972 reviewed Oct 29, 2018

View reviewed changes

brianjohnson5972 approved these changes Oct 29, 2018

View reviewed changes

brianjohnson5972 suggested changes Oct 30, 2018

View reviewed changes

heifner changed the title ~~Light block verification~~ Start block state creation early Oct 31, 2018

heifner added 16 commits October 31, 2018 13:49

Use weak_ptr so if future get never called does not prevent destruction

7c8061d

Add verify_signee() method that uses future if set

83af362

Delay block signee verififcation for light validation

4d34a70

Rename trust to more descriptive skip_validate_signee

1deb426

Split verify_signee into two methods to make is clearer purpose

48a72c5

Cleanup from peer review comments

d430371

Add block id to EOS_ASSERT message

3649fab

Remove unused block_signing_key_future

42611d3

Remove used verify_signee_future()

d08bee3

Add EOS_ASSERTs to add( block_state_ptr ) since now called externally

1c575b4

Update to use new push_block that takes a future

3be1511

Update tests to use new push_block that takes a future

67a9df7

Remove no longer valid comment

91c0baa

Undo code changes to minimize diff

edf82ea

Pass signed_block_ptr to apply_block (revert previous uneeded pass of…

8793bf3

… block_state_ptr)

heifner force-pushed the light-block-verification branch from 3859179 to 8793bf3 Compare October 31, 2018 20:43

arhag suggested changes Oct 31, 2018

View reviewed changes

Add optional verify of previous exist to fork_db add( block_state_ptr )

7cea107

arhag approved these changes Nov 1, 2018

View reviewed changes

brianjohnson5972 approved these changes Nov 6, 2018

View reviewed changes

wanderingbort merged commit b828413 into develop Nov 13, 2018

wanderingbort deleted the light-block-verification branch November 13, 2018 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start block state creation early #6167

Start block state creation early #6167

heifner commented Oct 26, 2018 •

edited

brianjohnson5972 Oct 29, 2018

brianjohnson5972 Oct 29, 2018

brianjohnson5972 Oct 29, 2018

brianjohnson5972 Oct 29, 2018

brianjohnson5972 Oct 29, 2018

heifner commented Oct 30, 2018

brianjohnson5972 left a comment

arhag Oct 31, 2018

heifner Nov 1, 2018

Start block state creation early #6167

Start block state creation early #6167

Conversation

heifner commented Oct 26, 2018 • edited

brianjohnson5972 Oct 29, 2018

Choose a reason for hiding this comment

brianjohnson5972 Oct 29, 2018

Choose a reason for hiding this comment

brianjohnson5972 Oct 29, 2018

Choose a reason for hiding this comment

brianjohnson5972 Oct 29, 2018

Choose a reason for hiding this comment

brianjohnson5972 Oct 29, 2018

Choose a reason for hiding this comment

heifner commented Oct 30, 2018

brianjohnson5972 left a comment

Choose a reason for hiding this comment

arhag Oct 31, 2018

Choose a reason for hiding this comment

heifner Nov 1, 2018

Choose a reason for hiding this comment

heifner commented Oct 26, 2018 •

edited