Skip to content

cmd: refuse unsafe in-progress fee recipient registrations #4821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
OisinKyne wants to merge 1 commit into from
Closed

cmd: refuse unsafe in-progress fee recipient registrations #4821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 35 additions & 0 deletions cmd/feerecipientsign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,10 @@ func filterPubkeysByStatus(
}

if matchingIncomplete != nil {
if err := checkAdoptableRegistration(matchingIncomplete.Message, explicitTimestamp); err != nil {
return nil, errors.Wrap(err, "refusing to adopt in-progress registration", z.Str("pubkey", valPubKey))
}

// Adopt the timestamp and gas limit from the in-progress group so all operators sign the same message.
if explicitTimestamp != 0 && !matchingIncomplete.Message.Timestamp.Equal(time.Unix(explicitTimestamp, 0)) {
log.Warn(ctx, "Ignoring --timestamp flag, adopting the in-progress registration timestamp so partial signatures aggregate", nil,
Expand Down Expand Up @@ -325,6 +329,11 @@ func filterPubkeysByStatus(
z.I64("quorum_timestamp", quorumGroup.Message.Timestamp.Unix()))
}
} else {
if gasLimit == 0 {
return nil, errors.New("could not determine gas limit for validator from cluster lock, overrides file or remote API; pass --gas-limit explicitly",
z.Str("pubkey", valPubKey))
}

if quorumGroup == nil && ok {
// Check if there's any incomplete group (with a different fee recipient) and no quorum yet.
// This means another operator started a fee change that hasn't completed — block.
Expand Down Expand Up @@ -389,6 +398,32 @@ func validateFeeRecipient(address string) error {
return nil
}

// maxAdoptedTimestampSkew is the maximum amount a timestamp adopted from an in-progress registration
// may be ahead of the local clock without explicit operator confirmation via --timestamp.
const maxAdoptedTimestampSkew = 5 * time.Minute

// checkAdoptableRegistration returns an error if the in-progress registration message has values that
// must not be adopted implicitly: a zero gas limit (never valid), or a timestamp too far in the future
// (which would block later registrations) unless the operator explicitly confirmed it via --timestamp.
func checkAdoptableRegistration(msg *eth2v1.ValidatorRegistration, explicitTimestamp int64) error {
if msg == nil {
return errors.New("in-progress registration has no message")
}

if msg.GasLimit == 0 {
return errors.New("in-progress registration has gas limit 0; coordinate with your cluster operators to start a new registration with a valid --gas-limit and --timestamp")
}

if msg.Timestamp.After(time.Now().Add(maxAdoptedTimestampSkew)) && explicitTimestamp != msg.Timestamp.Unix() {
return errors.New("in-progress registration timestamp is too far in the future; verify it with your cluster operators and pass the same timestamp explicitly via --timestamp to confirm it",
z.I64("in_progress_timestamp", msg.Timestamp.Unix()),
z.U64("in_progress_gas_limit", msg.GasLimit),
z.Str("max_skew", maxAdoptedTimestampSkew.String()))
}

return nil
}

// validateTimestamp checks that the provided timestamp is strictly greater than any existing
// registration timestamp (from the cluster lock or overrides file) for the requested validators.
func validateTimestamp(timestamp int64, pubkeys []string, cl cluster.Lock, overrides map[string]eth2v1.ValidatorRegistration) error {
Expand Down
163 changes: 162 additions & 1 deletion cmd/feerecipientsign_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ import (
"testing"
"time"

eth2v1 "github.com/attestantio/go-eth2-client/api/v1"
"github.com/stretchr/testify/require"

"github.com/obolnetwork/charon/app/log"
"github.com/obolnetwork/charon/app/obolapi"
"github.com/obolnetwork/charon/app/z"
"github.com/obolnetwork/charon/cluster"
"github.com/obolnetwork/charon/tbls"
Expand Down Expand Up @@ -226,7 +228,8 @@ func TestFeeRecipientSignAdoptsInProgressTimestamp(t *testing.T) {
validatorPubkey := c.lock.Validators[0].PublicKeyHex()
newFeeRecipient := "0x0000000000000000000000000000000000001234"

ts1 := time.Now().Add(time.Hour).Unix()
// ts1 is within maxAdoptedTimestampSkew so it can be adopted implicitly.
ts1 := time.Now().Add(time.Minute).Unix()
ts2 := time.Now().Add(2 * time.Hour).Unix()

// The first operator anchors the in-progress registration at ts1.
Expand All @@ -247,6 +250,164 @@ func TestFeeRecipientSignAdoptsInProgressTimestamp(t *testing.T) {
require.Equal(t, ts1, regs[0].V1.Message.Timestamp.Unix())
}

// TestFeeRecipientSignRefusesFutureInProgressTimestamp verifies that operators do not implicitly
// adopt an in-progress registration timestamp that is too far in the future, unless they confirm
// it by passing the same --timestamp.
func TestFeeRecipientSignRefusesFutureInProgressTimestamp(t *testing.T) {
ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name()))

c := setupFeeRecipientTestCluster(t, 1)
validatorPubkey := c.lock.Validators[0].PublicKeyHex()
newFeeRecipient := "0x0000000000000000000000000000000000001234"

future := time.Now().Add(time.Hour).Unix()

// The first operator anchors the in-progress registration far in the future.
require.NoError(t, runFeeRecipientSign(ctx, signConfigForOperator(c, 0, validatorPubkey, newFeeRecipient, future)))

tests := []struct {
name string
timestamp int64
expectedErr string
}{
{
name: "no explicit timestamp",
timestamp: 0,
expectedErr: "in-progress registration timestamp is too far in the future",
},
{
name: "different explicit timestamp",
timestamp: future + 60,
expectedErr: "in-progress registration timestamp is too far in the future",
},
{
name: "confirmed explicit timestamp",
timestamp: future,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
err := runFeeRecipientSign(ctx, signConfigForOperator(c, 1, validatorPubkey, newFeeRecipient, test.timestamp))
if test.expectedErr != "" {
require.ErrorContains(t, err, test.expectedErr)
} else {
require.NoError(t, err)
}
})
}
}

func TestCheckAdoptableRegistration(t *testing.T) {
now := time.Now()

tests := []struct {
name string
msg *eth2v1.ValidatorRegistration
explicitTS int64
expectedErr string
}{
{
name: "valid",
msg: &eth2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now},
},
{
name: "slightly in the future",
msg: &eth2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now.Add(time.Minute)},
},
{
name: "nil message",
expectedErr: "no message",
},
{
name: "zero gas limit",
msg: &eth2v1.ValidatorRegistration{GasLimit: 0, Timestamp: now},
expectedErr: "gas limit 0",
},
{
name: "zero gas limit with confirmed timestamp",
msg: &eth2v1.ValidatorRegistration{GasLimit: 0, Timestamp: now.Add(time.Hour)},
explicitTS: now.Add(time.Hour).Unix(),
expectedErr: "gas limit 0",
},
{
name: "far future",
msg: &eth2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now.Add(time.Hour)},
expectedErr: "too far in the future",
},
{
name: "far future with different explicit timestamp",
msg: &eth2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now.Add(time.Hour)},
explicitTS: now.Unix(),
expectedErr: "too far in the future",
},
{
name: "far future confirmed",
msg: &eth2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: time.Unix(now.Add(time.Hour).Unix(), 0)},
explicitTS: now.Add(time.Hour).Unix(),
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
err := checkAdoptableRegistration(test.msg, test.explicitTS)
if test.expectedErr != "" {
require.ErrorContains(t, err, test.expectedErr)
} else {
require.NoError(t, err)
}
})
}
}

