Orphaned block exception in SuggestBlock #5152
Conversation
|
posdao and engine hive tests are green |
|
What about hive consensus tests, there is no regression? |
I'm still running them. Will let you know when ended |
There was a problem hiding this comment.
This branch is fixing hive consensus test UncleFromSideChain_Merge:
https://github.com/NethermindEth/nethermind/actions/runs/4045495974/jobs/6957042719
Which fails on master:
https://github.com/NethermindEth/nethermind/actions/runs/4045499712/jobs/6957050455
There is no regression in other tests so IMO ready to be merged
| @@ -1962,7 +1962,7 @@ private void SetTotalDifficulty(BlockHeader header) | |||
| void SetTotalDifficultyDeep(BlockHeader current) | |||
| { | |||
| Stack<BlockHeader> stack = new(); | |||
| while (!current.IsNonZeroTotalDifficulty()) | |||
| while (current.TotalDifficulty is null) | |||
There was a problem hiding this comment.
this is quite a big logical change. I think it has been done only to handle TotalDifficulty = 0 in the genesis block. Could we break while when we reach the genesis block instead of logic change?
There was a problem hiding this comment.
In this case every computation of td will require a computation of td for every previous block. It's too slow I think
There was a problem hiding this comment.
You're right but I'm also right :)
Your fix is for the edge case situation when we start with genesis.Difficulty = 0, TTD = 0
The current code is working fine in the transition period. The total difficulty in DB is not nullable, because of that we have to save TD as 0 zero in DB. However, TD = 0 means that we don't have the current total difficulty. Basically, in this context TD=0 means the same as null.
Could you think about how to resolve both fixes? I believe good condition (with some reference to genesis) will handle both cases
| @@ -227,7 +227,7 @@ private bool ValidateTimestamp(BlockHeader parent, BlockHeader header) | |||
| protected virtual bool ValidateTotalDifficulty(BlockHeader parent, BlockHeader header) | |||
| { | |||
| bool totalDifficultyCorrect = true; | |||
| if (header.IsNonZeroTotalDifficulty()) | |||
| if (header.TotalDifficulty is not null) | |||
There was a problem hiding this comment.
IsNonZeroTotalDifficulty has this impl : header.TotalDifficulty is not null && header.TotalDifficulty != UInt256.Zero, so why does the new code not check if it's a Zero as well ?
Fixes Closes Resolves #
In some Ethereum merge tests difficulty for all blocks is zero. In this case we failing in td calculation.
Fixes hive/consensus UncleFromSideChain_Merge test
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?