Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SoR History update #416

Merged
merged 28 commits into from Apr 15, 2024
Merged

SoR History update #416

merged 28 commits into from Apr 15, 2024

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Mar 27, 2024

closes #414

The implementation:

  • adds a new backfiller resource to track and backfill past empty entries in the array
  • adds events when a past gap in detected and backfillled
  • adds a minimum SoR length check
  • updates and expands tests to cover the gap cases

@tarakby tarakby changed the title WIP: SoR History update SoR History update Mar 27, 2024
// - `blockHeight` is the height where the backfill happened, it also defines the SoR used to backfill
// - `gapStartHeight` is the height of the first backfilled entry
// - `count` is the number of backfilled entries
// Note that in very rare cases, the backfilled gap may not be contiguous. This event does not
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for adding 2 events and not only RandomHistoryBackfilled is the edge case specifically mentioned in the comment.
If that edge case happens while only emitting RandomHistoryBackfilled, there is no clear way to know what exact indices got backfilled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tarakby tarakby marked this pull request as ready for review March 29, 2024 23:10
@joshuahannan
Copy link
Member

Are you planning on doing this upgrade before or after Cadence 1.0? If the answer is after, I would prefer that you make these changes based on the stable-cadence branch instead of master

@tarakby
Copy link
Contributor Author

tarakby commented Apr 1, 2024

Are you planning on doing this upgrade before or after Cadence 1.0?

I don't know exactly when Cadence 1.0 will be on MN. This PR change should go as soon as the the PR is merged to fix a current on-chain issue, so I guess it's gonna be before Cadence 1.0

Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of minor comments and one question about the backfilled value guarantees

contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Show resolved Hide resolved
Comment on lines 127 to 132
// cache the array length in a local variable to avoid multiple calls to `length`
// when the array is too large
var arrayLength = UInt64(RandomBeaconHistory.randomSourceHistory.length)

// if a new gap is detected, emit an event
if correctIndex > arrayLength {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed with @turbolent in a discord thread that getting an array's length via arr.length is constant time, so seems this comment's content isn't a concern as I thought it might be earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since accessing the length is constant time and cheap, I added this minor optimization FYI e913f3a

contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like changes in 9e5d257 overwrote interface changes in #418 - is there a reason we want to specify the values when calling backfill instead of deriving them in scope?

contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

tests/test_random_beacon_history.cdc Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Outdated Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Show resolved Hide resolved
contracts/RandomBeaconHistory.cdc Show resolved Hide resolved
@tarakby
Copy link
Contributor Author

tarakby commented Apr 15, 2024

@joshuahannan can you please have a look? I think the PR can only be merged after your approval.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After we merge this, you'll need to make an upgrade plan and post it in a forum post like we've done for previous upgrades.

Then you'll also need to make a branch based off of the stable-cadence branch that includes these changes so that we can make sure the upgrades are working properly in the Cadence 1.0 migrations.

@tarakby tarakby merged commit 272df6c into master Apr 15, 2024
2 checks passed
@tarakby tarakby deleted the tarak/fix-sor-history branch May 3, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SoR history gaps when heartbeat call fails
5 participants