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

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

Closed
wants to merge 2 commits into from

Conversation

jmjatlanta
Copy link
Contributor

@jmjatlanta jmjatlanta commented Apr 30, 2018

This is a solution for Issue #831, back ported from EOSIO/eos@fc61a26

  • Verify signature is valid before trusting
  • Develop tests

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

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

This change only partially fixes the issue if done correctly.

Need more discussion.

libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
// 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.

libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
@abitmore abitmore added 2a Discussion Needed Prompt for team to discuss at next stand up. security labels May 1, 2018
@abitmore abitmore added this to the Future Non-Consensus-Changing Release milestone May 2, 2018
libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
libraries/chain/db_block.cpp Outdated Show resolved Hide resolved
// 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.

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

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

@pmconrad
Copy link
Contributor

@jmjatlanta status?

@jmjatlanta
Copy link
Contributor Author

This one is stuck due to forking/LIB considerations. See this comment above and the context surrounding it.

@abitmore
Copy link
Member

Closing in favor of #1987.

@abitmore abitmore closed this Sep 13, 2019
@abitmore abitmore removed this from the Future Feature Release milestone Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2a Discussion Needed Prompt for team to discuss at next stand up. security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants