stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

Add `cycle-id` ranges to synthetic pox events

Open zone117x opened this issue 1 year ago • 2 comments

Fixes https://github.com/stacks-network/stacks-core/issues/4413

WIP:

  • [x] Add start-cycle-id to pox events
  • [x] Add end-cycle-id to pox events
  • [ ] Tests

zone117x avatar Feb 22 '24 18:02 zone117x

Codecov Report

Attention: Patch coverage is 93.91892% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 73.15%. Comparing base (87347e4) to head (1e14b29). Report is 13 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4414      +/-   ##
==========================================
- Coverage   83.10%   73.15%   -9.95%     
==========================================
  Files         453      453              
  Lines      327459   328034     +575     
  Branches      323      323              
==========================================
- Hits       272122   239972   -32150     
- Misses      55329    88054   +32725     
  Partials        8        8              
Files Coverage Δ
...tackslib/src/chainstate/stacks/boot/pox_4_tests.rs 80.08% <97.37%> (-19.60%) :arrow_down:
pox-locking/src/events.rs 81.23% <76.28%> (-9.41%) :arrow_down:

... and 225 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 87347e4...1e14b29. Read the comment docs.

codecov[bot] avatar Feb 22 '24 19:02 codecov[bot]

Yes -- the existing pox-4 tests would invoke these paths. Those tests should be expanded to check the event receipts though.

kantai avatar Feb 23 '24 14:02 kantai

I took @hstove 's suggestion and chaned end-cycle-id to an optional since it was relying on an optional (until_burn_height) for calculation in delegate-stx.

The test ensures that the events are emitted without Clarity parsing issues BUT, I am not sure about the values I am asserting against at all - I basically filled those from what was being returned so @zone117x @hstove @kantai please check if those values actually make sense for what was intended here.

8marz8 avatar Mar 08 '24 02:03 8marz8

Thanks for the help with this @8marz8! I'm going to test this in an end-to-end environment because I'm also not totally sure if the cycle ids in the tests are accurate or not.

zone117x avatar Mar 08 '24 11:03 zone117x

the assertion is that start-cycle-id is 22 and end-cycle-id is 24. Steph made the transaction during 22, with a lock period of 1. So she would start stacking in 23, and end at the start of 24. This might be ok if we said "the user starts stacking in the cycle after start-cycle-id, but we just need to be consistent with these events. This is also kinda subjective, I'm curious of @zone117x 's thoughts.

In this case I think we'd want start-cycle-id to be 23, because we are using these IDs to match against reward set data. So IIUC we don't care about when stx are locked, we care about when stx are committed. Does that make sense, or is my understanding/terminology correct here?

zone117x avatar Mar 12 '24 11:03 zone117x

✅ Updated this PR to match my understanding of what the range queries should achieve from a consumer perspective (be able to use event data for reward/voting-power calculations). I hope I haven't misinterpreted anything here.

  • Update test to use dynamic cycle id and lock periods -- using the assumption: start-cycle-id == next_cycle by default, (if not in prepare phase case)
  • Update aggregation functions to have start&end == {reward_cycle} (they don't span multiple cycles)
  • Update prepare_offsets to always be based on current burn_height (this could be hardcoded into the prepare snippet)
  • Removed let/prepare_offset blocks where not needed
  • Increase methods will use the normal current burn_height for "start" and re-use the previous "end". (Careful: this will generate [contained] overlapping ranges, either use the never one or modify the existing entry on ingest)
  • Extend methods are the same but use the new unlock-ht for "end". (Careful: same warnings apply, but with an [protruding] overlap here)

janniks avatar Mar 12 '24 23:03 janniks