diff --git a/app/builderregistration.go b/app/builderregistration.go index be2bff57e..b83552c9c 100644 --- a/app/builderregistration.go +++ b/app/builderregistration.go @@ -3,12 +3,14 @@ package app import ( + "bytes" "context" "encoding/hex" "encoding/json" "maps" "os" "path/filepath" + "slices" "strings" "sync" "time" @@ -344,21 +346,7 @@ func (s *builderRegistrationService) fetchFromAPI(ctx context.Context) (bool, er } if len(pv.AggregatedRegs) > 0 { - // Verify signatures on aggregated registrations. - if s.forkVersion != (eth2p0.Version{}) { - var verified []*eth2api.VersionedSignedValidatorRegistration - - for _, reg := range pv.AggregatedRegs { - if err := verifyRegistrationSignature(reg, s.forkVersion); err != nil { - log.Warn(ctx, "Skipping fetched builder registration with invalid signature", err) - continue - } - - verified = append(verified, reg) - } - - pv.AggregatedRegs = verified - } + pv.AggregatedRegs = filterVerifiedRegistrations(ctx, pv.AggregatedRegs, s.forkVersion) log.Info(ctx, "Fetched builder registrations from Obol API", z.Int("fully_signed", len(pv.AggregatedRegs)), @@ -394,7 +382,7 @@ func (s *builderRegistrationService) recompute(ctx context.Context) { } // mergeOverrides combines two override slices, keeping the entry with the highest -// timestamp per pubkey. +// timestamp per pubkey. Entries in a win ties. func mergeOverrides(a, b []*eth2api.VersionedSignedValidatorRegistration) []*eth2api.VersionedSignedValidatorRegistration { if len(a) == 0 { return b @@ -404,41 +392,124 @@ func mergeOverrides(a, b []*eth2api.VersionedSignedValidatorRegistration) []*eth return a } - byPubkey := make(map[string]*eth2api.VersionedSignedValidatorRegistration) + return mergeRegistrations(a, b, nil) +} + +// mergeRegistrations merges incoming registrations into base, keeping the entry with the +// newest timestamp per validator pubkey. Base entries win ties. Nil or incomplete entries +// are dropped. The result is sorted by pubkey for deterministic output. When an incoming +// entry loses to a base entry, onStale (if not nil) is called with both. +func mergeRegistrations( + base, incoming []*eth2api.VersionedSignedValidatorRegistration, + onStale func(kept, dropped *eth2api.VersionedSignedValidatorRegistration), +) []*eth2api.VersionedSignedValidatorRegistration { + byPubkey := make(map[string]*eth2api.VersionedSignedValidatorRegistration, len(base)+len(incoming)) - for _, reg := range a { + for _, reg := range base { if reg == nil || reg.V1 == nil || reg.V1.Message == nil { continue } - key := strings.ToLower(hex.EncodeToString(reg.V1.Message.Pubkey[:])) - byPubkey[key] = reg + byPubkey[string(reg.V1.Message.Pubkey[:])] = reg } - for _, reg := range b { + for _, reg := range incoming { if reg == nil || reg.V1 == nil || reg.V1.Message == nil { continue } - key := strings.ToLower(hex.EncodeToString(reg.V1.Message.Pubkey[:])) + key := string(reg.V1.Message.Pubkey[:]) + + prev, ok := byPubkey[key] + if ok && !reg.V1.Message.Timestamp.After(prev.V1.Message.Timestamp) { + if onStale != nil { + onStale(prev, reg) + } - existing, ok := byPubkey[key] - if !ok || existing.V1 == nil || existing.V1.Message == nil || reg.V1.Message.Timestamp.After(existing.V1.Message.Timestamp) { - byPubkey[key] = reg + continue } + + byPubkey[key] = reg } - result := make([]*eth2api.VersionedSignedValidatorRegistration, 0, len(byPubkey)) + merged := make([]*eth2api.VersionedSignedValidatorRegistration, 0, len(byPubkey)) for _, reg := range byPubkey { - result = append(result, reg) + merged = append(merged, reg) } - return result + slices.SortFunc(merged, func(a, b *eth2api.VersionedSignedValidatorRegistration) int { + return bytes.Compare(a.V1.Message.Pubkey[:], b.V1.Message.Pubkey[:]) + }) + + return merged +} + +// filterVerifiedRegistrations returns the registrations whose BLS signature verifies, +// logging and skipping the ones that fail. +func filterVerifiedRegistrations( + ctx context.Context, + regs []*eth2api.VersionedSignedValidatorRegistration, + forkVersion eth2p0.Version, +) []*eth2api.VersionedSignedValidatorRegistration { + verified := make([]*eth2api.VersionedSignedValidatorRegistration, 0, len(regs)) + + for _, reg := range regs { + if err := verifyRegistrationSignature(reg, forkVersion); err != nil { + log.Warn(ctx, "Skipping builder registration with invalid signature", err) + continue + } + + verified = append(verified, reg) + } + + return verified +} + +// MergeBuilderRegistrationOverrides merges fetched builder registrations into the existing +// overrides at path and returns the merged set, sorted by validator pubkey. The existing file +// is loaded leniently: an unreadable or invalid file is logged and treated as empty, and +// entries failing signature verification are logged and dropped, so a broken file is rebuilt +// rather than blocking updates. Fetched registrations failing verification are likewise +// logged and skipped. When both sources contain an entry for the same validator, the newest +// timestamp wins; existing entries win ties. +func MergeBuilderRegistrationOverrides( + ctx context.Context, + path string, + forkVersion eth2p0.Version, + fetched []*eth2api.VersionedSignedValidatorRegistration, +) []*eth2api.VersionedSignedValidatorRegistration { + var existing []*eth2api.VersionedSignedValidatorRegistration + + data, err := os.ReadFile(path) + if err != nil && !os.IsNotExist(err) { + log.Warn(ctx, "Failed to read existing builder registration overrides file; rebuilding it", err, z.Str("path", path)) + } else if err == nil { + if err := json.Unmarshal(data, &existing); err != nil { + log.Warn(ctx, "Existing builder registration overrides file is invalid; rebuilding it", err, z.Str("path", path)) + + existing = nil + } + } + + existing = filterVerifiedRegistrations(ctx, existing, forkVersion) + verified := filterVerifiedRegistrations(ctx, fetched, forkVersion) + + return mergeRegistrations(existing, verified, func(kept, dropped *eth2api.VersionedSignedValidatorRegistration) { + if kept.V1.Message.FeeRecipient == dropped.V1.Message.FeeRecipient && kept.V1.Message.GasLimit == dropped.V1.Message.GasLimit { + return // Re-fetch of an already applied registration, nothing to report. + } + + log.Warn(ctx, "Fetched builder registration is not newer than existing override, keeping existing", nil, + z.Str("pubkey", hex.EncodeToString(kept.V1.Message.Pubkey[:])), + z.I64("fetched_timestamp", dropped.V1.Message.Timestamp.Unix()), + z.I64("existing_timestamp", kept.V1.Message.Timestamp.Unix()), + ) + }) } // LoadBuilderRegistrationOverrides reads builder registration overrides from the given JSON file. -// It returns nil if the file does not exist. When forkVersion is non-zero, each registration's -// BLS signature is verified against the validator pubkey embedded in the message. +// It returns nil if the file does not exist. Each registration's BLS signature is verified +// against the validator pubkey embedded in the message. func LoadBuilderRegistrationOverrides(path string, forkVersion eth2p0.Version) ([]*eth2api.VersionedSignedValidatorRegistration, error) { data, err := os.ReadFile(path) if os.IsNotExist(err) { @@ -452,11 +523,9 @@ func LoadBuilderRegistrationOverrides(path string, forkVersion eth2p0.Version) ( return nil, errors.Wrap(err, "unmarshal builder registration overrides file", z.Str("path", path)) } - if forkVersion != (eth2p0.Version{}) { - for _, reg := range regs { - if err := verifyRegistrationSignature(reg, forkVersion); err != nil { - return nil, err - } + for _, reg := range regs { + if err := verifyRegistrationSignature(reg, forkVersion); err != nil { + return nil, err } } diff --git a/app/builderregistration_internal_test.go b/app/builderregistration_internal_test.go index 2097e0bee..49e9eb1d1 100644 --- a/app/builderregistration_internal_test.go +++ b/app/builderregistration_internal_test.go @@ -3,6 +3,7 @@ package app import ( + "bytes" "context" "encoding/json" "maps" @@ -21,7 +22,6 @@ import ( "github.com/obolnetwork/charon/cluster" "github.com/obolnetwork/charon/core" - "github.com/obolnetwork/charon/eth2util" "github.com/obolnetwork/charon/eth2util/registration" "github.com/obolnetwork/charon/tbls" "github.com/obolnetwork/charon/tbls/tblsconv" @@ -42,16 +42,16 @@ func TestLoadBuilderRegistrationOverrides(t *testing.T) { require.ErrorContains(t, err, "unmarshal builder registration overrides file") }) - t.Run("valid without verification", func(t *testing.T) { + t.Run("unsigned override fails verification", func(t *testing.T) { regs := []*eth2api.VersionedSignedValidatorRegistration{ makeUnsignedOverride(t), } path := writeOverridesFile(t, regs) - loaded, err := LoadBuilderRegistrationOverrides(path, eth2p0.Version{}) - require.NoError(t, err) - require.Len(t, loaded, 1) + // A zero fork version is mainnet's genesis fork version and must not disable verification. + _, err := LoadBuilderRegistrationOverrides(path, eth2p0.Version{}) + require.Error(t, err) }) t.Run("valid with signature verification", func(t *testing.T) { @@ -77,6 +77,100 @@ func TestLoadBuilderRegistrationOverrides(t *testing.T) { }) } +func TestMergeBuilderRegistrationOverrides(t *testing.T) { + ctx := t.Context() + + lock, sign := makeRegistrationSigner(t) + forkVersion := eth2p0.Version(lock.ForkVersion) + + feeA := bellatrix.ExecutionAddress{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xaa} + feeB := bellatrix.ExecutionAddress{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xbb} + + // Truncate to seconds so timestamps survive the JSON round trip unchanged. + baseTime := time.Now().Truncate(time.Second) + + reg0 := sign(0, feeA, baseTime) + reg1 := sign(1, feeA, baseTime) + + regsOf := func(regs ...*eth2api.VersionedSignedValidatorRegistration) []*eth2api.VersionedSignedValidatorRegistration { + return regs + } + + t.Run("no existing file", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "missing.json") + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, regsOf(reg0)) + require.Len(t, merged, 1) + require.Equal(t, reg0, merged[0]) + }) + + t.Run("disjoint pubkeys merged and sorted", func(t *testing.T) { + path := writeOverridesFile(t, regsOf(reg1)) + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, regsOf(reg0)) + require.Len(t, merged, 2) + require.Negative(t, bytes.Compare(merged[0].V1.Message.Pubkey[:], merged[1].V1.Message.Pubkey[:])) + }) + + t.Run("newer fetched wins", func(t *testing.T) { + path := writeOverridesFile(t, regsOf(reg0)) + newer := sign(0, feeB, baseTime.Add(time.Hour)) + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, regsOf(newer)) + require.Len(t, merged, 1) + require.Equal(t, feeB, merged[0].V1.Message.FeeRecipient) + }) + + t.Run("older fetched keeps existing", func(t *testing.T) { + path := writeOverridesFile(t, regsOf(reg0)) + older := sign(0, feeB, baseTime.Add(-time.Hour)) + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, regsOf(older)) + require.Len(t, merged, 1) + require.Equal(t, feeA, merged[0].V1.Message.FeeRecipient) + }) + + t.Run("equal timestamp keeps existing", func(t *testing.T) { + path := writeOverridesFile(t, regsOf(reg0)) + equal := sign(0, feeB, baseTime) + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, regsOf(equal)) + require.Len(t, merged, 1) + require.Equal(t, feeA, merged[0].V1.Message.FeeRecipient) + }) + + t.Run("invalid fetched skipped", func(t *testing.T) { + path := writeOverridesFile(t, regsOf(reg0)) + + bad := sign(1, feeB, baseTime) + bad.V1.Signature[0] ^= 0xff + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, regsOf(bad)) + require.Len(t, merged, 1) + require.Equal(t, reg0.V1.Message.Pubkey, merged[0].V1.Message.Pubkey) + }) + + t.Run("corrupt existing file rebuilt", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "overrides.json") + require.NoError(t, os.WriteFile(path, []byte("{corrupt"), 0o644)) + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, regsOf(reg0)) + require.Len(t, merged, 1) + require.Equal(t, reg0.V1.Message.Pubkey, merged[0].V1.Message.Pubkey) + }) + + t.Run("invalid existing entry dropped", func(t *testing.T) { + badExisting := sign(1, feeA, baseTime) + badExisting.V1.Signature[0] ^= 0xff + + path := writeOverridesFile(t, regsOf(reg0, badExisting)) + + merged := MergeBuilderRegistrationOverrides(ctx, path, forkVersion, nil) + require.Len(t, merged, 1) + require.Equal(t, reg0.V1.Message.Pubkey, merged[0].V1.Message.Pubkey) + }) +} + func Test_applyBuilderRegistrationOverrides(t *testing.T) { lock, _ := makeSignedOverrides(t) ctx := t.Context() @@ -280,19 +374,15 @@ func TestBuilderRegistrationService(t *testing.T) { }) } -// makeSignedOverrides creates a test cluster lock and properly signed builder registration overrides. -func makeSignedOverrides(t *testing.T) (cluster.Lock, []*eth2api.VersionedSignedValidatorRegistration) { +// makeRegistrationSigner creates a test cluster lock and a signer producing properly signed +// builder registrations for its validators. +func makeRegistrationSigner(t *testing.T) (cluster.Lock, func(valIdx int, feeRecipient bellatrix.ExecutionAddress, timestamp time.Time) *eth2api.VersionedSignedValidatorRegistration) { t.Helper() random := rand.New(rand.NewSource(0)) lock, _, keyShares := cluster.NewForT(t, 2, 4, 4, 0, random) - forkVersion, err := eth2util.NetworkToForkVersionBytes(eth2util.Goerli.Name) - require.NoError(t, err) - - var overrides []*eth2api.VersionedSignedValidatorRegistration - - for valIdx, val := range lock.Validators { + sign := func(valIdx int, feeRecipient bellatrix.ExecutionAddress, timestamp time.Time) *eth2api.VersionedSignedValidatorRegistration { // Reconstruct root secret from shares. sharesMap := make(map[int]tbls.PrivateKey) for i, share := range keyShares[valIdx] { @@ -302,31 +392,45 @@ func makeSignedOverrides(t *testing.T) (cluster.Lock, []*eth2api.VersionedSigned rootSecret, err := tbls.RecoverSecret(sharesMap, uint(len(keyShares[valIdx])), uint(lock.Threshold)) require.NoError(t, err) - pubkey, err := tblsconv.PubkeyToETH2(tbls.PublicKey(val.PubKey)) + pubkey, err := tblsconv.PubkeyToETH2(tbls.PublicKey(lock.Validators[valIdx].PubKey)) require.NoError(t, err) - feeRecipient := bellatrix.ExecutionAddress{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x99, byte(valIdx)} - msg := ð2v1.ValidatorRegistration{ FeeRecipient: feeRecipient, GasLimit: registration.DefaultGasLimit, - Timestamp: time.Now().Add(time.Hour), + Timestamp: timestamp, Pubkey: pubkey, } - sigRoot, err := registration.GetMessageSigningRoot(msg, eth2p0.Version(forkVersion)) + sigRoot, err := registration.GetMessageSigningRoot(msg, eth2p0.Version(lock.ForkVersion)) require.NoError(t, err) sig, err := tbls.Sign(rootSecret, sigRoot[:]) require.NoError(t, err) - overrides = append(overrides, ð2api.VersionedSignedValidatorRegistration{ + return ð2api.VersionedSignedValidatorRegistration{ Version: eth2spec.BuilderVersionV1, V1: ð2v1.SignedValidatorRegistration{ Message: msg, Signature: eth2p0.BLSSignature(sig), }, - }) + } + } + + return lock, sign +} + +// makeSignedOverrides creates a test cluster lock and properly signed builder registration overrides. +func makeSignedOverrides(t *testing.T) (cluster.Lock, []*eth2api.VersionedSignedValidatorRegistration) { + t.Helper() + + lock, sign := makeRegistrationSigner(t) + + var overrides []*eth2api.VersionedSignedValidatorRegistration + + for valIdx := range lock.Validators { + feeRecipient := bellatrix.ExecutionAddress{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x99, byte(valIdx)} + overrides = append(overrides, sign(valIdx, feeRecipient, time.Now().Add(time.Hour))) } return lock, overrides diff --git a/cmd/depositfetch_internal_test.go b/cmd/depositfetch_internal_test.go index 7ba491a50..66c93a580 100644 --- a/cmd/depositfetch_internal_test.go +++ b/cmd/depositfetch_internal_test.go @@ -51,7 +51,7 @@ func TestDepositFetchValid(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) diff --git a/cmd/depositsign_internal_test.go b/cmd/depositsign_internal_test.go index 7d68a5215..fc821b164 100644 --- a/cmd/depositsign_internal_test.go +++ b/cmd/depositsign_internal_test.go @@ -51,7 +51,7 @@ func TestDepositSignValid(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) diff --git a/cmd/exit_broadcast_internal_test.go b/cmd/exit_broadcast_internal_test.go index 1f074e822..c48e434ff 100644 --- a/cmd/exit_broadcast_internal_test.go +++ b/cmd/exit_broadcast_internal_test.go @@ -117,7 +117,7 @@ func testRunBcastFullExitCmdFlow(t *testing.T, fromFile bool, all bool) { defer srv.Close() - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) for idx := range operatorAmt { baseDir := filepath.Join(root, fmt.Sprintf("op%d", idx)) @@ -285,7 +285,7 @@ func Test_runBcastFullExitCmd_Config(t *testing.T) { mBytes, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) for opIdx := range operatorAmt { del(t, test, root, opIdx) @@ -494,7 +494,7 @@ func TestExitBcastFullExitNotActivated(t *testing.T) { defer srv.Close() - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) for idxOp := range operatorAmt { // submit partial exits only for a subset diff --git a/cmd/exit_delete_internal_test.go b/cmd/exit_delete_internal_test.go index e6921ca81..1a63db014 100644 --- a/cmd/exit_delete_internal_test.go +++ b/cmd/exit_delete_internal_test.go @@ -98,7 +98,7 @@ func testRunDeleteExitFullFlow(t *testing.T, all bool) { defer srv.Close() - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) idx := 1 baseDir := filepath.Join(root, fmt.Sprintf("op%d", idx)) diff --git a/cmd/exit_fetch_internal_test.go b/cmd/exit_fetch_internal_test.go index ecc82f84b..4aa0271bc 100644 --- a/cmd/exit_fetch_internal_test.go +++ b/cmd/exit_fetch_internal_test.go @@ -100,7 +100,7 @@ func testRunFetchExitFullFlow(t *testing.T, all bool) { defer srv.Close() - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) for idx := range operatorAmt { baseDir := filepath.Join(root, fmt.Sprintf("op%d", idx)) @@ -315,7 +315,7 @@ func TestFetchExitFullFlowNotActivated(t *testing.T) { defer srv.Close() - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) for idxOp := range operatorAmt { // submit partial exits only for a subset diff --git a/cmd/exit_list_internal_test.go b/cmd/exit_list_internal_test.go index d2b5e3833..52b81f7ec 100644 --- a/cmd/exit_list_internal_test.go +++ b/cmd/exit_list_internal_test.go @@ -51,7 +51,7 @@ func Test_runListActiveVals(t *testing.T) { mBytes, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) validatorSet := beaconmock.ValidatorSet{} @@ -118,7 +118,7 @@ func Test_listActiveVals(t *testing.T) { mBytes, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) t.Run("all validators in the cluster are active", func(t *testing.T) { validatorSet := beaconmock.ValidatorSet{} diff --git a/cmd/exit_sign_internal_test.go b/cmd/exit_sign_internal_test.go index b36f34170..b84eefdd6 100644 --- a/cmd/exit_sign_internal_test.go +++ b/cmd/exit_sign_internal_test.go @@ -28,18 +28,16 @@ import ( "github.com/obolnetwork/charon/testutil/obolapimock" ) -//nolint:unparam // we mostly pass "4" for operatorAmt but we might change it later. func writeAllLockData( t *testing.T, root string, - operatorAmt int, enrs []*k1.PrivateKey, operatorShares [][]tbls.PrivateKey, manifestBytes []byte, ) { t.Helper() - for opIdx := range operatorAmt { + for opIdx := range enrs { opID := fmt.Sprintf("op%d", opIdx) oDir := filepath.Join(root, opID) keysDir := filepath.Join(oDir, "validator_keys") @@ -196,7 +194,7 @@ func runSubmitPartialExitFlowTest(t *testing.T, useValIdx bool, skipBeaconNodeCh defer srv.Close() - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) baseDir := filepath.Join(root, fmt.Sprintf("op%d", 0)) @@ -337,7 +335,7 @@ func Test_runSubmitPartialExit_Config(t *testing.T) { mBytes, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, mBytes) + writeAllLockData(t, root, enrs, operatorShares, mBytes) for opIdx := range operatorAmt { del(t, test, root, opIdx) diff --git a/cmd/feerecipientfetch.go b/cmd/feerecipientfetch.go index fe2d59638..a1bc37388 100644 --- a/cmd/feerecipientfetch.go +++ b/cmd/feerecipientfetch.go @@ -9,6 +9,7 @@ import ( "path/filepath" eth2api "github.com/attestantio/go-eth2-client/api" + eth2p0 "github.com/attestantio/go-eth2-client/spec/phase0" "github.com/spf13/cobra" "github.com/obolnetwork/charon/app" @@ -127,32 +128,57 @@ func runFeeRecipientFetch(ctx context.Context, config feerecipientFetchConfig) e return nil } - err = writeSignedValidatorRegistrations(config.OverridesFilePath, pv.AggregatedRegs) + mergedRegs := app.MergeBuilderRegistrationOverrides(ctx, config.OverridesFilePath, eth2p0.Version(cl.ForkVersion), pv.AggregatedRegs) + + err = writeSignedValidatorRegistrations(config.OverridesFilePath, mergedRegs) if err != nil { return errors.Wrap(err, "write builder registrations overrides", z.Str("path", config.OverridesFilePath)) } log.Info(ctx, "Successfully wrote builder registrations overrides", - z.Int("total", len(pv.AggregatedRegs)), + z.Int("total", len(mergedRegs)), + z.Int("fetched", len(pv.AggregatedRegs)), z.Str("path", config.OverridesFilePath), ) return nil } +// writeSignedValidatorRegistrations writes the registrations to filename atomically +// (temp file + rename), so a crash mid-write cannot leave a truncated file behind. func writeSignedValidatorRegistrations(filename string, regs []*eth2api.VersionedSignedValidatorRegistration) error { data, err := json.MarshalIndent(regs, "", " ") if err != nil { return errors.Wrap(err, "marshal registrations to JSON") } - if err := os.MkdirAll(filepath.Dir(filename), 0o755); err != nil { + dir := filepath.Dir(filename) + if err := os.MkdirAll(dir, 0o755); err != nil { return errors.Wrap(err, "create output directory") } - err = os.WriteFile(filename, data, 0o644) //nolint:gosec // G306: world-readable output file is intentional + tmp, err := os.CreateTemp(dir, filepath.Base(filename)+".tmp") if err != nil { - return errors.Wrap(err, "write registrations overrides file") + return errors.Wrap(err, "create temporary registrations overrides file") + } + defer os.Remove(tmp.Name()) // No-op after successful rename. + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return errors.Wrap(err, "write temporary registrations overrides file") + } + + if err := tmp.Chmod(0o644); err != nil { + tmp.Close() + return errors.Wrap(err, "chmod temporary registrations overrides file") + } + + if err := tmp.Close(); err != nil { + return errors.Wrap(err, "close temporary registrations overrides file") + } + + if err := os.Rename(tmp.Name(), filename); err != nil { + return errors.Wrap(err, "rename temporary registrations overrides file") } return nil diff --git a/cmd/feerecipientfetch_internal_test.go b/cmd/feerecipientfetch_internal_test.go index 9eb93c9cf..5d81d331f 100644 --- a/cmd/feerecipientfetch_internal_test.go +++ b/cmd/feerecipientfetch_internal_test.go @@ -3,6 +3,8 @@ package cmd import ( + "context" + "encoding/hex" "encoding/json" "fmt" "math/rand" @@ -10,11 +12,15 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "time" + eth2api "github.com/attestantio/go-eth2-client/api" + eth2p0 "github.com/attestantio/go-eth2-client/spec/phase0" "github.com/stretchr/testify/require" + "github.com/obolnetwork/charon/app" "github.com/obolnetwork/charon/app/log" "github.com/obolnetwork/charon/app/z" "github.com/obolnetwork/charon/cluster" @@ -22,28 +28,26 @@ import ( "github.com/obolnetwork/charon/testutil/obolapimock" ) -func TestFeeRecipientFetchValid(t *testing.T) { - ctx := t.Context() - ctx = log.WithCtx(ctx, z.Str("test_case", t.Name())) +// feeRecipientTestCluster holds a test cluster with lock data written to disk and a +// mock Obol API server, shared by the feerecipient command tests. +type feeRecipientTestCluster struct { + lock cluster.Lock + root string + srvURL string +} - valAmt := 4 - operatorAmt := 4 +func setupFeeRecipientTestCluster(t *testing.T, valAmt int) feeRecipientTestCluster { + t.Helper() + + const operatorAmt = 4 random := rand.New(rand.NewSource(0)) - lock, enrs, keyShares := cluster.NewForT( - t, - valAmt, - operatorAmt, - operatorAmt, - 0, - random, - ) + lock, enrs, keyShares := cluster.NewForT(t, valAmt, operatorAmt, operatorAmt, 0, random) root := t.TempDir() operatorShares := make([][]tbls.PrivateKey, operatorAmt) - for opIdx := range operatorAmt { for _, share := range keyShares { operatorShares[opIdx] = append(operatorShares[opIdx], share[opIdx]) @@ -53,56 +57,232 @@ func TestFeeRecipientFetchValid(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) srv := httptest.NewServer(handler) - defer srv.Close() + t.Cleanup(srv.Close) addLockFiles(lock) - // First, submit partial signatures from threshold operators. - newFeeRecipient := "0x0000000000000000000000000000000000001234" - validatorPubkey := lock.Validators[0].PublicKeyHex() + return feeRecipientTestCluster{lock: lock, root: root, srvURL: srv.URL} +} - for opIdx := range lock.Threshold { - baseDir := filepath.Join(root, fmt.Sprintf("op%d", opIdx)) +// signConfigForOperator returns a sign config for the given operator index. +func signConfigForOperator(c feeRecipientTestCluster, opIdx int, validatorPubkey, feeRecipient string, timestamp int64) feerecipientSignConfig { + baseDir := filepath.Join(c.root, fmt.Sprintf("op%d", opIdx)) - signConfig := feerecipientSignConfig{ - feerecipientConfig: feerecipientConfig{ - ValidatorPublicKeys: []string{validatorPubkey}, - PrivateKeyPath: filepath.Join(baseDir, "charon-enr-private-key"), - LockFilePath: filepath.Join(baseDir, "cluster-lock.json"), - PublishAddress: srv.URL, - PublishTimeout: 10 * time.Second, - }, - ValidatorKeysDir: filepath.Join(baseDir, "validator_keys"), - FeeRecipient: newFeeRecipient, - } + return feerecipientSignConfig{ + feerecipientConfig: feerecipientConfig{ + ValidatorPublicKeys: []string{validatorPubkey}, + PrivateKeyPath: filepath.Join(baseDir, "charon-enr-private-key"), + LockFilePath: filepath.Join(baseDir, "cluster-lock.json"), + PublishAddress: c.srvURL, + PublishTimeout: 10 * time.Second, + }, + ValidatorKeysDir: filepath.Join(baseDir, "validator_keys"), + FeeRecipient: feeRecipient, + Timestamp: timestamp, + } +} + +// signFeeRecipientThreshold submits partial signatures from a threshold of operators. +func signFeeRecipientThreshold(ctx context.Context, t *testing.T, c feeRecipientTestCluster, validatorPubkey, feeRecipient string, timestamp int64) { + t.Helper() + for opIdx := range c.lock.Threshold { + signConfig := signConfigForOperator(c, opIdx, validatorPubkey, feeRecipient, timestamp) require.NoError(t, runFeeRecipientSign(ctx, signConfig), "operator %d submit feerecipient sign", opIdx) } +} - // Now fetch the aggregated registrations. - overridesFile := filepath.Join(root, "output", "builder_registrations_overrides.json") - require.NoError(t, os.MkdirAll(filepath.Dir(overridesFile), 0o755)) +// fetchFeeRecipient runs the fetch command for the given validators into overridesFile. +func fetchFeeRecipient(ctx context.Context, t *testing.T, c feeRecipientTestCluster, validatorPubkeys []string, overridesFile string) { + t.Helper() fetchConfig := feerecipientFetchConfig{ feerecipientConfig: feerecipientConfig{ - LockFilePath: filepath.Join(root, "op0", "cluster-lock.json"), - OverridesFilePath: overridesFile, - PublishAddress: srv.URL, - PublishTimeout: 10 * time.Second, + ValidatorPublicKeys: validatorPubkeys, + LockFilePath: filepath.Join(c.root, "op0", "cluster-lock.json"), + OverridesFilePath: overridesFile, + PublishAddress: c.srvURL, + PublishTimeout: 10 * time.Second, }, } require.NoError(t, runFeeRecipientFetch(ctx, fetchConfig)) +} - // Verify output file exists and contains registrations. - data, err := os.ReadFile(overridesFile) +// readOverridesFile reads and parses the overrides file, returning fee recipients keyed by +// 0x-prefixed lowercase validator pubkey hex. +func readOverridesFile(t *testing.T, path string) ([]*eth2api.VersionedSignedValidatorRegistration, map[string]string) { + t.Helper() + + data, err := os.ReadFile(path) require.NoError(t, err) - require.NotEmpty(t, data) + + var regs []*eth2api.VersionedSignedValidatorRegistration + require.NoError(t, json.Unmarshal(data, ®s)) + + byPubkey := make(map[string]string, len(regs)) + + for _, reg := range regs { + require.NotNil(t, reg) + require.NotNil(t, reg.V1) + require.NotNil(t, reg.V1.Message) + + pubkey := "0x" + hex.EncodeToString(reg.V1.Message.Pubkey[:]) + byPubkey[pubkey] = strings.ToLower(reg.V1.Message.FeeRecipient.String()) + } + + return regs, byPubkey +} + +func TestFeeRecipientFetchValid(t *testing.T) { + ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name())) + + c := setupFeeRecipientTestCluster(t, 4) + + newFeeRecipient := "0x0000000000000000000000000000000000001234" + validatorPubkey := c.lock.Validators[0].PublicKeyHex() + + signFeeRecipientThreshold(ctx, t, c, validatorPubkey, newFeeRecipient, 0) + + overridesFile := filepath.Join(c.root, "output", "builder_registrations_overrides.json") + fetchFeeRecipient(ctx, t, c, nil, overridesFile) + + regs, byPubkey := readOverridesFile(t, overridesFile) + require.Len(t, regs, 1) + require.Equal(t, newFeeRecipient, byPubkey[strings.ToLower(validatorPubkey)]) +} + +func TestFeeRecipientFetchMergesWithExistingOverrides(t *testing.T) { + ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name())) + + c := setupFeeRecipientTestCluster(t, 2) + + overridesFile := filepath.Join(c.root, "output", "builder_registrations_overrides.json") + validatorAPubkey := c.lock.Validators[0].PublicKeyHex() + validatorBPubkey := c.lock.Validators[1].PublicKeyHex() + + signFeeRecipientThreshold(ctx, t, c, validatorAPubkey, "0x0000000000000000000000000000000000001234", 0) + fetchFeeRecipient(ctx, t, c, []string{validatorAPubkey}, overridesFile) + + signFeeRecipientThreshold(ctx, t, c, validatorBPubkey, "0x0000000000000000000000000000000000005678", 0) + fetchFeeRecipient(ctx, t, c, []string{validatorBPubkey}, overridesFile) + + regs, byPubkey := readOverridesFile(t, overridesFile) + require.Len(t, regs, 2) + require.Equal(t, "0x0000000000000000000000000000000000001234", byPubkey[strings.ToLower(validatorAPubkey)]) + require.Equal(t, "0x0000000000000000000000000000000000005678", byPubkey[strings.ToLower(validatorBPubkey)]) +} + +// TestFeeRecipientFetchUpdatesSameValidator verifies the core update flow: re-signing the +// same validator with a new fee recipient and a later timestamp replaces the on-disk entry. +func TestFeeRecipientFetchUpdatesSameValidator(t *testing.T) { + ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name())) + + c := setupFeeRecipientTestCluster(t, 1) + + overridesFile := filepath.Join(c.root, "output", "builder_registrations_overrides.json") + validatorPubkey := c.lock.Validators[0].PublicKeyHex() + + ts1 := time.Now().Add(time.Hour).Unix() + ts2 := time.Now().Add(2 * time.Hour).Unix() + + signFeeRecipientThreshold(ctx, t, c, validatorPubkey, "0x0000000000000000000000000000000000001234", ts1) + fetchFeeRecipient(ctx, t, c, nil, overridesFile) + + regs, byPubkey := readOverridesFile(t, overridesFile) + require.Len(t, regs, 1) + require.Equal(t, "0x0000000000000000000000000000000000001234", byPubkey[strings.ToLower(validatorPubkey)]) + + signFeeRecipientThreshold(ctx, t, c, validatorPubkey, "0x0000000000000000000000000000000000005678", ts2) + fetchFeeRecipient(ctx, t, c, nil, overridesFile) + + regs, byPubkey = readOverridesFile(t, overridesFile) + require.Len(t, regs, 1) + require.Equal(t, "0x0000000000000000000000000000000000005678", byPubkey[strings.ToLower(validatorPubkey)]) + require.Equal(t, ts2, regs[0].V1.Message.Timestamp.Unix()) + + // A repeated fetch of the same data must be idempotent. + fetchFeeRecipient(ctx, t, c, nil, overridesFile) + + regs, byPubkey = readOverridesFile(t, overridesFile) + require.Len(t, regs, 1) + require.Equal(t, "0x0000000000000000000000000000000000005678", byPubkey[strings.ToLower(validatorPubkey)]) +} + +// TestFeeRecipientFetchSelfHealsCorruptOverrides verifies that a corrupt overrides file does +// not block fetching: the file is rebuilt from the fetched registrations. +func TestFeeRecipientFetchSelfHealsCorruptOverrides(t *testing.T) { + ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name())) + + c := setupFeeRecipientTestCluster(t, 1) + + overridesFile := filepath.Join(c.root, "output", "builder_registrations_overrides.json") + require.NoError(t, os.MkdirAll(filepath.Dir(overridesFile), 0o755)) + require.NoError(t, os.WriteFile(overridesFile, []byte("{corrupt"), 0o644)) + + newFeeRecipient := "0x0000000000000000000000000000000000001234" + validatorPubkey := c.lock.Validators[0].PublicKeyHex() + + signFeeRecipientThreshold(ctx, t, c, validatorPubkey, newFeeRecipient, 0) + fetchFeeRecipient(ctx, t, c, nil, overridesFile) + + regs, byPubkey := readOverridesFile(t, overridesFile) + require.Len(t, regs, 1) + require.Equal(t, newFeeRecipient, byPubkey[strings.ToLower(validatorPubkey)]) +} + +// TestFeeRecipientFetchSkipsInvalidSignature verifies that merging skips a registration with +// an invalid signature and still merges the remaining, valid ones, rather than aborting the +// whole batch. +func TestFeeRecipientFetchSkipsInvalidSignature(t *testing.T) { + ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name())) + + c := setupFeeRecipientTestCluster(t, 2) + + validatorAPubkey := c.lock.Validators[0].PublicKeyHex() + validatorBPubkey := c.lock.Validators[1].PublicKeyHex() + + signFeeRecipientThreshold(ctx, t, c, validatorAPubkey, "0x0000000000000000000000000000000000001234", 0) + signFeeRecipientThreshold(ctx, t, c, validatorBPubkey, "0x0000000000000000000000000000000000005678", 0) + + // Fetch each validator into its own file so we can extract the individually-signed, + // fully aggregated registrations before feeding them into the merge directly. + fetchInto := func(validatorPubkey, path string) *eth2api.VersionedSignedValidatorRegistration { + t.Helper() + + fetchFeeRecipient(ctx, t, c, []string{validatorPubkey}, path) + + regs, err := app.LoadBuilderRegistrationOverrides(path, eth2p0.Version(c.lock.ForkVersion)) + require.NoError(t, err) + require.Len(t, regs, 1) + + return regs[0] + } + + regA := fetchInto(validatorAPubkey, filepath.Join(c.root, "output-a", "overrides.json")) + regB := fetchInto(validatorBPubkey, filepath.Join(c.root, "output-b", "overrides.json")) + + // Corrupt validator B's signature to simulate a tampered/invalid fetched registration. + regB.V1.Signature[0] ^= 0xff + + targetFile := filepath.Join(c.root, "output", "builder_registrations_overrides.json") + + merged := app.MergeBuilderRegistrationOverrides(ctx, targetFile, eth2p0.Version(c.lock.ForkVersion), []*eth2api.VersionedSignedValidatorRegistration{regA, regB}) + require.Len(t, merged, 1, "invalid registration must be skipped, valid one must still be merged") + require.Equal(t, strings.ToLower(validatorAPubkey), "0x"+hex.EncodeToString(merged[0].V1.Message.Pubkey[:])) + + // If every fetched registration is invalid, the existing overrides file's entry for + // validator A must be preserved untouched rather than being wiped out or erroring. + require.NoError(t, writeSignedValidatorRegistrations(targetFile, merged)) + + mergedAfterAllInvalid := app.MergeBuilderRegistrationOverrides(ctx, targetFile, eth2p0.Version(c.lock.ForkVersion), []*eth2api.VersionedSignedValidatorRegistration{regB}) + require.Len(t, mergedAfterAllInvalid, 1, "existing valid override must survive a batch that is entirely invalid") + require.Equal(t, strings.ToLower(validatorAPubkey), "0x"+hex.EncodeToString(mergedAfterAllInvalid[0].V1.Message.Pubkey[:])) } func TestFeeRecipientFetchInvalidLockFile(t *testing.T) { @@ -121,46 +301,26 @@ func TestFeeRecipientFetchInvalidLockFile(t *testing.T) { func TestFeeRecipientFetchAPIUnreachable(t *testing.T) { ctx := t.Context() - valAmt := 1 - operatorAmt := 4 - - random := rand.New(rand.NewSource(0)) + c := setupFeeRecipientTestCluster(t, 1) - lock, enrs, keyShares := cluster.NewForT(t, valAmt, operatorAmt, operatorAmt, 0, random) - - root := t.TempDir() - - operatorShares := make([][]tbls.PrivateKey, operatorAmt) - for opIdx := range operatorAmt { - for _, share := range keyShares { - operatorShares[opIdx] = append(operatorShares[opIdx], share[opIdx]) - } - } - - lockJSON, err := json.Marshal(lock) - require.NoError(t, err) - - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) - - // Start and immediately close the server so the URL is unreachable. + // Start and immediately close a server so the URL is unreachable. srv := httptest.NewServer(http.NotFoundHandler()) srv.Close() config := feerecipientFetchConfig{ feerecipientConfig: feerecipientConfig{ - LockFilePath: filepath.Join(root, "op0", "cluster-lock.json"), + LockFilePath: filepath.Join(c.root, "op0", "cluster-lock.json"), PublishAddress: srv.URL, PublishTimeout: time.Second, }, } - err = runFeeRecipientFetch(ctx, config) + err := runFeeRecipientFetch(ctx, config) require.ErrorContains(t, err, "fetch builder registrations from Obol API") } func TestFeeRecipientFetchNoQuorum(t *testing.T) { - ctx := t.Context() - ctx = log.WithCtx(ctx, z.Str("test_case", t.Name())) + ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name())) valAmt := 1 operatorAmt := 4 @@ -181,7 +341,7 @@ func TestFeeRecipientFetchNoQuorum(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) // dropOnePsig=true causes the mock to drop one partial, preventing quorum. handler, addLockFiles := obolapimock.MockServer(true, nil) diff --git a/cmd/feerecipientlist_internal_test.go b/cmd/feerecipientlist_internal_test.go index 141de8e69..9d91ab891 100644 --- a/cmd/feerecipientlist_internal_test.go +++ b/cmd/feerecipientlist_internal_test.go @@ -446,7 +446,7 @@ func TestFeeRecipientListFetchesRemote(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) @@ -517,7 +517,7 @@ func TestFeeRecipientListRemoteOnlyTriggersSuggestion(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) @@ -587,7 +587,7 @@ func TestFeeRecipientListOverridesMatchRemote(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) @@ -708,7 +708,7 @@ func TestFeeRecipientListIncompleteNoQuorum(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) // dropOnePsig=true prevents quorum from being reached. handler, addLockFiles := obolapimock.MockServer(true, nil) diff --git a/cmd/feerecipientsign.go b/cmd/feerecipientsign.go index ddcb8bf14..7ef3f8084 100644 --- a/cmd/feerecipientsign.go +++ b/cmd/feerecipientsign.go @@ -66,7 +66,7 @@ func newFeeRecipientSignCmd(runFunc func(context.Context, feerecipientSignConfig cmd.Flags().StringVar(&config.ValidatorKeysDir, validatorKeysDir.String(), ".charon/validator_keys", "Path to the directory containing the validator private key share files and passwords.") cmd.Flags().StringSliceVar(&config.ValidatorPublicKeys, "validator-public-keys", nil, "[REQUIRED] Comma-separated list of validator public keys to sign builder registrations for.") cmd.Flags().StringVar(&config.FeeRecipient, "fee-recipient", "", "[REQUIRED] New fee recipient address to be applied to all specified validators.") - cmd.Flags().Uint64Var(&config.GasLimit, "gas-limit", 0, "Optional gas limit override for builder registrations. If not set, the existing gas limit from the cluster lock or overrides file is used.") + cmd.Flags().Uint64Var(&config.GasLimit, "gas-limit", 0, "Optional gas limit override for builder registrations. If not set, the most recent gas limit from the cluster lock, overrides file or remote API is used.") cmd.Flags().Int64Var(&config.Timestamp, "timestamp", 0, "Optional Unix timestamp for the builder registration message. When set, all operators can sign independently with the same timestamp. If not set, either the current time is used for new registrations or if another peer already submitted partial signature to the API, its timestamp is used.") wrapPreRunE(cmd, func(cmd *cobra.Command, _ []string) error { @@ -147,8 +147,8 @@ func findRegistrationGroups(v *obolapi.FeeRecipientValidator, feeRecipient strin } func runFeeRecipientSign(ctx context.Context, config feerecipientSignConfig) error { - if _, err := eth2util.ChecksumAddress(config.FeeRecipient); err != nil { - return errors.Wrap(err, "invalid fee recipient address", z.Str("fee_recipient", config.FeeRecipient)) + if err := validateFeeRecipient(config.FeeRecipient); err != nil { + return err } identityKey, err := k1util.Load(config.PrivateKeyPath) @@ -205,7 +205,7 @@ func runFeeRecipientSign(ctx context.Context, config feerecipientSignConfig) err nowFunc = func() time.Time { return time.Unix(config.Timestamp, 0) } } - pubkeysToSign, err := filterPubkeysByStatus(ctx, oAPI, cl.LockHash, config.ValidatorPublicKeys, config.FeeRecipient, config.GasLimit, *cl, overrides, nowFunc) + pubkeysToSign, err := filterPubkeysByStatus(ctx, oAPI, cl.LockHash, config.ValidatorPublicKeys, config.FeeRecipient, config.GasLimit, config.Timestamp, *cl, overrides, nowFunc) if err != nil { return err } @@ -246,8 +246,9 @@ func runFeeRecipientSign(ctx context.Context, config feerecipientSignConfig) err // use for signing. Validators with a quorum-complete registration for the requested fee recipient // are skipped. In-progress (non-quorum) registrations with a mismatched fee recipient cause an error. // For validators with a matching in-progress registration, the existing timestamp and gas limit are -// adopted so all operators sign the identical message. For unknown validators, now() and the -// gas limit from the config override or cluster lock are used. +// adopted so all operators sign the identical message, warning when that discards an explicit +// --timestamp or --gas-limit. For new registrations, the timestamp must be strictly later than the +// current quorum registration's, otherwise the result would never be applied. func filterPubkeysByStatus( ctx context.Context, oAPI obolapi.Client, @@ -255,6 +256,7 @@ func filterPubkeysByStatus( requestedPubkeys []string, feeRecipient string, gasLimitOverride uint64, + explicitTimestamp int64, cl cluster.Lock, overrides map[string]eth2v1.ValidatorRegistration, now func() time.Time, @@ -273,36 +275,65 @@ func filterPubkeysByStatus( v, ok := validatorByPubkey[normalizedKey] + // Find the first incomplete group whose fee recipient matches the requested one. + // Stale incompletes (different fee recipient) are ignored — they may linger on the + // API after quorum was reached for a previous fee recipient and must not block new + // fee recipient changes. + var quorumGroup, matchingIncomplete *obolapi.FeeRecipientBuilderRegistration + if ok { + quorumGroup, matchingIncomplete = findRegistrationGroups(&v, feeRecipient) + } + + var quorumMsg *eth2v1.ValidatorRegistration + if quorumGroup != nil { + quorumMsg = quorumGroup.Message + } + // Default: anchor new timestamp and resolve gas limit. // These will be overridden if there's a matching incomplete registration. timestamp := now() - gasLimit := resolveGasLimit(gasLimitOverride, cl, overrides, normalizedKey) + gasLimit := resolveGasLimit(gasLimitOverride, cl, overrides, normalizedKey, quorumMsg) - if ok { - // Find the first incomplete group whose fee recipient matches the requested one. - // Stale incompletes (different fee recipient) are ignored — they may linger on the - // API after quorum was reached for a previous fee recipient and must not block new - // fee recipient changes. - quorumGroup, matchingIncomplete := findRegistrationGroups(&v, feeRecipient) - - if quorumGroup != nil && strings.EqualFold(quorumGroup.Message.FeeRecipient.String(), feeRecipient) { - log.Info(ctx, "Validator already has a complete builder registration, skipping", + if quorumGroup != nil && strings.EqualFold(quorumGroup.Message.FeeRecipient.String(), feeRecipient) { + log.Info(ctx, "Validator already has a complete builder registration, skipping", + z.Str("pubkey", valPubKey), + z.Str("fee_recipient", quorumGroup.Message.FeeRecipient.String())) + + continue + } + + if matchingIncomplete != nil { + // 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, z.Str("pubkey", valPubKey), - z.Str("fee_recipient", quorumGroup.Message.FeeRecipient.String())) + z.I64("provided_timestamp", explicitTimestamp), + z.I64("adopted_timestamp", matchingIncomplete.Message.Timestamp.Unix())) + } - continue + if gasLimitOverride != 0 && matchingIncomplete.Message.GasLimit != gasLimitOverride { + log.Warn(ctx, "Ignoring --gas-limit flag, adopting the in-progress registration gas limit so partial signatures aggregate", nil, + z.Str("pubkey", valPubKey), + z.U64("provided_gas_limit", gasLimitOverride), + z.U64("adopted_gas_limit", matchingIncomplete.Message.GasLimit)) } - if matchingIncomplete != nil { - // Adopt the timestamp and gas limit from the in-progress group so all operators sign the same message. - timestamp = matchingIncomplete.Message.Timestamp - gasLimit = matchingIncomplete.Message.GasLimit + timestamp = matchingIncomplete.Message.Timestamp + gasLimit = matchingIncomplete.Message.GasLimit + + log.Info(ctx, "Validator has partial builder registration with matching fee recipient, proceeding", + z.Str("pubkey", valPubKey), + z.Str("fee_recipient", matchingIncomplete.Message.FeeRecipient.String()), + z.Int("partial_count", len(matchingIncomplete.PartialSignatures))) - log.Info(ctx, "Validator has partial builder registration with matching fee recipient, proceeding", + if quorumGroup != nil && !timestamp.After(quorumGroup.Message.Timestamp) { + log.Warn(ctx, "In-progress registration timestamp is not newer than the current quorum registration, it will never be applied", nil, z.Str("pubkey", valPubKey), - z.Str("fee_recipient", matchingIncomplete.Message.FeeRecipient.String()), - z.Int("partial_count", len(matchingIncomplete.PartialSignatures))) - } else if quorumGroup == nil { + z.I64("in_progress_timestamp", timestamp.Unix()), + z.I64("quorum_timestamp", quorumGroup.Message.Timestamp.Unix())) + } + } else { + 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. for _, reg := range v.BuilderRegistrations { @@ -316,9 +347,17 @@ func filterPubkeysByStatus( } // No in-progress group and no quorum: use defaults set above. } - // else: Quorum exists with different fee, no matching incomplete: use defaults set above. + + // Registrations are applied by timestamp, so a new registration that is not strictly + // newer than the current quorum registration would be signed but never take effect. + if quorumGroup != nil && !timestamp.After(quorumGroup.Message.Timestamp) { + return nil, errors.New("timestamp must be later than the current registration with quorum; pass --timestamp with a later value", + z.Str("pubkey", valPubKey), + z.I64("provided_timestamp", timestamp.Unix()), + z.I64("quorum_timestamp", quorumGroup.Message.Timestamp.Unix()), + ) + } } - // else: Unknown validator: use defaults set above. pubkey, err := parsePubkey(valPubKey) if err != nil { @@ -335,6 +374,29 @@ func filterPubkeysByStatus( return pubkeysToSign, nil } +// validateFeeRecipient checks that the fee recipient address is well-formed, is not the zero +// (burn) address, and, when supplied in mixed case, matches its EIP-55 checksum. +func validateFeeRecipient(address string) error { + checksummed, err := eth2util.ChecksumAddress(address) + if err != nil { + return errors.Wrap(err, "invalid fee recipient address", z.Str("fee_recipient", address)) + } + + if checksummed == "0x0000000000000000000000000000000000000000" { + return errors.New("fee recipient address must not be the zero address", z.Str("fee_recipient", address)) + } + + hexPart := strings.TrimPrefix(address, "0x") + if hexPart != strings.ToLower(hexPart) && address != checksummed { + return errors.New("fee recipient address does not match its EIP-55 checksum; use all-lowercase or the checksummed form", + z.Str("fee_recipient", address), + z.Str("checksummed", checksummed), + ) + } + + 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 { @@ -372,9 +434,10 @@ func validateTimestamp(timestamp int64, pubkeys []string, cl cluster.Lock, overr } // resolveGasLimit returns gasLimitOverride if non-zero. Otherwise it picks the gas limit from -// whichever source (cluster lock or overrides file) has the higher timestamp for the given -// validator pubkey. This ensures the most recent registration's gas limit is used. -func resolveGasLimit(gasLimitOverride uint64, cl cluster.Lock, overrides map[string]eth2v1.ValidatorRegistration, normalizedPubkeyHex string) uint64 { +// whichever source (cluster lock, overrides file, or the current quorum registration on the +// remote API) has the highest timestamp for the given validator pubkey. Consulting the remote +// quorum registration keeps operators with divergent local files signing the same gas limit. +func resolveGasLimit(gasLimitOverride uint64, cl cluster.Lock, overrides map[string]eth2v1.ValidatorRegistration, normalizedPubkeyHex string, quorumMsg *eth2v1.ValidatorRegistration) uint64 { if gasLimitOverride != 0 { return gasLimitOverride } @@ -396,9 +459,14 @@ func resolveGasLimit(gasLimitOverride uint64, cl cluster.Lock, overrides map[str if override, ok := overrides[normalizedPubkeyHex]; ok { if override.Timestamp.After(bestTimestamp) { bestGasLimit = override.GasLimit + bestTimestamp = override.Timestamp } } + if quorumMsg != nil && quorumMsg.Timestamp.After(bestTimestamp) { + bestGasLimit = quorumMsg.GasLimit + } + return bestGasLimit } diff --git a/cmd/feerecipientsign_internal_test.go b/cmd/feerecipientsign_internal_test.go index 85f221c14..cd100fbdd 100644 --- a/cmd/feerecipientsign_internal_test.go +++ b/cmd/feerecipientsign_internal_test.go @@ -53,7 +53,7 @@ func TestFeeRecipientSignValid(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) @@ -104,7 +104,7 @@ func TestFeeRecipientSignWithTimestamp(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil) @@ -153,6 +153,106 @@ func TestFeeRecipientSignInvalidFeeRecipient(t *testing.T) { require.ErrorContains(t, err, "invalid fee recipient address") } +func TestValidateFeeRecipient(t *testing.T) { + // EIP-55 test vector address. + const checksummed = "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed" + + tests := []struct { + name string + address string + expectedErr string + }{ + { + name: "valid lowercase", + address: strings.ToLower(checksummed), + }, + { + name: "valid checksummed", + address: checksummed, + }, + { + name: "invalid checksum", + address: "0x5AAeb6053F3E94C9b9A09f33669435E7Ef1BeAed", + expectedErr: "does not match its EIP-55 checksum", + }, + { + name: "zero address", + address: "0x0000000000000000000000000000000000000000", + expectedErr: "must not be the zero address", + }, + { + name: "not an address", + address: "not-an-address", + expectedErr: "invalid fee recipient address", + }, + { + name: "too short", + address: "0x1234", + expectedErr: "invalid fee recipient address", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := validateFeeRecipient(test.address) + if test.expectedErr != "" { + require.ErrorContains(t, err, test.expectedErr) + } else { + require.NoError(t, err) + } + }) + } +} + +// TestFeeRecipientSignStaleTimestampRejected verifies that signing a new fee recipient with a +// timestamp that is not later than the current quorum registration is rejected, since the +// resulting registration would never be applied. +func TestFeeRecipientSignStaleTimestampRejected(t *testing.T) { + ctx := log.WithCtx(t.Context(), z.Str("test_case", t.Name())) + + c := setupFeeRecipientTestCluster(t, 1) + validatorPubkey := c.lock.Validators[0].PublicKeyHex() + + ts1 := time.Now().Add(time.Hour).Unix() + + signFeeRecipientThreshold(ctx, t, c, validatorPubkey, "0x0000000000000000000000000000000000001234", ts1) + + signConfig := signConfigForOperator(c, 0, validatorPubkey, "0x0000000000000000000000000000000000005678", ts1) + err := runFeeRecipientSign(ctx, signConfig) + require.ErrorContains(t, err, "timestamp must be later than the current registration with quorum") +} + +// TestFeeRecipientSignAdoptsInProgressTimestamp verifies that operators joining an in-progress +// registration adopt its timestamp even when they pass a conflicting --timestamp, so partial +// signatures aggregate to quorum. +func TestFeeRecipientSignAdoptsInProgressTimestamp(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" + + ts1 := time.Now().Add(time.Hour).Unix() + ts2 := time.Now().Add(2 * time.Hour).Unix() + + // The first operator anchors the in-progress registration at ts1. + require.NoError(t, runFeeRecipientSign(ctx, signConfigForOperator(c, 0, validatorPubkey, newFeeRecipient, ts1))) + + // The remaining operators pass a different explicit timestamp, which is ignored in + // favor of the in-progress registration's timestamp. + for opIdx := 1; opIdx < c.lock.Threshold; opIdx++ { + require.NoError(t, runFeeRecipientSign(ctx, signConfigForOperator(c, opIdx, validatorPubkey, newFeeRecipient, ts2))) + } + + overridesFile := filepath.Join(c.root, "output", "builder_registrations_overrides.json") + fetchFeeRecipient(ctx, t, c, nil, overridesFile) + + regs, byPubkey := readOverridesFile(t, overridesFile) + require.Len(t, regs, 1, "partial signatures must aggregate to quorum despite conflicting --timestamp flags") + require.Equal(t, newFeeRecipient, byPubkey[strings.ToLower(validatorPubkey)]) + require.Equal(t, ts1, regs[0].V1.Message.Timestamp.Unix()) +} + func TestFeeRecipientSignInvalidLockFile(t *testing.T) { config := feerecipientSignConfig{ feerecipientConfig: feerecipientConfig{ @@ -190,7 +290,7 @@ func TestFeeRecipientSignAPIUnreachable(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) // Start and immediately close the server so the URL is unreachable. srv := httptest.NewServer(http.NotFoundHandler()) @@ -236,7 +336,7 @@ func TestFeeRecipientSignPubkeyNotInCluster(t *testing.T) { lockJSON, err := json.Marshal(lock) require.NoError(t, err) - writeAllLockData(t, root, operatorAmt, enrs, operatorShares, lockJSON) + writeAllLockData(t, root, enrs, operatorShares, lockJSON) handler, addLockFiles := obolapimock.MockServer(false, nil)