feat: fix rewards calculations with stake address map in bootstrap #480
Conversation
lowhung
force-pushed
the
lowhung/generate-spdd-from-bootstrap
branch
from
47a4ee5 to
f718dac
Compare
lowhung
changed the title
lowhung
requested review from
alexwoods,
buddhisthead,
sandtreader and
whankinsiv
and removed request for
alexwoods
sandtreader
left a comment
sandtreader
left a comment
There was a problem hiding this comment.
Great stuff - agree that UTXOs should be decoded into existing core types in the future. Requested change is watch out for flattening rationals too early - better to preserve them.
| d.array()?; | ||
| let num: i64 = d.decode()?; | ||
| let den: u64 = d.decode()?; | ||
| return Ok(num as f64 / den as f64); |
There was a problem hiding this comment.
Beware of flattening rationals into f64 - precision can be lost. Better to keep as a RationalNumber
| use crate::snapshot::utxo::{TransactionInput, TransactionOutput, Value}; | ||
|
|
||
| /// Helper to create a test UtxoEntry with just an address | ||
| fn make_test_utxo(address_bytes: Vec<u8>) -> UtxoEntry { |
There was a problem hiding this comment.
This and the tests that use it should be in utxos.rs now, I think.
common/src/snapshot/utxo.rs
Outdated
| /// - Base addresses (type 0-3): 1 byte header + 28 bytes payment + 28 bytes stake | ||
| /// - For types 0,1: stake part is key hash | ||
| /// - For types 2,3: stake part is script hash | ||
| pub fn extract_stake_credential(&self) -> Option<StakeCredential> { |
There was a problem hiding this comment.
We already have this code in address.rs I think
There was a problem hiding this comment.
Good point, I wonder if this would suffice? d8a7a4a
| context.publish(&snapshot_topic, message).await.unwrap_or_else(|e| { | ||
| tracing::error!("Failed to publish SPO bootstrap message: {}", e) | ||
| }); | ||
| }) |
There was a problem hiding this comment.
Callbacks should be async I think - noted on Chris' PR
There was a problem hiding this comment.
The parser isn't properly async at the moment, so we're blocking the calls for now. I'd like to discuss this further -- whether this process be fully synchronous or async.
- Add SignedRationalNumber type to support signed numerators (needed for log-likelihood values which are typically negative) - Update Likelihood to use Vec<SignedRationalNumber> instead of Vec<f64> - Refactor decode_likelihood_value to return SignedRationalNumber - Consolidate duplicate test modules in utxo.rs
# Conflicts: # common/src/snapshot/streaming_snapshot.rs # modules/snapshot_bootstrapper/src/publisher.rs
…dd-from-bootstrap # Conflicts: # common/examples/test_streaming_parser.rs # common/src/snapshot/streaming_snapshot.rs # common/src/snapshot/utxo.rs
sandtreader
left a comment
sandtreader
left a comment
There was a problem hiding this comment.
Looks great - async stuff can wait
Description
Fixes incorrect reward calculation during snapshot bootstrap. Previously, the bootstrap process used instantaneous reward balances from
DState, which didn't match the actual stake distribution used by Cardano for epoch calculations. This PR updates the parser to extract rewards from thepulsing_rew_updatesnapshot, ensuring accurate stake distribution and correct SPDD generation.Related Issue(s)
Fixes #388
How was this tested?
make runfor local snapshot bootstrap with epoch 509 worksepoch_staketable using (spdd generation code was unstaged, only used as means to test stake address map creation was valid)Checklist
Impact / Side effects
Positive: SPDD now matches cardano-db-sync data, enabling accurate stake calculations and proper node initialization when booting from snapshot. It provides the basis for future utxo / address deltas being applied to the correct base state for the stake address map (which is what the spdd is generated from).
Performance: Minimal impact - snapshot parsing is still quick.
Reviewer notes / Areas to focus
parse_pulsing_reward_update()method - handles both pulsing and completed reward states