Add `cycle-id` ranges to synthetic pox events
Fixes https://github.com/stacks-network/stacks-core/issues/4413
WIP:
- [x] Add
start-cycle-idto pox events - [x] Add
end-cycle-idto pox events - [ ] Tests
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 dataPowered by Codecov. Last update 87347e4...1e14b29. Read the comment docs.
Yes -- the existing pox-4 tests would invoke these paths. Those tests should be expanded to check the event receipts though.
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.
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.
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?
✅ 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
aggregationfunctions 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)