Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: #7833 mine: parent-grinding fault overcheck #7834

Closed

Conversation

llifezou
Copy link
Contributor

@llifezou llifezou commented Dec 20, 2021

Related Issues

#7833

Additional Information

This pr changes to the api because the slashfilter cannot determine whether the parent block is valid.
This may be the smallest fix, but there are changes to the API interface

@ZenGround0
Copy link
Contributor

ZenGround0 commented Jul 1, 2022

Hey @llifezou I'm not experienced with this part of the lotus code but its not clear to me that the first block is kept from propagating out over the network. If it propagates and we bypass slash filtering on the second block the miner will get slashed. Does this code prevent propagation of the first block?

Ok I think I understand this better. Unfortunately I think it is unsafe. If you release both blocks I think you will be slashed. If this code merges and a block miner falls into the situation you describe in #7833 I believe that miner can be slashed which is much worse than the slash filter taking away their block. Please verify that this is the case or explain what I'm missing. If we both agree on this I will close down this PR.

It is reasonable to ask the question of whether this type of slashing is actually desirable. It has the advantage of pushing block miners to wait until the full parent tipset is available before mining which is probably a good thing. It might unfairly punish nodes with high latency on the edge of a network. To me this is an open question and the right place to discuss it is in the FIP discussion forum.

@@ -48,7 +72,7 @@ func (f *SlashFilter) MinedBlock(bh *types.BlockHeader, parentEpoch abi.ChainEpo
}
}

{
if checkParentGrinding {
Copy link
Contributor

@ZenGround0 ZenGround0 Jul 1, 2022

Choose a reason for hiding this comment

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

Ok I'm starting to get it this is not two blocks in the epoch it is parent grinding fault. I still need to understand what's going on but my first impression is we either need to change this logic to more closely match the actual slashing logic directly and if we can't there's something unsafe going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should make sure to be safer, because the above is less likely to happen. But what I understand is that in this case, the abnormal parent block is not introduced into the chain, so it will not be punished. I understand that it may be over-checked

Copy link
Contributor

Choose a reason for hiding this comment

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

The nuance here is that the bad parent block is spread to the wider network. I understand that it doesn't make it onto the main chain. However that is not related to whether the block can cause a slashing event. As long as the block is sent out over the network anyone else who records that block has the right to slash you if you also mine the second block which ignores the bad parent.

Copy link
Contributor Author

@llifezou llifezou Jul 6, 2022

Choose a reason for hiding this comment

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

Oh, so it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also means that if miner get the right to manufacture two consecutive blocks, as long as the first block is invalid, the second block will also be lost.

@ZenGround0
Copy link
Contributor

ZenGround0 commented Jul 8, 2022

Moving discussion of the actual problem to fips discussion. Closing this down because the path forward won't involve code like this: its either going to be to do nothing, or remove this type of consensus fault from fault checking.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants