fix(chunks): properly handle missing block, previously causing chunk signature check to fail #3026
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
birchmd
changed the title
bowenwang1996
approved these changes
Jul 23, 2020
chain/client/src/client.rs
Outdated
|
|
||
| // process_partial_encoded_chunk should return Ok(NeedBlock) if the chunk is | ||
| // based on a missing block. | ||
| match client.shards_mgr.process_partial_encoded_chunk( |
There was a problem hiding this comment.
probably better to use matches! here
|
|
@birchmd sure that sounds good. |
|
@bowenwang1996 Please see #3033 |
birchmd
force-pushed
the
fix-chunks-handle-missing-block
branch
from
98d6d5e to
705fd54
Compare
|
@bowenwang1996 Successfully rebased. Please take another look. |
| @@ -921,6 +921,12 @@ impl ShardsManager { | |||
| return Err(Error::InvalidPartMessage); | |||
| } | |||
|
|
|||
| // check part merkle proofs | |||
| let num_total_parts = self.runtime_adapter.num_total_parts(); | |||
There was a problem hiding this comment.
any reason for moving this block of code?
There was a problem hiding this comment.
This check doesn't require looking up the epoch id. I thought it might be good to do as many check as possible before hitting the missing block error.
There was a problem hiding this comment.
I think it should be the other way around: the part of the processing that happens before the signature validation we do for each received chunk.
So if someone is sending invalid chunks (or many valid chunks maliciously created for the same height), if the signature verification goes first, we will only check the signature for each of them. Otherwise we will do all the steps above. And in particular verifying the merkle path is not very cheap (though may be not too expensive compared to the sig verification).
There was a problem hiding this comment.
But isn't it easier to send chunks for blocks that don't exist if we put the signature check first? All the fields could contain random bytes and we would accept the message because the parent hash is not known. If the signature check is last we will at least know this is some kind of well-formed chunk.
There was a problem hiding this comment.
I see your point, someone can send a message that has invalid data, but valid chunk hash and a signature.
Then I agree, checking the merkle proofs before the chunk signature is the correct approach.
bowenwang1996
approved these changes
Jul 25, 2020
birchmd
added a commit
that referenced
this pull request
Jul 28, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Aug 7, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Aug 7, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Aug 21, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Aug 22, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Aug 24, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Sep 3, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Sep 17, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
birchmd
added a commit
that referenced
this pull request
Sep 28, 2020
…signature check to fail (#3026) Description: For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block. Testing plan: * New unit test for processing a chunk with an unknown parent block
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block.
Testing plan: