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

Fraud proof is not verified in the block import pipeline #1527

Closed
Tracked by #1706
NingLin-P opened this issue Jun 9, 2023 · 2 comments
Closed
Tracked by #1706

Fraud proof is not verified in the block import pipeline #1527

NingLin-P opened this issue Jun 9, 2023 · 2 comments
Labels
bug Something isn't working execution Subspace execution fraud-proof
Milestone

Comments

@NingLin-P
Copy link
Member

In #1456 we changed the way to extract fraud proof from block to only extract the fraud proof that has been accepted on the runtime. The hash of these fraud proof will be stored at a temporary storage SuccessfulFraudProofs and the client will use a runtime API to query this storage to determine if a fraud proof is accepted:
https://github.com/subspace/subspace/blob/0df744b2987c3076f59ff111197bb66ae74e3568/crates/sc-consensus-fraud-proof/src/lib.rs#L82-L105

However, we have used the wrong hash for the runtime API, using the parent_hash will query the parent block's SuccessfulFraudProofs storage while checking against the fraud proof in the current block's extrinsic. Thus we won't get the fraud proof even if it is accepted on the runtime.

Simply changing header.parent_hash() to header.hash() doesn't help because the client's import_block (namely inner.import_block) is not invoked yet thus the block is not committed to the backend storage, using header.hash() will result in some error like calling runtime API with unknown block hash.

Moving inner.import_block in front of the fraud proof check also does not help, because if the fraud proof is invalid we want to abort the block import pipeline to reject this block, but at that time the block is already committed to the backend storage through inner.import_block. So we are in a dilemma here.

cc @liuchengxu

@liuchengxu
Copy link
Contributor

Looking at the runtime logic of process_fraud_proof, I find there are only two fallible operations that are related to the PrimaryBlockHash we plan to remove in DecEx v2, the other logics are either to remove the invalid receipt or to slash the operator, which is infallible.

https://github.com/subspace/subspace/blob/6b538a0f60586f42f158c2f2ca0bf5993cd35324/crates/pallet-settlement/src/lib.rs#L339

https://github.com/subspace/subspace/blob/6b538a0f60586f42f158c2f2ca0bf5993cd35324/crates/pallet-settlement/src/lib.rs#L345

That being said, SuccessfullFraudProofs is unnecessary, all the submit_fraud_proof extrinsics included in the block must be successful so that we can simply extract them out of the extrinsic. In that case, the wrong block hash should be fine practically.

@liuchengxu liuchengxu added the bug Something isn't working label Jun 10, 2023
@liuchengxu liuchengxu added this to the Decoupled Execution v2 milestone Jun 10, 2023
@liuchengxu liuchengxu added the execution Subspace execution label Jun 10, 2023
@NingLin-P NingLin-P mentioned this issue Aug 21, 2023
30 tasks
@vedhavyas
Copy link
Contributor

Closing this as this is not required anymore since we started using Host function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Subspace execution fraud-proof
Projects
Status: Done
Development

No branches or pull requests

4 participants