Fix primary_parent_hash and add domain_parent_hash in invalid state transition proof#1291
Merged
liuchengxu merged 3 commits intomainfrom Mar 26, 2023
Merged
Conversation
…erating the invalid state transition proof The main point of this commit is to fix a bug that the legacy `parent_hash` in `InvalidStateTransitionProof` was incorrectly initialized as the domain parent hash, which should be the parent hash of the parent block(used to retrieve the domain runtime code from the primary chain using the primary chain runtime api). We didn't catch it earlier because it was manually set to the _right_ hash in the test by accident which is essentially caused the gap between the test and production environment. I hope it won't happen again when we add tests closer to the production code with the new testing framework. This commit also makes it more explicit by renaming `parent_hash` to `primary_parent_hash`.
liuchengxu
requested review from
nazar-pc,
rg3l3dr and
vedhavyas
as code owners
Closed
2 tasks
nazar-pc
approved these changes
Mar 25, 2023
| let parent_hash = T::Hash::decode(&mut fraud_proof.parent_hash.encode().as_slice()) | ||
| .map_err(|_| FraudProofError::WrongHashType)?; | ||
| let primary_parent_hash = | ||
| T::Hash::decode(&mut fraud_proof.primary_parent_hash.encode().as_slice()) |
Member
There was a problem hiding this comment.
Another lesson learned is that encoding/decoding erases types and can easily cause problems. We should consider adding to/from bounds and ensure that types are compatible statically even if we can't name them explicitly due to use of generics.
Contributor
Author
There was a problem hiding this comment.
Although I'm not super happy about adding the from/to bounds due to several reasons, I agree on the benefits gained from the compiler. We had a TODO, but haven't fixed it and applied the rule everythere.
liuchengxu
deleted the
fix-primary_parent_hash-in-InvalidStateTransitionProof
branch
i1i1
added a commit
to autonomys/subspace-pulsar-sdk
that referenced
this pull request
Mar 26, 2023
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
There are two commits in this PR, the code changes are not a lot, but they are kind of tricky, let me know if the following context is still confusing.
The first commit fixes a bug that
parent_hashshould be the hash of primary parent block but incorrectly construcuted as the hash of domain parent block when generating the proof.https://github.com/subspace/subspace/blob/f6c1c5480bf616f27d1dce9dfbf13704a5f80307/domains/client/domain-executor/src/fraud_proof.rs#L145
It can be easier to understand from the call site where
parent_hashwas used to invoke the primary chain runtime api to fetch the domain runtime from the primary chain.https://github.com/subspace/subspace/blob/f6c1c5480bf616f27d1dce9dfbf13704a5f80307/crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs#L407-L413
The first commits fixes it and rename it to
primary_parent_hashfor better clarity.BTW, why it wasn't caught earlier in the test? Because we manually constructed the proof in the tests
(it happens to use the correct hash) to essentially only ensure the execution prover engine itself runs well and didn't cover the
FraudProofGeneratorwhich was actually used in the production. The lesson learnt is to have more tests close the production workflow, I believe @NingLin-P can contribute significantly with the new testing infra on this front.The second commit adds back the
domain_parent_hashtoExecutionPhase::InitializeBlockas it's needed to retrieve the pre_state_root for InitializeBlock, which happens to resolves Retrieve state transition data forInitializeBlock#1289 (I had a misunderstanding there, given this usage inpre_state_root, we could use thedomain_parent_hashdirectly when rebuilding the state transition data for InitializeBlock)Close Retrieve state transition data for
InitializeBlock#1289Code contributor checklist: