Rewards update 2 #235
Merged
Rewards update 2 #235
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also use real Tau parameter now
Reflects the fact that we process the rewards *after* having pushed the snapshot
Matches Haskell output for pool 30c6319d1f, epoch 213 Added more NOTES
- calculated differently from SPO margin payout!
Now accept parameter change earlier so we have ShelleyParams for epoch 208 - this triggers an additional monetary change, which means we have the correct stake_rewards available when we start epoch 212. Removed 'latest' extra snapshot so we do it all one epoch earlier Made stake_rewards Lovelace outside rewards.rs to keep it simpler Removed fees from snapshot - no longer needed because we have them for previous epoch to hand
Move to AccountState so we can filter against previous versions and log only what we're interested in
Temporary fix storing entire stake_address state in snapshot - too expensive, should be reduced to registered SPO reward accounts only
Trying to remove storage of full stake_addresses in snapshots, which is very unpleasant. Storing just a flag which indicates if the SPO's reward account is registered seems like a good solution, but it changes semantics - previous version used the 'go' (staking) version of the SPO's reward account and checked it against the 'mark' stake accounts, and matched DBSync in epoch 211, specifically not paying SPO abcdef... However, the new version uses the 'mark' (performance) version of the SPO's reward account, which is wrong - #abcdef changed reward account during 210 so now gets (wrongly) paid. This needs fixing. Also, more importantly and permanently, now checks delegators accounts for registration
We retain the single flag per SPO in the snapshot, but derive them from the reward accounts of the SPOs in two-previous snapshot now passed in.
Note Verifier now always exists and has optional configuration, decides itself whether to do anything
Note Verifier now always exists and has optional configuration, decides itself whether to do anything
This is ugly, but should be resolved when we switch to StakeAddress throughout, and use .get_hash() on it
- Ignore refunds - Sort including type - Count errors - Trim logs - Log successes at debug
Haskell redoes this calculation each time and keeps fractional part of the (f-cost)*(1-m) bit. We were truncating it which made 1LL difference to a small number of rewards. Rewards paid for epoch 211 now match! Downgraded some debug too
Was still looking up block counts by VRF, but this is already done. Triggers eta=0 in epoch 213 (because d<0.8)
We need to know all effective (and not re-registered) deregistrations that happened between 'go' and 'mark', and in early Shelley, the ones in the current epoch until the rewards calc starts too. Registration changes are now managed through the snapshot chain and this makes it much easier to think about.
Still a weird one with SPO 7fe4d9c44e85fcc958 in rewards epoch (mark) 213. It changed its reward address between 211 and 213, and the new one was only registered during 213, the old one never was (check!). Yet the Haskell node pays out...?
Tidying after massive rebase to main
|
@buddhisthead can you add your CoPilot magic to this? I really need to get this merged or the constant conflict resolution is going to kill me :-) |
whankinsiv
reviewed
Oct 13, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR improves the rewards calculation system to accurately match early Shelley epochs (208-214), implementing significant architectural changes to replicate the behavior and timing bugs present in the original Cardano node implementation.
- Switches SPO identification from VRF keys to issuer key hashes (keyhash_224) throughout the codebase
- Moves snapshot management from rewards module to accounts state and adds timing-sensitive reward calculation
- Implements verification system against captured CSV data from Haskell node/DBSync for validation
Reviewed Changes
Copilot reviewed 27 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| processes/omnibus/omnibus.toml | Adds configuration for pots and rewards verification files |
| modules/stake_delta_filter/src/state.rs | Minor import organization improvement |
| modules/spo_state/src/test_utils.rs | Updates test to use new spo_blocks field instead of vrf_vkey_hashes |
| modules/spo_state/src/state.rs | Major refactor replacing VRF-based SPO identification with issuer key hashes and adding pending updates handling |
| modules/spo_state/src/spo_state.rs | Updates epoch activity handling to use SPO blocks instead of VRF mapping |
| modules/spo_state/src/epochs_history.rs | Updates test to use new spo_blocks field |
| modules/rest_blockfrost/src/handlers/pools.rs | Changes query parameter from vrf_key_hash to spo_id |
| modules/epochs_state/src/state.rs | Switches from VRF-based to issuer-based block counting |
| modules/epochs_state/src/epochs_state.rs | Updates mint handling to use issuer_vkey |
| modules/epochs_state/src/epochs_history.rs | Updates tests for new field names |
| modules/accounts_state/test-data/startStep.txt | Adds test data for early Shelley epochs |
| modules/accounts_state/test-data/pots.mainnet.csv | Comprehensive pots verification data for mainnet epochs |
| modules/accounts_state/src/verifier.rs | New verification system against Haskell node output |
| modules/accounts_state/src/state.rs | Major overhaul adding snapshot management, registration tracking, and delayed reward calculation |
| modules/accounts_state/src/snapshot.rs | Enhanced snapshot creation with two-epoch reward account checking |
| modules/accounts_state/src/rewards.rs | Complete rewards calculation rewrite with proper delegation and timing bug handling |
| modules/accounts_state/src/monetary.rs | Updates return type and treasury cut calculation |
| modules/accounts_state/src/accounts_state.rs | Integrates verifier and reorders parameter handling |
| modules/accounts_state/README.md | Documentation for verification system |
| modules/accounts_state/NOTES.md | Updated test data and analysis |
| modules/accounts_state/Cargo.toml | Adds dependencies for verification (csv, itertools, rayon) |
| common/src/queries/epochs.rs | Updates query field name from vrf_key_hash to spo_id |
| common/src/messages.rs | Changes EpochActivityMessage field from vrf_vkey_hashes to spo_blocks |
| common/src/ledger_state.rs | Adds updates field to SPOState |
| common/src/crypto.rs | Replaces blake2 dependency with cryptoxide and adds keyhash_224 function |
| common/src/address.rs | Updates to use new keyhash_224 function |
| common/Cargo.toml | Switches from blake2 to cryptoxide dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Wow! Co-pilot had nothing to say! (Note to team, I'm investigating Code Rabbit for better Ai PR review process) |
whankinsiv
reviewed
Oct 13, 2025
Comment on lines
+350
to
+357
| // Pool owners don't get member rewards (seems unfair!) | ||
| if spo.pool_owners.contains(hash) { | ||
| debug!( | ||
| "Skipping pool owner reward account {}, losing {to_pay}", | ||
| hex::encode(hash) | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
From my experience as an SPO pool owners do get their proportionate amount of the member rewards.
There was a problem hiding this comment.
Hmm, OK, I added that to match the Haskell node - without it we diverge - needs investigation!
There was a problem hiding this comment.
whankinsiv
reviewed
Oct 13, 2025
whankinsiv
reviewed
Oct 13, 2025
Comment on lines
323
to
326
| let margin_cost = ((&pool_rewards - &fixed_cost) | ||
| * (&margin | ||
| + (BigDecimal::one() - &margin) * (relative_owner_stake / relative_pool_stake))) | ||
| .with_scale(0); |
There was a problem hiding this comment.
From my brief research into margin cost I don't believe pledge influence (relative_owner_stake / relative_pool_stake) is relevant to calculating the margin cost.
There was a problem hiding this comment.
This is a bit of a nightmare - there are two different uses of margin, one to calculate the payout to the SPO, the other the amount taken off the payout to the delegators. You'd think they were the same, but they're not (at least in Shelley)! And they differ by the relative_owner_stake/relative_pool_stake as you say... See slightly further down...
// You'd think this was just (pool_rewards - costs) here, but the Haskell code recalculates
// the margin without the relative_owner_stake term !?
// Note keeping fractional part, which is non-obvious
let to_delegators = (&pool_rewards - &fixed_cost) * (BigDecimal::one() - &margin);
I did verify this with the Haskell code and empirically it works!
There was a problem hiding this comment.
See also Amaru leader_rewards():
and comparison with margin subtracted in member_rewards():
This was referenced Oct 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains improvements of the rewards calculation, working through the early epochs of Shelley - currently able to match epochs 208-214, minor differences in 215.
Significant changes: