Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 135 additions & 18 deletions module/fork_spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@ package module
import (
"context"
"fmt"
"github.com/cockroachdb/errors"
"github.com/hyperledger-labs/yui-relayer/log"
math "math"
"math/big"
"os"
"slices"
"strconv"

"github.com/cockroachdb/errors"
"github.com/ethereum/go-ethereum/core/types"
"github.com/hyperledger-labs/yui-relayer/log"
)

type Network string
Expand Down Expand Up @@ -224,6 +228,7 @@ func FindTargetForkSpec(forkSpecs []*ForkSpec, height uint64, timestamp uint64)
var boundaryHeightCache = make(map[uint64]uint64)

func GetBoundaryHeight(ctx context.Context, headerFn getHeaderFn, currentHeight uint64, currentForkSpec ForkSpec) (*BoundaryHeight, error) {
var err error
logger := log.GetLogger()
boundaryHeight := uint64(0)
if condition, ok := currentForkSpec.GetHeightOrTimestamp().(*ForkSpec_Height); ok {
Expand All @@ -234,27 +239,139 @@ func GetBoundaryHeight(ctx context.Context, headerFn getHeaderFn, currentHeight
boundaryHeight = v
} else {
logger.DebugContext(ctx, "seek fork height", "currentHeight", currentHeight, "ts", ts)
for i := int64(currentHeight); i >= 0; i-- {
h, err := headerFn(ctx, uint64(i))
if err != nil {
return nil, err
}
if MilliTimestamp(h) == ts {
boundaryHeight = h.Number.Uint64()
logger.DebugContext(ctx, "seek fork height found", "currentHeight", currentHeight, "ts", ts, "boundaryHeight", boundaryHeight)
boundaryHeightCache[ts] = boundaryHeight
break
} else if MilliTimestamp(h) < ts {
boundaryHeight = h.Number.Uint64() + 1
logger.DebugContext(ctx, "seek fork height found", "currentHeight", currentHeight, "ts", ts, "boundaryHeight", boundaryHeight)
boundaryHeightCache[ts] = boundaryHeight
break
}
boundaryHeight, err = searchBoundaryHeight(ctx, currentHeight, ts, headerFn)
if err != nil {
return nil, err
}
boundaryHeightCache[ts] = boundaryHeight
}
}
return &BoundaryHeight{
Height: boundaryHeight,
CurrentForkSpec: currentForkSpec,
}, nil
}

func searchBoundaryHeight(ctx context.Context, currentHeight uint64, targetTs uint64, headerFn getHeaderFn) (uint64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really efficient and clever approach 👍 👍 👍

// There are potentially many blocks between the boundary and the current
// blocks. Also, finding the timestamp for a particular block is expensive
// as it requires an RPC call to a node.
//
// Thus, this implementation aims to prune a large number of blocks from the
// search space by estimating the distance between the boundary and the
// current block (based on the average rate of block production) and jumping
// directly to a candidate block at that distance. In case of a miss, all
// blocks on one side of the candidate can be discarded, and a new attempt
// can be made by re-estimating the new distance and jumping to a candidate
// on the other side.
//
// Theoretical worst-case performance is O(N), but since the rate of block
// production can be predicted with high accuracy, this implementation is
// expected to be faster than binary search in practice.
var (
position uint64 = currentHeight // candidate block number currently under consideration
low uint64 = 0 // inclusive lower bound of the current search range
high uint64 = currentHeight + 1 // exclusive upper bound of the current seach range
previousHeader *types.Header // header of the block seen in the previous iteration
)

// Loop invariant:
//
// 0 <= low <= position < high <= currentHeight + 1
// &&
// low <= result < high
//
// Bound function (decreases in each iteration, and is always >= 0):
//
// high - low
for low < high {
currentHeader, err := headerFn(ctx, uint64(position))
if err != nil {
return 0, err
}

currentTs := MilliTimestamp(currentHeader)
if currentTs == targetTs {
return currentHeader.Number.Uint64(), nil
}

distance := estimateDistance(previousHeader, currentHeader, targetTs)

if currentTs > targetTs {
// Jump to a lower block.
high = position

// Since these are unsigned, position-distance might underflow.
if low+distance > position {
position = low
} else {
position = position - distance
}
} else {
// Jump to a higher block.
low = position + 1

position = position + distance

if position >= high {
position = high - 1
}
}

previousHeader = currentHeader
}

// If no block with an exact timestamp match was found, then we want the
// earliest block that's _after_ the target timestamp.
return low, nil
}

// estimateDistance returns the estimated number of blocks between the block indicated by currentHeader
// and the boundary block nearest to targetTs. It assumes that previousHeader either is nil, or refers to
// a different block than currentHeader.
func estimateDistance(previousHeader, currentHeader *types.Header, targetTs uint64) uint64 {
if previousHeader == nil {
return 1
}

var (
timeDiffPrevCur uint64 // milliseconds between the previous and current blocks
timeDiffTargetCur uint64 // milliseconds between the current block and target timestamp
)

currentTs := MilliTimestamp(currentHeader)
previousTs := MilliTimestamp(previousHeader)

blockCountPrevCurBig := new(big.Int).Sub(previousHeader.Number, currentHeader.Number)
blockCountPrevCurBig = blockCountPrevCurBig.Abs(blockCountPrevCurBig)
blockCountPrevCur, _ := blockCountPrevCurBig.Float64()

if currentTs > previousTs {
timeDiffPrevCur = currentTs - previousTs
} else {
timeDiffPrevCur = previousTs - currentTs
}

if timeDiffPrevCur == 0 {
// Found two different blocks with the same timestamp. The distance
// should be at least 1 to avoid getting stuck in the current block.
return 1
}

if currentTs > targetTs {
timeDiffTargetCur = currentTs - targetTs
} else {
timeDiffTargetCur = targetTs - currentTs
}

avgBlocksPerMs := blockCountPrevCur / float64(timeDiffPrevCur)

if avgBlocksPerMs <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))
 }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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)))
}