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

When signing a block that updates the signing witness's signing key, use correct signing key #125

Closed
vikramrajkumar opened this Issue Jan 18, 2017 · 6 comments

Comments

Projects
4 participants
@vikramrajkumar
Member

vikramrajkumar commented Jan 18, 2017

From @theoreticalbts on November 2, 2015 13:27

If block applies transactions then validates witness signature, we should sign with the new key.

If block validates witness signature then applies transactions, we should sign with the old key.

Copied from original issue: cryptonomex#430

@oxarbitrage oxarbitrage modified the milestones: Hardfork - Permission Issues, Hardfork - Operations and Authority related., Hardfork - Authorities/Sign Issues Aug 13, 2017

@abitmore abitmore modified the milestones: Hardfork - Operations and Authority related., Future Consensus-Changing Release Feb 25, 2018

@abitmore abitmore removed the hardfork label Jul 29, 2018

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jul 29, 2018

@abitmore abitmore added the bug label Jul 29, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 29, 2018

Just realized this is not a consensus changing issue, but a bug about generating / signing blocks.

Block signing key is checked here before generating the block:

const auto& witness_obj = witness_id(*this);
if( !(skip & skip_witness_signature) )
FC_ASSERT( witness_obj.signing_key == block_signing_private_key.get_public_key() );

At a glance the check is done before generating the block, however, actually the check is done with pending transactions, which means the state is dirty -- if there is a witness_update_operation pending, witness_obj.signing_key retrieved by the code would actually be the new key, consequently, the block will be signed with the new key.

Then, when pushing the block, pending transactions will be temporarily removed first, so the code will check whether the block is signed with the old key:

detail::without_pending_transactions( *this, std::move(_pending_tx),
[&]()
{
result = _push_block(new_block);
});

const witness_object& witness = next_block.witness(*this);
if( !(skip&skip_witness_signature) )
FC_ASSERT( next_block.validate_signee( witness.signing_key ) );

I'll try to fix this. After it's fixed, switching keys (at any time) will no longer cause missing of blocks.

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 29, 2018

This code checks signing key before trying to generate a block, which is done with pending transactions / dirty state as well:

graphene::chain::public_key_type scheduled_key = scheduled_witness( db ).signing_key;
auto private_key_itr = _private_keys.find( scheduled_key );
if( private_key_itr == _private_keys.end() )
{
capture("scheduled_key", scheduled_key);
return block_production_condition::no_private_key;
}

Need to fix.

@abitmore abitmore added the 9c Large label Jul 29, 2018

abitmore added a commit to abitmore/bitshares-core that referenced this issue Jul 29, 2018

abitmore added a commit that referenced this issue Jul 29, 2018

Test case to reproduce #125 block producing issue
When signing key is updated in the same block, producing will fail.
@abitmore

This comment has been minimized.

Member

abitmore commented Jul 29, 2018

See PR #1203.

@abitmore abitmore removed this from New -Awaiting Core Team Evaluation in Project Backlog Jul 29, 2018

@abitmore abitmore added this to To do in Feature release (201810) via automation Jul 29, 2018

@abitmore abitmore moved this from To do to In progress in Feature release (201810) Jul 29, 2018

@pmconrad

This comment has been minimized.

pmconrad commented Jul 31, 2018

Good catch!

abitmore added a commit to abitmore/bitshares-core that referenced this issue Aug 20, 2018

Test case to reproduce bitshares#125 block producing issue
When signing key is updated in the same block, producing will fail.

abitmore added a commit to abitmore/bitshares-core that referenced this issue Aug 20, 2018

oxarbitrage pushed a commit to oxarbitrage/bitshares-core that referenced this issue Aug 23, 2018

abitmore added a commit that referenced this issue Aug 31, 2018

Merge pull request #1203 from bitshares/125-signing-key-check
Fix issue #125: sign block with old key if signing key updated in same block
@abitmore

This comment has been minimized.

Member

abitmore commented Aug 31, 2018

Fixed by #1203.

@abitmore abitmore closed this Aug 31, 2018

Feature release (201810) automation moved this from In progress to Done Aug 31, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 1, 2018

Interestingly, Steem is moving the whole block generation code to witness plugin: steemit/steem#2861 steemit/steem#2717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment