Add cycle-id ranges to synthetic pox events

[zone117x]

Fixes #4413

WIP:

• Add start-cycle-id to pox events
• Add end-cycle-id to pox events
• Tests

[codecov]

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%)	⬇️
pox-locking/src/events.rs	81.23% <76.28%> (-9.41%)	⬇️

... 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.

[hstove]

I think this looks good, and tests will catch any Clarity errors, right? It would be nice to have tests for these new event keys.

[kantai]

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

[8marz8]

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.

[zone117x]

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.

[hstove]

@8marz8 so for the check against Steph's stack-stx, 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.

Then for the stack-increase check, both the start and end are 24. When calling stack increase, you don't change your cycles at all, you're just adding more to the existing cycles. So in my opinion, this should match the stack-stx behavior, especially because the tx is made in the same cycle as the stack-stx tx.

This is very close though, nice job!

[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?

[janniks]

✅ 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)

[hstove]

LGTM. Nice improvements and tests @janniks !

[blockstack-devops]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.