Skip to content

Commit

Permalink
chore(nodebuilder/das | pruner): Privatise samplingWindow in DAS pa…
Browse files Browse the repository at this point in the history
…rams, add Duration method to `AvailabilityWindow` (#3378)

While this isn't really a bug fix, it's more of a "UX" fix - I'm
removing `SamplingWindow` from the node's config so that it doesn't
confuse users to see "0ms" as the config value even though in practice
it's 30 days.

The reason there's a discrepancy is because we force the light sampling
window in the DASer for light nodes in nodebuilder anyway so that config
value is ignored regardless, meaning it does not need to be exposed.

Also adding `Duration()` method to return time.Duration instead of
having to cast everywhere.
  • Loading branch information
renaynay authored May 15, 2024
1 parent 8398afa commit 007dae7
Show file tree
Hide file tree
Showing 9 changed files with 23 additions and 17 deletions.
4 changes: 2 additions & 2 deletions das/daser.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,10 @@ func (d *DASer) sample(ctx context.Context, h *header.ExtendedHeader) error {

func (d *DASer) isWithinSamplingWindow(eh *header.ExtendedHeader) bool {
// if sampling window is not set, then all headers are within the window
if d.params.SamplingWindow == 0 {
if d.params.samplingWindow.Duration() == 0 {
return true
}
return time.Since(eh.Time()) <= d.params.SamplingWindow
return time.Since(eh.Time()) <= d.params.samplingWindow.Duration()
}

// SamplingStats returns the current statistics over the DA sampling process.
Expand Down
3 changes: 2 additions & 1 deletion das/daser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/celestiaorg/celestia-node/header"
"github.com/celestiaorg/celestia-node/header/headertest"
headerfraud "github.com/celestiaorg/celestia-node/header/headertest/fraud"
"github.com/celestiaorg/celestia-node/pruner"
"github.com/celestiaorg/celestia-node/share"
"github.com/celestiaorg/celestia-node/share/availability/full"
"github.com/celestiaorg/celestia-node/share/availability/light"
Expand Down Expand Up @@ -260,7 +261,7 @@ func TestDASer_SamplingWindow(t *testing.T) {

// create and start DASer
daser, err := NewDASer(avail, sub, getter, ds, fserv, newBroadcastMock(1),
WithSamplingWindow(time.Second))
WithSamplingWindow(pruner.AvailabilityWindow(time.Second)))
require.NoError(t, err)

tests := []struct {
Expand Down
12 changes: 7 additions & 5 deletions das/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"errors"
"fmt"
"time"

"github.com/celestiaorg/celestia-node/pruner"
)

// ErrInvalidOption is an error that is returned by Parameters.Validate
Expand Down Expand Up @@ -42,10 +44,10 @@ type Parameters struct {
// ConcurrencyLimit.
SampleTimeout time.Duration

// SamplingWindow determines the time window that headers should fall into
// samplingWindow determines the time window that headers should fall into
// in order to be sampled. If set to 0, the sampling window will include
// all headers.
SamplingWindow time.Duration
samplingWindow pruner.AvailabilityWindow
}

// DefaultParameters returns the default configuration values for the daser parameters
Expand Down Expand Up @@ -163,9 +165,9 @@ func WithSampleTimeout(sampleTimeout time.Duration) Option {
}

// WithSamplingWindow is a functional option to configure the DASer's
// `SamplingWindow` parameter.
func WithSamplingWindow(samplingWindow time.Duration) Option {
// `samplingWindow` parameter.
func WithSamplingWindow(samplingWindow pruner.AvailabilityWindow) Option {
return func(d *DASer) {
d.params.SamplingWindow = samplingWindow
d.params.samplingWindow = samplingWindow
}
}
3 changes: 1 addition & 2 deletions nodebuilder/das/constructors.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package das
import (
"context"
"fmt"
"time"

"github.com/ipfs/go-datastore"

Expand Down Expand Up @@ -49,7 +48,7 @@ func newDASer(
availWindow pruner.AvailabilityWindow,
options ...das.Option,
) (*das.DASer, *modfraud.ServiceBreaker[*das.DASer, *header.ExtendedHeader], error) {
options = append(options, das.WithSamplingWindow(time.Duration(availWindow)))
options = append(options, das.WithSamplingWindow(availWindow))

ds, err := das.NewDASer(da, hsub, store, batching, fraudServ, bFn, options...)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pruner/find.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func (s *Service) findPruneableHeaders(
ctx context.Context,
lastPruned *header.ExtendedHeader,
) ([]*header.ExtendedHeader, error) {
pruneCutoff := time.Now().UTC().Add(time.Duration(-s.window))
pruneCutoff := time.Now().UTC().Add(-s.window.Duration())

if !lastPruned.Time().UTC().Before(pruneCutoff) {
// this can happen when the network is young and all blocks
Expand Down
2 changes: 1 addition & 1 deletion pruner/full/window_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ import (
// TestFullWindowConst exists to ensure that any changes to the sampling window
// are deliberate.
func TestFullWindowConst(t *testing.T) {
assert.Equal(t, time.Duration(Window), (30*24*time.Hour)+time.Hour)
assert.Equal(t, Window.Duration(), (30*24*time.Hour)+time.Hour)
}
2 changes: 1 addition & 1 deletion pruner/light/window_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ import (
// TestLightWindowConst exists to ensure that any changes to the sampling window
// are deliberate.
func TestLightWindowConst(t *testing.T) {
assert.Equal(t, time.Duration(Window), 30*24*time.Hour)
assert.Equal(t, Window.Duration(), 30*24*time.Hour)
}
4 changes: 2 additions & 2 deletions pruner/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func TestPrune_LargeNumberOfBlocks(t *testing.T) {
require.NoError(t, err)

// ensures availability window has passed
time.Sleep(time.Duration(availabilityWindow) + time.Millisecond*100)
time.Sleep(availabilityWindow.Duration() + time.Millisecond*100)

// trigger a prune job
lastPruned, err := serv.lastPruned(ctx)
Expand Down Expand Up @@ -273,7 +273,7 @@ func TestFindPruneableHeaders(t *testing.T) {
require.NoError(t, err)
require.Len(t, pruneable, tc.expectedLength)

pruneableCutoff := time.Now().Add(-time.Duration(tc.availWindow))
pruneableCutoff := time.Now().Add(-tc.availWindow.Duration())
// All returned headers are older than the availability window
for _, h := range pruneable {
require.WithinRange(t, h.Time(), tc.startTime, pruneableCutoff)
Expand Down
8 changes: 6 additions & 2 deletions pruner/window.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@ import (

type AvailabilityWindow time.Duration

func (aw AvailabilityWindow) Duration() time.Duration {
return time.Duration(aw)
}

// IsWithinAvailabilityWindow checks whether the given timestamp is within the
// given AvailabilityWindow. If the window is disabled (0), it returns true for
// every timestamp.
func IsWithinAvailabilityWindow(t time.Time, window AvailabilityWindow) bool {
if window == AvailabilityWindow(time.Duration(0)) {
if window.Duration() == time.Duration(0) {
return true
}
return time.Since(t) <= time.Duration(window)
return time.Since(t) <= window.Duration()
}

0 comments on commit 007dae7

Please sign in to comment.