diff --git a/cmd/feerecipientsign.go b/cmd/feerecipientsign.go index e769e48ba..7f1b074bf 100644 --- a/cmd/feerecipientsign.go +++ b/cmd/feerecipientsign.go @@ -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, @@ -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. @@ -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 { diff --git a/cmd/feerecipientsign_internal_test.go b/cmd/feerecipientsign_internal_test.go index fe44b0618..42fc88d20 100644 --- a/cmd/feerecipientsign_internal_test.go +++ b/cmd/feerecipientsign_internal_test.go @@ -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" @@ -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. @@ -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: ð2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now}, + }, + { + name: "slightly in the future", + msg: ð2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now.Add(time.Minute)}, + }, + { + name: "nil message", + expectedErr: "no message", + }, + { + name: "zero gas limit", + msg: ð2v1.ValidatorRegistration{GasLimit: 0, Timestamp: now}, + expectedErr: "gas limit 0", + }, + { + name: "zero gas limit with confirmed timestamp", + msg: ð2v1.ValidatorRegistration{GasLimit: 0, Timestamp: now.Add(time.Hour)}, + explicitTS: now.Add(time.Hour).Unix(), + expectedErr: "gas limit 0", + }, + { + name: "far future", + msg: ð2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now.Add(time.Hour)}, + expectedErr: "too far in the future", + }, + { + name: "far future with different explicit timestamp", + msg: ð2v1.ValidatorRegistration{GasLimit: 30000000, Timestamp: now.Add(time.Hour)}, + explicitTS: now.Unix(), + expectedErr: "too far in the future", + }, + { + name: "far future confirmed", + msg: ð2v1.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",