Optimize search for hard fork boundary

[izquierdo-datachain]

Visiting every block when searching for the hard fork boundary (when the boundary is specified by timestamp instead of by height) can cause the relayer to hang for a long time without processing packets.

This PR implements a faster version of the boundary search.

[abicky]

What scenario is this condition indented for? From what I can tell in the code, this condition will never be met because both blockCountPrevCur and timeDiffPrevCur appears to always be positive.

[izquierdo-datachain]

I'll answer separately for the two sub-scenarios: avgBlocksPerMs == 0 and avgBlocksPerMs < 0:

• avgBlocksPerMs == 0

With float64 division, this number can become 0 for if blockCountPrevCur is very small and timeDiffPrevCur is very big. In normal scenarios this shouldn't happen, but I left it as a prevention measure.

For reference, there's one scenario I found where avgBlocksPerMs would be very small which occurs when running a new test node locally, because block 0 has a timestamp from year 2020, and blocks 1 and further have a timestamp of e.g. 2025, so avgBlocksPerMs ends up being something like "1 block per every 5 years". While this particular scenario is not enough to make the average rate be 0, I preferred to be defensive and catch that potential scenario.

• avgBlocksPerMs < 0

I agree that from this code, this condition can never be met. However, if we're going to be checking for == 0 I thought we might as well also check for <. Ideally what I would prefer here is an assertion statement though.

[abicky]

Thank you for the explanation!
I tried to reproduce the scenario you described but I couldn't: https://go.dev/play/p/6CHHW7OZqC9

Would the following change make sense instead?

diff --git a/module/fork_spec.go b/module/fork_spec.go
index 46d3584..d7716cd 100644
--- a/module/fork_spec.go
+++ b/module/fork_spec.go
@@ -346,6 +346,13 @@ func estimateDistance(previousHeader, currentHeader *types.Header, targetTs uint
        blockCountPrevCurBig = blockCountPrevCurBig.Abs(blockCountPrevCurBig)
        blockCountPrevCur, _ := blockCountPrevCurBig.Float64()

+       if blockCountPrevCur == 0 {
+               // Blocks are being produced so slowly that the current block is still expected
+               // to be the latest block at any future timestamp. Return 1 to avoid getting stuck
+               // in the current block.
+               return 1
+       }
+
        if currentTs > previousTs {
                timeDiffPrevCur = currentTs - previousTs
        } else {
@@ -365,13 +372,5 @@ func estimateDistance(previousHeader, currentHeader *types.Header, targetTs uint
        }

        avgBlocksPerMs := blockCountPrevCur / float64(timeDiffPrevCur)
-
-       if avgBlocksPerMs <= 0 {
-               // Blocks are being produced so slowly that the current block is still expected
-               // to be the latest block at any future timestamp. Return 1 to avoid getting stuck
-               // in the current block.
-               return 1
-       }
-
        return uint64(math.Ceil(avgBlocksPerMs * float64(timeDiffTargetCur)))
 }

[yoshidan]

I can see from the code that it cannot be lower than 0, but I think the reason for the discussion is that avgBlocksPerMs <= 0 is explicitly stated.
The following reverse branching might be better.

if avgBlocksPerMs > 0 {
      return uint64(math.Ceil(avgBlocksPerMs * float64(timeDiffTargetCur)))
}
return 1

[izquierdo-datachain]

Would the following change make sense instead?

While the early check makes sense, I would still like to check the result of avgBlocksPerMs. To clarify more:

https://go.dev/play/p/UwgHE3fMCUf

outputs

a > 0? true
b > 0? true
(a / b) == 0? true

So with float64, a > 0 && b > 0 does not imply a / b > 0 (even though in general math the implication does hold).

I tried to reproduce the scenario you described but I couldn't

Sorry for the confusion. In my local testing, avgBlocksPerMs did not become equal to 0 either. I only used that as an example of how the number can have an unexpectedly low value even though usually blocks are produced at a sane rate, which makes me worried about an unknown scenario where it becomes 0 causing the relayer to become stuck.

[abicky]

This is a really efficient and clever approach 👍 👍 👍

[yoshidan]

I think it is a very nice and efficient logic.
I would suggest adding test code to searchBoundaryHeight and estimateDistance to prevent degreasing when modifying.

[yoshidan]

I can see from the code that it cannot be lower than 0, but I think the reason for the discussion is that avgBlocksPerMs <= 0 is explicitly stated.
The following reverse branching might be better.

if avgBlocksPerMs > 0 {
      return uint64(math.Ceil(avgBlocksPerMs * float64(timeDiffTargetCur)))
}
return 1

[abicky]

👍

[yoshidan]

Please add the test code for searchBoundaryHeight and estimateDistance.

[izquierdo-datachain]

Thanks for catching the mixed up test names!