Revamp invalid state transition proof (part 2)#1270
Conversation
- On the verifier side, there is no need to include the full call data as the verifier has to retrieve it on its own. - On the prover side, now the call data is passed as an argument. The tests are disabled temperorarily.
`total_extrinsic` is added to `ExecutionPhase::FinalizeBlock`.
A new runtime api is added to fetch the state root of specific domain block as `pre_state_root` for `ExecutionPhase::InitializeBlock` is the state root of parent block. This commit completes the `pre_state_root` verification.
Changes the return type of `verify_pre_state_root` from `bool` to `Result<(), VerificationError>`.
…ng `post_state_root` as well This avoids doing the computation unnecessarily if someone creates a proof that proves nothing bad, verifying the execution is relatively a high cost operation for the verifier.
liuchengxu
requested review from
nazar-pc,
rg3l3dr and
vedhavyas
as code owners
crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs
Outdated
Show resolved
Hide resolved
| call_data: new_header.encode(), | ||
| }; | ||
| let execution_phase = ExecutionPhase::InitializeBlock; | ||
| let initialize_block_call_data = new_header.encode(); |
There was a problem hiding this comment.
The new_header is mock, I'm curious about why use a mock header here.
There was a problem hiding this comment.
It's not a mock and not supposed to be a mock, new_header is essentially the exact state transition data for InitializeBlock by which all the on_initialize hooks are executed.
It was correct when initially introduced, but now I think it's problematic for the system domain due to the changes on digests (we manually inject the primary block has info as a digest when building the system domain block), we should do the same here when proving the execution. For the core domain, it should still be correct.
I believe such glitches can be catched earlier by the tests when we re-enable them again. Probably nice to fix it when you update this test to the new testing framework?https://github.com/subspace/subspace/blob/236b3345ec028b521e6d6e1fbc5dec0a99f44eed/crates/subspace-fraud-proof/src/tests.rs#L171
There was a problem hiding this comment.
Sure! I will update this test with the new testing framework to cover the initialize_block case.
vedhavyas
left a comment
vedhavyas
left a comment
There was a problem hiding this comment.
I'm confused on the traces indexes we use to verify pre and post state root. Can you help me understand pls
| ExecutionPhase::InitializeBlock => todo!("Get state_root of parent domain block"), | ||
| ExecutionPhase::ApplyExtrinsic(trace_index_of_pre_state_root) | ||
| | ExecutionPhase::FinalizeBlock { | ||
| total_extrinsics: trace_index_of_pre_state_root, |
There was a problem hiding this comment.
This seems wrong to me. The total extrinsics for example are 4, we would have a total of 6 including initialise and finalise. From the executor changes, this seems to give extrinsic count which is 4, so the pre state root will be, pre start root of the finalize extrinsinc instead of initaialize extrinsic.
There was a problem hiding this comment.
There are 4 extrinsics, then the trace will a total of 6 state roots. Let's make a table for it:
| InitializeBlock | ApplyExtrinsic(0) | ApplyExtrinsic(1) | ApplyExtrinsic(2) | ApplyExtirnsic(3) | FinalizeBlock |
|---|---|---|---|---|---|
| trace0 | trace1 | trace2 | trace3 | trace4 | trace5 |
The pre_state_root of ApplyExtrinsic(0) is the state root after the last stage, i.e., trace0 after InitializeBlock.
The post_state_root of ApplyExtrinsic(0) is the state root after this stage itself, i.e., trace1 after ApplyExtrinsic(0).
In general, the trace index of pre_state_root and post_state_root of given ApplyExtrinsic(u32)/FinalizeBlock{total_extrinsics} is u32/total_extrinsics and u32/total_extrinsics + 1 respectively.
|
|
||
| let post_state_root_onchain = match execution_phase { | ||
| ExecutionPhase::InitializeBlock => trace | ||
| .get(0) |
There was a problem hiding this comment.
confused here again. Shouldn't we be lookig at the last state root in the trace?
3 participants
This PR finishes the verification of
pre_state_rootandpost_state_rootin the invalid state transition proof. The main change is replacing the full call data inExecutionPhasewith the corresponding trace index for the verifier to retrieve the state root. Some tests are disabled temperorarily, they will be enabled again when the last task in #1230 completes.Part of #1230
Code contributor checklist: