feat: add ETH cluster migration support with dual reward trees - #26
feat: add ETH cluster migration support with dual reward trees#26olegshmuelov wants to merge 5 commits into
Conversation
4867f99
handleClusterMigration read live nodeStorage state, which races ahead of the postgres recorder. This caused FK violations on post-migration validator adds and wrong migration_day attribution for SSV→ETH hop scenarios. Replaces the badger read with a chronological walk over validator_events at the migration's (block, log_index) position. Extracts reduceClusterMembers as a pure function with 11 unit tests.
There was a problem hiding this comment.
LGTM overall, the open items are mostly external validation and test coverage rather than code defects.
Things to confirm before relying on this in production (details inline):
- Validate the
ClusterMigratedToETHABI/topic against the deployed contract — the topic test is circular and can't catch a wrong signature. - Confirm the ETH tree is published to a separate on-chain distributor (a recipient can appear in both cumulative files).
- Pin down what the event's
validatorCountmeans, then decide if the mismatch should be a hard error. - Add coverage for the dual-tree reward math + the SQL day-split (
cmd/ssv-rewards/has no tests today).
|
|
||
| // ClusterMigratedToETHTopic is keccak256 of the ClusterMigratedToETH event signature. | ||
| var ClusterMigratedToETHTopic = crypto.Keccak256Hash([]byte( | ||
| "ClusterMigratedToETH(address,uint64[],uint256,uint256,uint32,(uint32,uint64,uint64,bool,uint256))", |
There was a problem hiding this comment.
Highest-risk item: unverified event signature. This topic hash comes from a signature that (per the QA doc) isn't emitted on mainnet yet, and TestClusterMigratedToETHTopic only asserts that this exact string hashes to a hardcoded digest — it's circular and cannot detect a wrong signature.
The two failure modes are asymmetric and both bad:
- Topic hash differs → the events fall through the unknown-event path and are silently ignored; no migrations are detected and nothing errors.
- Topic matches but the data layout differs →
Inputs.Unpackerrors andhandleClusterMigrationreturns an error, which aborts the whole sync.
Please validate against the deployed contract and replace the circular test with a real on-chain log fixture before depending on this.
| {"indexed": false, "name": "operatorIds", "type": "uint64[]"}, | ||
| {"indexed": false, "name": "ethDeposited", "type": "uint256"}, | ||
| {"indexed": false, "name": "ssvRefunded", "type": "uint256"}, | ||
| {"indexed": false, "name": "effectiveBalance", "type": "uint32"}, |
There was a problem hiding this comment.
Tell that the ABI still needs confirming: effectiveBalance as uint32. uint32 maxes out at ~4.29e9, which can't hold a Gwei effective balance (32 ETH = 32e9 Gwei).
One correction to be precise about the consequence: a wider on-chain type wouldn't shift the fields after it — every static scalar (uint32/uint64/uint256) occupies exactly one 32-byte word, so the cluster tuple at decoded[4] stays put either way. The actual failure mode is the opposite of a silent mis-decode:
- go-ethereum's
Unpackrange-checks each integer slot (ReadInteger→errBadUint32), so an on-chain value exceedinguint32max fails the entire decode.handleClusterMigrationthen returns an error and aborts the sync — fail-closed, not silent. - Note
effectiveBalance(decoded[3]) isn't even read by the handler, yet declaring ituint32is enough to halt sync on the first real event if the units are Gwei.
Fix: declare it uint256 (decodes any value safely), or pin the exact type/units against the deployed contract — an unused field shouldn't be able to abort the sync. (Verified against the pinned go-ethereum v1.15.1.)
| zap.String("owner", strings.ToLower(owner.Hex())), | ||
| ) | ||
| return nil | ||
| } else if len(members) != int(validatorCount) { |
There was a problem hiding this comment.
Downgrading this to a warning means a wrong member derivation silently lands validators in the wrong tree → wrong rewards, with no hard stop, on irreversible on-chain output.
Two questions:
- What does
validatorCountin the event'sclustertuple represent? If it's the post-migration SSV count (likely0), this warns on every real migration and becomes pure noise. - Once the semantics are pinned down, consider whether this should be a hard error rather than a warn given the stakes.
| } | ||
| validatorCount := clusterStruct.ValidatorCount | ||
|
|
||
| clusterID, err := ssvtypes.ComputeClusterIDHash(owner.Bytes(), operatorIds) |
There was a problem hiding this comment.
Nit: ComputeClusterIDHash sorts operatorIds in place. Harmless here (the slice isn't used afterward), but reduceClusterMembers defensively copies before the same call — a one-line comment here keeps the two consistent and guards against a future edit that reuses operatorIds after this point.
| for _, p := range ethTotalByRecipient { | ||
| ethTotalRewards["0x"+p.RecipientAddress] = p.reward.String() | ||
| } | ||
| ef, err := os.Create(filepath.Join(roundDir, "cumulative-eth.json")) |
There was a problem hiding this comment.
Confirm before publishing. A recipient with both pre- and post-migration days appears in both cumulative.json and this cumulative-eth.json. The historical claim model is cumulative-per-address against a single distributor; if both roots are published to the same distributor, the cumulative accounting collides (double-count / under-pay). Please confirm the ETH tree goes to a separate distributor funded from the ETH-fee pool, and document that assumption.
| // processRoundWithMigration handles reward calculation for rounds with ETH migration support. | ||
| // It queries both SSV and ETH validator participations, computes a combined effective balance | ||
| // for tier selection, and applies a single shared inflation cap across both trees. | ||
| func (c *CalcCmd) processRoundWithMigration( |
There was a problem hiding this comment.
This is the most financially-sensitive new code, and cmd/ssv-rewards/ currently has no tests at all. The combined-EB tier selection and the single shared inflation cap across both trees are subtle.
I confirmed by reading that with no migrations this reduces exactly to processRound, but that invariant deserves a regression test. Suggest at least:
- SSV active_days + ETH active_days = original active_days across the
migration_dayboundary, and - shared cap: SSV total + ETH total for a capped round equals the pre-split capped total.
|
|
||
| if eventTrace.Error != nil { | ||
| // Check if this is a ClusterMigratedToETH event we can handle ourselves. | ||
| if len(eventTrace.Log.Topics) > 0 && eventTrace.Log.Topics[0] == rewards.ClusterMigratedToETHTopic { |
There was a problem hiding this comment.
This interception only fires because the pinned bloxapp/ssv treats ClusterMigratedToETH as an unknown event (eventTrace.Error != nil). I confirmed that holds for the current pin (EventByID fails → trace emitted with Error set), but if the dependency is ever bumped to a version that recognizes this event, Error becomes nil and this branch is silently bypassed. Worth a comment noting the coupling, or also handling the non-error path.
| continue | ||
| } | ||
| recordedEvents++ | ||
| // Fetch databaseEvent before error check — needed for both migration |
There was a problem hiding this comment.
Minor: moving the databaseEvent lookup ahead of the error check means it now runs for every event, including the unknown/malformed ones that previously continued before any query. That adds a DB round-trip per error event during the reconciliation-critical replay and a new hard-fail path if the lookup ever misses. Almost certainly fine — just flagging the slightly widened failure surface.
| AND ( | ||
| CASE migration_filter | ||
| WHEN 'ssv' THEN (v.migration_day IS NULL OR vp.day < v.migration_day) | ||
| WHEN 'eth' THEN (v.migration_day IS NOT NULL AND vp.day >= v.migration_day) |
There was a problem hiding this comment.
The migration_filter day-split is the heart of the dual-tree logic and lives entirely in SQL that CI never compiles or runs. Together with the lack of calc tests, the boundary semantics (< migration_day → SSV, >= migration_day → ETH) are validated only by the manual QA plan. A DB-backed test over a small fixture would de-risk this considerably.
Summary
ClusterMigratedToETHevents: setsmigration_dayon affected validators and inserts correspondingvalidator_eventsrecordsmigration_filterparameter ('ssv'/'eth') to all SQL participation and exclusion functions, splitting validator-days by tree-ethsuffixed outputs for ETH treestaking_upgradeconfig (block + log_index) to mark the on-chain staking contract migration boundaryisPostStakingUpgradeboundary logic, ABI decoding round-trip, topic hash, andStakingUpgradeplan validationTest plan
go test ./...passesALTER TABLE validators ADD COLUMN IF NOT EXISTS migration_day DATE;, thencalc— SSV merkle root must match the latest published roundsync --fresh --keep-cache+calcon a DB withClusterMigratedToETHevents — verify SSV and ETH trees are produced correctlycumulative-eth.jsonis only written when ETH tree has entriescumulative.jsonandcumulative-eth.json