feat(error): revamp make_canonical error
#3899
Conversation
Codecov Report
... and 263 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mattsse
added
C-debt
|
@Rjected friendly bump looks like some tests are stalling though @tcoratger perhaps we also need to update some checks? |
|
Looks like the tests still need to be fixed, @tcoratger do you need any help on this? |
|
@Rjected Yes sure, I thought my last commit solved the problem but apparently not. I feel like the CI tests stop because many of them are too slow. But I don't understand why because nothing fundamental changes here? Maybe I missed something. |
|
@mattsse Do you know why the tests fail here? |
| @@ -1398,7 +1398,9 @@ where | |||
| // it's part of the canonical chain: if it's the safe or the finalized block | |||
| if matches!( | |||
| err, | |||
| Error::Execution(BlockExecutionError::BlockHashNotFoundInChain { .. }) | |||
| Error::Canonical(CanonicalError::BlockchainTree( | |||
There was a problem hiding this comment.
@mattsse @Rjected I still don't understand what's going wrong with this one. @mattsse I just saw that you had merge master in there two weeks ago but that doesn't change anything. Considering where the tests are slow, I imagine it's coming from that place in the code right? but it seems strange to me because there is no major change right?
There was a problem hiding this comment.
some change must have led to an issue with the tokio test runner
try with #[tokio::test(flavor = "multi_thread")] for forkchoice_updated_invalid_pow
and you'll see a failing assertion
assertion
left == rightfailed
left: ForkchoiceUpdated { payload_status: PayloadStatus { status: Syncing, latest_valid_hash: None }, payload_id: None }
right: ForkchoiceUpdated { payload_status: PayloadStatus { status: Invalid { validation_error: "Block 0x1a46761f926c32ba02e1da554fc3e4b53af8b44fb55e20036370745b54a2246f is pre merge" }, latest_valid_hash: Some(0x0000000000000000000000000000000000000000000000000000000000000000) }, payload_id: None }
| error @ BlockExecutionError::Validation(BlockValidationError::BlockPreMerge { | ||
| .. | ||
| }), | ||
| Error::Canonical( |
There was a problem hiding this comment.
@mattsse I think I spotted the bug, this one had to be modified too. Now it should be good
This reverts commit afbe88f.
Should solve #2752.
I added a dedicated
CanonicalErrorenum and embedded that insidereth_interfaces::error::Error. I don't know whether to also change the declaration of the functionpub fn make_canonical(&mut self, block_hash: &BlockHash) -> Result<CanonicalOutcome, Error>topub fn make_canonical(&mut self, block_hash: &BlockHash) -> Result<CanonicalOutcome, CanonicalError>but I think that would induce many other changes which are not desired, am I wrong?