feat!: dedicated security error code for ongoing fork #583
Conversation
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
| @@ -15,4 +16,13 @@ pub(crate) mod security { | |||
| ) -> Result<bool, DispatchError> { | |||
| <security::Pallet<T>>::parachain_block_expired(opentime, period) | |||
| } | |||
|
|
|||
| pub fn insert_ongoing_fork_error<T: crate::Config>() { | |||
| <security::Pallet<T>>::set_status(StatusCode::Error); | |||
There was a problem hiding this comment.
Is it possible for the parachain to be in shutdown when this happens? If not, please add a comment somewhere to that effect
| @@ -535,6 +539,7 @@ impl<T: Config> Pallet<T> { | |||
| // if we added a block to a fork, we may need to reorder the chains | |||
| Self::reorganize_chains(&blockchain)?; | |||
| } else { | |||
| Self::update_no_ongoing_fork(current_block_height)?; | |||
There was a problem hiding this comment.
I think this code path can be taken even when there is a fork ahead of the mainchain (since I think reordering is not done until a fork gets 6 blocks ahead). If that is the case, this update_no_ongoing_fork is incorrect.
This PR looks like a simple change, but the bitcoin code is deceptively complex and error prone. I would like to see more tests, including for the scenario above. Additionally, I would like to have a check that the error status is equal to the output of ensure_no_ongoing_fork in check_store_block_header_invariants
|
Superseded by #1119 - the error type was removed. We should update downstream systems to determine transaction validity some other way, possibly by dry running first. |
Signed-off-by: Gregory Hill gregorydhill@outlook.com
Closes #565