// TestFilterPubkeysByStatusZeroGasLimit verifies that a zero resolved gas limit (e.g. a pre-v1.7
// lock without builder registrations and no overrides) is rejected unless --gas-limit is provided.
func TestFilterPubkeysByStatusZeroGasLimit(t *testing.T) {
ctx := t.Context()

c := setupFeeRecipientTestCluster(t, 1)
validatorPubkey := c.lock.Validators[0].PublicKeyHex()

// Simulate a lock without builder registrations.
lock := c.lock
lock.Validators = append([]cluster.DistValidator(nil), c.lock.Validators...)
lock.Validators[0].BuilderRegistration = cluster.BuilderRegistration{}

oAPI, err := obolapi.New(c.srvURL)
require.NoError(t, err)

tests := []struct {
name string
gasLimit uint64
expectedErr string
}{
{
name: "no gas limit resolved",
gasLimit: 0,
expectedErr: "could not determine gas limit",
},
{
name: "explicit gas limit",
gasLimit: 36000000,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
res, err := filterPubkeysByStatus(ctx, oAPI, c.lock.LockHash, []string{validatorPubkey},
"0x0000000000000000000000000000000000001234", test.gasLimit, 0, lock, nil, time.Now)
if test.expectedErr != "" {
require.ErrorContains(t, err, test.expectedErr)
return
}

require.NoError(t, err)
require.Len(t, res, 1)
require.Equal(t, test.gasLimit, res[0].GasLimit)
})
}
}

func TestFeeRecipientSignInvalidLockFile(t *testing.T) {
config := feerecipientSignConfig{
PrivateKeyPath: "nonexistent",
Expand Down
Loading