Check the execution receipt received from network properly #236
Conversation
nazar-pc
left a comment
nazar-pc
left a comment
There was a problem hiding this comment.
Overall, I think we need something like orphan ERs in this case to handle it gracefully, but it is definitely tricky to do in a way that is DoS resistant
| rx.recv()?? | ||
| }; | ||
|
|
||
| // TODO: What happens for this obvious error? |
There was a problem hiding this comment.
Same as in any other case, you compare trace item one by one and generate fraud proof for item that is wrong or missing
There was a problem hiding this comment.
There is a possible case that an external ER that contains all the valid roots and some extra ones? In that case comparing on the iterator won't detect it, but I believe the block will still fail in the verification somewhere else.
There was a problem hiding this comment.
Everything is possible, adversary can create whatever ER they want 🙂
liuchengxu
force-pushed
the
check-external-ER
branch
from
efd84d7 to
e0631d3
Compare
liuchengxu
force-pushed
the
check-external-ER
branch
2 times, most recently
from
67f06be to
65320d4
Compare
liuchengxu
force-pushed
the
check-external-ER
branch
from
65320d4 to
86236f0
Compare
liuchengxu
force-pushed
the
check-external-ER
branch
2 times, most recently
from
7e0b1d9 to
7381b67
Compare
liuchengxu
force-pushed
the
check-external-ER
branch
from
41d70fd to
d3e1903
Compare
liuchengxu
force-pushed
the
check-external-ER
branch
from
d3e1903 to
5363a50
Compare
|
@nazar-pc This PR is ready for another review, the last 4 commits are new since your last review. |
nazar-pc
left a comment
nazar-pc
left a comment
There was a problem hiding this comment.
Some comments, but nothing critical
| let mut to_delete_block_number = first_saved_receipt; | ||
| while to_delete_block_number < new_first_saved_receipt { |
There was a problem hiding this comment.
to_delete_block_number is basically from and new_first_saved_receipt is basically to.
Instead of transforming those into Block::Header::Number, I'd convert them into subspace_core_primitives::BlockNumber and iterated over the range of numbers instead, that would be easier to read rather than manually incrementing stuff.
There was a problem hiding this comment.
Converting to subspace_core_primitives::BlockNumber might be helpful to simplify the loop, but you have to convert it back to Number as the encode for u32 and Block::Header::Number is different(discovered by the existing test, perhaps the compact of codec is related, I didn't look into this) unless you want to use u32 as the part of the final key to insert into the store.
There was a problem hiding this comment.
I think it is fine to use BlockNumber as a final key. Also it might be different in tests (I see that often tests use u64 for block number), but should be the same in production since we use u32 everywhere as a block number otherwise.
| let mut new_first_saved_receipt = first_saved_receipt; | ||
|
|
||
| if block_number - first_saved_receipt >= PRUNING_DEPTH.saturated_into() { | ||
| new_first_saved_receipt = block_number.saturating_sub((PRUNING_DEPTH - 1).saturated_into()); |
There was a problem hiding this comment.
In these cases I would honestly prefer .expect() since we don't expect these numbers to ever be beyond u32::MAX.
There was a problem hiding this comment.
Feel free to refactor it directly as I don't see another style that is satisfying to me using expect() :P.
Refactor `ExecutionReceipt`: - Rename `state_transition_root` to `trace_root`. - Include the list of all storage roots in the execution as `trace`.
Only the authority executors are allowed to participate the election, the winner of which will be able to broadcast the exeuction recepit to the network, and then we could run an authority executor node and a full executor node locally.
We have to use the block number as key because the time is measured in block number only.
Store the execution receipt by block hash, use another storage for tracking the associated block number to have knowledge of if it's too old and should be pruned. This commit also removes the deletion of local ER even it's correctly validated, now the obsolete ERs will be removed soly according to the prune depth.
…te_roots()` Using `parent_hash` is wrong because we need to call the api on top of the newly created best block to get the intermediate roots for this just executed block.
liuchengxu
force-pushed
the
check-external-ER
branch
from
5363a50 to
81eeeca
Compare
liuchengxu
force-pushed
the
check-external-ER
branch
from
81eeeca to
872b1b4
Compare
This PR is mainly for making the locally produced execution receipt persistent for a while so that the executor can compare it with the external ER and propose a fraud proof in case of any invalid state transition in the external ER.
The first 3 commits are basically about some refactorings, and then the core of this PR follows. Making it a draft due to these TODOs, but the other commits are ready to be reviewed.
TODOs:
Better approach than using, acceptable for now, will have to see if it's usable for the early stage.(Ref Check the execution receipt received from network properly #236 (comment)).sleep()while waiting for the local ER