-
Notifications
You must be signed in to change notification settings - Fork 3
feat: withdrawalQueue and deallocationQueue logic into rewards calculation #476
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.
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
base: master
Are you sure you want to change the base?
Conversation
53e12db to
ef95a04
Compare
| qsw.strategy, | ||
| qsw.shares_to_withdraw as shares, | ||
| b_queued.block_time as queued_timestamp, | ||
| date(b_queued.block_time) as queued_date, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is rounding being done here (cast block_time to date)? My thinking is that the purpose of this table is to be used to keep track of shares that were in the queue and then once they are completable we insert a diff back into the staker_shares table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is date rounding, not allocation rounding. This is converting block timestamps to dates to figure out what day a withdrawal was queued, compare with snapshot_date, and calculating +14 day withdrawable_date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to do date rounding either. Ideally we just insert the share difference, once the withdrawal is completable, into the staker_shares table. That way, we let the logic we've already written handle the date rounding (staker_share_snapshots)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A design principle that makes sense is that we should do as little rounding as possible. For this functionality, I'd like us to ideally just insert into the staker_shares table. With that in mind, maybe we should have a parallel staker shares table just for rewards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A design principle that makes sense is that we should do as little rounding as possible
+1 to this. dont round, treat this table as a time-series table like the staker shares table
I'd like us to ideally just insert into the staker_shares table.
Dont do this. The resulting values for the snapshots table should just query the staker shares and whatever other tables are needed to derive the values. Since we need to account for some kind of balances based on an effective timestamp, theres no clean way to trigger an insert for that, so querying it and deriving it is much cleaner and doesnt pollute the staker_shares table which is entirely sourced from on-chain events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We should just join a parallel withdrawal_queue table with staker_shares and then have that processed into staker_share_snapshots.
| operator, | ||
| avs, | ||
| strategy, | ||
| magnitude_decrease, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why store the diff? Fine to just store the magnitude itself and then get the latest magnitude for the given day upon calculation of the slashable stake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also work to store prev_magnitude, but I think it's easier to track the diff for multiple pending deallocations and easier aggregation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One invariant of the core protocol is there can be at most 1 pending allocation or deallocation at a time. Unsure if we can take advantage of this. It seems like we're not using the diff in the operatorAllocationSnapshots file
|
The withdrawals are still processed immediately instead of 14 days after. The stakerShares file also has to be edited to reflect this. One thing to think about here is that we would (presumably) want the sidecar API to return the actual shares of the user in the core protocol, which should immediately decremented on a queue. However, for rewards we do not want the shares to be immediately decremented. |
f51270b to
c6ae65a
Compare
11b9e55 to
d06c153
Compare
|
@ypatil12 This is the current architecture:
That's why it would be better not to edit stakerShares but having additional deltas table to correctly calculate rewards. |
d06c153 to
6d252e7
Compare
bfc21ac to
4d8299b
Compare
6d252e7 to
ef0aba3
Compare
ypatil12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to have sequence diagrams on how events are processed, including the propogtaion of an event through each file, for allocation/deallocation and withdrawals
| qsw.strategy, | ||
| qsw.shares_to_withdraw as shares, | ||
| b_queued.block_time as queued_timestamp, | ||
| date(b_queued.block_time) as queued_date, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to do date rounding either. Ideally we just insert the share difference, once the withdrawal is completable, into the staker_shares table. That way, we let the logic we've already written handle the date rounding (staker_share_snapshots)
|
|
||
| func (r *RewardsCalculator) AdjustStakerShareSnapshotsForWithdrawalQueue(snapshotDate string) error { | ||
| adjustQuery := ` | ||
| insert into staker_share_snapshots(staker, strategy, shares, snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we can actually do this. My hunch is that we can run into issues with the snapshotting that's already been done and overwrite state. For example, say the staker shares table looked like:
Day 0: Alice queues a withdrawal for 30 shares.
- Day 1: Alice has 35 shares
- Day 2: Alice has 40 shares (Deposits 5 extra)
Then on Day 2, we also apply a queued withdrawal decrementing her shares down to 10. How do we handle this diff appropriately?
Furthermore, staker_shares keeps track of a running sum of shares and am not sure this handles the shares that were previously credit to the staker.
| qsw.strategy, | ||
| qsw.shares_to_withdraw as shares, | ||
| b_queued.block_time as queued_timestamp, | ||
| date(b_queued.block_time) as queued_date, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A design principle that makes sense is that we should do as little rounding as possible. For this functionality, I'd like us to ideally just insert into the staker_shares table. With that in mind, maybe we should have a parallel staker shares table just for rewards?
ef0aba3 to
9faf22a
Compare
ypatil12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth writing down a diagram or flow on how the staker shares propagate based on a withdrawal and the tables at each step of the process
pkg/postgres/migrations/202511171438_withdrawalAndDeallocationQueues/up.go
Outdated
Show resolved
Hide resolved
41d0383 to
691afe7
Compare
| ROW_NUMBER() OVER (PARTITION BY operator, avs, strategy, operator_set_id, cast(block_time AS DATE) ORDER BY block_time DESC, log_index DESC) AS rn | ||
| FROM operator_allocations oa | ||
| INNER JOIN blocks b ON oa.block_number = b.number | ||
| INNER JOIN blocks b ON oa.effective_block = b.number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you dealing with backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COALESCE(oa.effective_block, oa.block_number) handles old/new records now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file didn't exist before, so there shouldn't be a backwards compatibility issue?
| -- First allocation: round down to current day (conservative default) | ||
| date_trunc('day', block_time) | ||
| -- First allocation: round up to next day | ||
| date_trunc('day', block_time) + INTERVAL '1' day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you dealing with backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an issue if operatorAllocationSnapshots didn't exist before?
4ddeab6 to
89ab4b5
Compare
|
My main point on the above design is that from a design perspective, we should strive to insert share increments/decrements into base bronze tables and then have the snapshot table process these share changes. It's cleaner than inserting a snapshotted record into an already snapshotted table... unless im missing something: So... you have |
3174111 to
9f3db12
Compare
ypatil12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Withdrawal review
| strategy, | ||
| shares, | ||
| block_time, | ||
| date_trunc('day', block_time) + INTERVAL '1' day AS snapshot_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should join the withdrawal_queue_adjustments here with staker_shares, and then have the snapshot calculation run
| `alter table queued_slashing_withdrawals add constraint fk_completion_block foreign key (completion_block_number) references blocks(number) on delete set null`, | ||
| // Note: Withdrawal queue logic uses withdrawable_date to determine when | ||
| // shares should stop earning rewards. The withdrawable_date is calculated as | ||
| // queued_date + 14 days. No additional columns needed in queued_slashing_withdrawals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we also need to do properly be backwards compatible with the queued_slashing_withdrawals table?
| -- First allocation: round down to current day (conservative default) | ||
| date_trunc('day', block_time) | ||
| -- First allocation: round up to next day | ||
| date_trunc('day', block_time) + INTERVAL '1' day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an issue if operatorAllocationSnapshots didn't exist before?
ca53477 to
472ee17
Compare
pkg/eigenState/precommitProcessors/slashingProcessor/slashing.go
Outdated
Show resolved
Hide resolved
ypatil12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accumulating the wads is smart! Brought up some minor cases, but generally we should be good to start really unit testing the stakerShareSnapshots.go file
pkg/eigenState/precommitProcessors/slashingProcessor/slashing.go
Outdated
Show resolved
Hide resolved
| WHERE qsw.operator = @operator | ||
| AND qsw.strategy = @strategy | ||
| -- Withdrawal was queued before this slash | ||
| AND qsw.block_number < @slashBlockNumber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check block number and log index. Since, a slash can happen right after withdrawal was queued
pkg/rewards/stakerShareSnapshots.go
Outdated
| SELECT number FROM blocks WHERE block_time <= TIMESTAMP '{{.cutoffDate}}' ORDER BY number DESC LIMIT 1 | ||
| ) | ||
| ORDER BY adj.slash_block_number DESC | ||
| LIMIT 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we also need to properly handle is beacon chain slashes. Native ETH (eigenpod) shares can be slashed by both the beacon chain and an AVS. We can handle this after we get more traction on ensuring the other parts are correct, but want to call this out. In the shares.go eigenstate we do:
wadSlashed := big.NewInt(1e18)
wadSlashed = wadSlashed.Mul(wadSlashed, new(big.Int).SetUint64(outputData.NewBeaconChainSlashingFactor))
wadSlashed = wadSlashed.Div(wadSlashed, new(big.Int).SetUint64(outputData.PrevBeaconChainSlashingFactor))
wadSlashed = wadSlashed.Sub(big.NewInt(1e18), wadSlashed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will write down an equation for this tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this case is already handled since beacon chain slashed propagate through decrements in stakerShares. What we need to ensure is that these types of slashes are not "double-decremented".
We handles avs slashes through the pathway of OperatorSlashed event, which should not affect the queued_slashing_withdrawals table. Is there anywhere where we blankly assume a decrease in staker shares is a queued withdrawal?
Here is an integration test we should eventually have:
T1: Alice has 200 shares
T2: Alice queues a withdrawal for 50 shares
- EigenState: Alice has 150 shares
- RewardsState: Alice has 200 shares
T3: Bob (Alice’s operator) is slashed for 25% for the BeaconChain Strategy.
- EigenState: -37.5 shares for Alice. Alice has 112.5 Shares
- RewardsState: -50 shares for Alice. Alice has 150 shares
T4: Alice is slashed for 50% for on the BeaconChain
- EigenState: -56.25 Shares for Alice. Alice has 56.25 shares
- RewardsState: -75 shares for Alice. Alice has 75 shares
T15: Alice’s withdrawal is completable
- EigenState: 56.25 shares
- RewardsState: 56.25 shares
a022ccb to
992f33b
Compare
9f3db12 to
8364d9f
Compare
992f33b to
e4874e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. If we need to close out this PR, wdyt about resolving the the non-testing comments and then merge this into a release branch (not master). Then, we can start a big testing push across all files.
otherwise, probably best to keep this PR open and just start adding tests separately. I can start testing tmrw
| OR (qsw.block_number = @slashBlockNumber AND qsw.log_index < @logIndex) | ||
| ) | ||
| -- Still within 14-day window (not yet completable) | ||
| AND DATE(b_queued.block_time) + INTERVAL '14 days' > ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The withdrawal queue window is 10 minutes on preprod/testnet but 14 days on mainnet. We'll have to think about how we should handle this on preprod/testnet environments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, should be good to merge this PR in, but I wrote it down as an open question to handle prior to deploying on preprod. My thinking here is to use the proper interval for each environment. However, tests (locally, integration) should assume we're on mainnet.
pkg/rewards/stakerShareSnapshots.go
Outdated
| SELECT number FROM blocks WHERE block_time <= TIMESTAMP '{{.cutoffDate}}' ORDER BY number DESC LIMIT 1 | ||
| ) | ||
| ORDER BY adj.slash_block_number DESC | ||
| LIMIT 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this case is already handled since beacon chain slashed propagate through decrements in stakerShares. What we need to ensure is that these types of slashes are not "double-decremented".
We handles avs slashes through the pathway of OperatorSlashed event, which should not affect the queued_slashing_withdrawals table. Is there anywhere where we blankly assume a decrease in staker shares is a queued withdrawal?
Here is an integration test we should eventually have:
T1: Alice has 200 shares
T2: Alice queues a withdrawal for 50 shares
- EigenState: Alice has 150 shares
- RewardsState: Alice has 200 shares
T3: Bob (Alice’s operator) is slashed for 25% for the BeaconChain Strategy.
- EigenState: -37.5 shares for Alice. Alice has 112.5 Shares
- RewardsState: -50 shares for Alice. Alice has 150 shares
T4: Alice is slashed for 50% for on the BeaconChain
- EigenState: -56.25 Shares for Alice. Alice has 56.25 shares
- RewardsState: -75 shares for Alice. Alice has 75 shares
T15: Alice’s withdrawal is completable
- EigenState: 56.25 shares
- RewardsState: 56.25 shares
pkg/rewards/stakerShareSnapshots.go
Outdated
| where rn = 1 | ||
| ), | ||
| -- Get the range for each operator, strategy pairing | ||
| -- Join with withdrawal queue at bronze level before creating windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still inserting these records after the snapshotting is being done. We ideally should insert them before the snapshotting is being done. Does doing so break the tests in any way?
Ideally, we only join with diffs and then we snapshot. Right now we're taking the staker share diffs, snapshotting, and then inserting these withdrawal queue adjustments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a change to: Bronze diffs → Join → Snapshot windows → Expand
| // T2: 50 shares (35 base + 15 queued after 50% slash) | ||
| // T3: 35 shares (queued withdrawal no longer counts) | ||
|
|
||
| assert.GreaterOrEqual(t, len(snapshots), 3, "Should have at least 3 unique snapshots") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should assert the actual values, not just the number of snapshots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and applied to the whole test
pkg/rewards/stakerShareSnapshots.go
Outdated
| where rn = 1 | ||
| ), | ||
| -- Get the range for each operator, strategy pairing | ||
| -- Join with withdrawal queue at bronze level before creating windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backwards compatibility, we really should be doing if casing on the sabine fork block, like we do across the rewards calc for other hard forks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the same block-based fork checking pattern like eigenState
| OR (qsw.block_number = @slashBlockNumber AND qsw.log_index < @logIndex) | ||
| ) | ||
| -- Still within 14-day window (not yet completable) | ||
| AND DATE(b_queued.block_time) + INTERVAL '14 days' > ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: these conversions don't work as intended because they convert queued.block_time into a date and then add 14 days. We need to add 14 days in seconds to the block_time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed DATE and using TIMESTAMP to have block_time in seconds
Description
Operators were not earning rewards during the deallocation queue period, even though their allocations remain at risk until the
effective_date.Current behavior:
operator_allocationstable immediately shows reduced allocationExpected behavior:
effective_dateeffective_date, earn on NEW (lower) allocationSolution
Added
AdjustOperatorShareSnapshotsForDeallocationQueue()function that:magnitude_decreasefromdeallocation_queue_snapshotsoperator_share_snapshotsduring queue periodThis mirrors the existing withdrawal queue adjustment for stakers.
Changes
deallocationQueueShareSnapshots.gorewards.go(line 684-689)effective_date- no manual cleanup neededType of change
How Has This Been Tested?
Checklist: