-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Refactor check_validation_outputs #4727
Changes from 1 commit
d247b75
de8235d
6371754
9c7b108
cfd1223
765461d
675dde3
6158f51
b798f91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -594,6 +594,7 @@ impl<T: Config> Pallet<T> { | |
| let scheduled = <scheduler::Pallet<T>>::scheduled(); | ||
|
|
||
| let relay_parent_number = now - One::one(); | ||
| let parent_storage_root = parent_header.state_root().clone(); | ||
|
|
||
| let check_ctx = CandidateCheckContext::<T>::new(now, relay_parent_number); | ||
| let backed_candidates = sanitize_backed_candidates::<T, _>( | ||
|
|
@@ -609,7 +610,7 @@ impl<T: Config> Pallet<T> { | |
| // That way we avoid possible duplicate checks while assuring all | ||
| // backed candidates fine to pass on. | ||
| check_ctx | ||
| .verify_backed_candidate(parent_hash, candidate_idx, backed_candidate) | ||
| .verify_backed_candidate(parent_hash, parent_storage_root, candidate_idx, backed_candidate) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is correct, we'd want to filter out all of them, not only the outer error. Missing a para chain header is also a good enough reason to drop the candidate iiuc
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm having a hard time connecting your comment to the code here. Can you explain in more detail?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are now only considering the outer error of the nested result, I'd argue that an error of the inner result type is also a good enough reason to discard the backed candidate iiuc. This can be deferred to a follow up PR, since as you correctly stated, behavior does not change compared to current master
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, fair. We should probably discard all backed candidates when encountering this error. It means something is seriously wrong and governance will probably need to step in. The on-chain logic is to ignore all the candidates anyway for the same reasons.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume we can change that in the next release/new PR. |
||
| .is_err() | ||
| }, | ||
| &scheduled[..], | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.