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.

Sign up for GitHub

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

Verify active witness before applying block - Issue #831 #884

Closed
wants to merge 2 commits into from
Closed

Verify active witness before applying block - Issue #831 #884

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04fe034
Verify active witness before applying block
jmjatlanta Apr 30, 2018
8b0a505
Throwing exception on block witness not being an active witness
jmjatlanta May 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions libraries/chain/db_block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,19 @@ bool database::_push_block(const signed_block& new_block)
uint32_t skip = get_node_properties().skip_flags;
if( !(skip&skip_fork_db) )
{
/// TODO: if the block is greater than the head block and before the next maitenance interval
// verify that the block signer is in the current set of active witnesses.

// If the block is greater than the head block and before the next maintenance interval
if (new_block.block_num() > head_block_num()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check every block after last irreversible block, but not only those after head block.

However, for blocks those are before last maintenance interval, we aren't able to verify their witness, since we don't store the active witness list of that time in current state (not hard to change related code to store that info anyway).

That said, active witness list in current state can be different than in the fork that contains the new block, if the fork happened before last maintenance interval. Which means it's incorrect less efficient (for switching to correct fork) if we drop the new block in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see if I am following your thought process correctly. I see 2 points here:

  1. We should change the if to look for blocks that came after the last maintenance interval instead of blocks > head_block_num()
  2. We should not be comparing this block's witness with the list of active witnesses, but instead compare this block's witnesses with the list of active witnesses on the fork where the new block will be placed. But we may not have enough information to do so, as we do not have the active witnesses on the fork, if the fork happened before the last maintenance interval.

Did I catch the meaning? Or am I off in the weeds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 should be last irreversible block, not last maintenance interval.

Need more discussion though. Perhaps @pmconrad will have some ideas.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tricky.

Let I be the last irreversible block, F the block after which the fork occurs (i. e. the last block common to both forks), M and M' the last maintenance blocks on the two forks, and W and W' the sets of active witnesses after M and M' respectively.

We know I <= F. If M = M' <= F then W=W' and we don't have a problem. So let's examine the case M != M' and W != W'.

Suppose we are on the M side of the fork, on block A=M+1. We have M' in the fork database. We receive A'=M'+1, signed by witness w'. w' is in W' but not in W. Then we receive B'=A'+1. That means (F,...,M',A',B') is a longer fork than (F,...,M,A), so we must switch over. We can only recognize the longer fork if we accept A' and B' into the fork database, despite the fact that we don't (as of today) know W'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that in extreme situations we can have long-lived forks that span several maintenance intervals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this discussion can lead to questioning about the correctness of "last irreversible block" mechanism. When forks are long enough, in extreme situations, each fork will have its own LIB.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EOSIO/eos#2718 is related.

&& new_block.timestamp < get_dynamic_global_properties().next_maintenance_time )
{
// make sure the block signer is in the current set of active witnesses
const auto& witnesses = get_global_properties().active_witnesses;
witnesses.find(new_block.witness);
if ( witnesses.find(new_block.witness) == witnesses.end() )
{
FC_THROW_EXCEPTION(fc::assert_exception, "database::_push_block: Block witness not in list of active witnesses.");
}
}

shared_ptr<fork_item> new_head = _fork_db.push_block(new_block);
//If the head block from the longest chain does not build off of the current head, we need to switch forks.
Expand Down