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

fix(cosmic-swingset): Provide blockTime to bootstrap #7125

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Mar 6, 2023

closes: #7124

Description

call timer.poll() before running the bootstrap code.

The wiring of the device is performed synchronously when the devices are built, so there the implementation won't throw.
The implementation of poll should only access its own state since no callbacks would be registered yet, which seems safe to do before any bootstrap code has actually run. I have not fully audited the device code however to check this, just a cursory look.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Tested locally with the loadgen to make sure this change actually solves #7124 and that no timer related errors occur.

@mhofman mhofman requested a review from warner March 6, 2023 21:09
@mhofman mhofman added the automerge:squash Automatically squash merge label Mar 6, 2023
@datadog-full-agoric
Copy link

datadog-full-agoric bot commented Mar 6, 2023

Datadog Report

Branch report: mhofman/7124-bootstrap-blocktime
Commit report: 3e413c2

agoric-sdk: 0 Failed, 0 New Flaky, 3660 Passed, 57 Skipped, 15m 15.39s Wall Time

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

That looks fine for start-from-scratch chains. Will this also work for the bulldozer upgrade, or will blockHeight be wrong? Should we be copying blockHeight out of action or something?

If this works for both cases, go ahead and land. If it won't work for the bulldozer upgrade, please land it and then file an issue to remind us to fix the second case before the release.

@mhofman
Copy link
Member Author

mhofman commented Mar 6, 2023

If this works for both cases, go ahead and land. If it won't work for the bulldozer upgrade, please land it and then file an issue to remind us to fix the second case before the release.

We're not currently using the blockHeight anywhere for bootstrap, and I'm actually not sure what the blockHeight should be in the bulldozer case. 0 is treated somewhat special in hangover checks, but bootstrap doesn't actually set that value itself, so it seems a bit arbitrary. @michaelfig any ideas? Should we attempt to use a different more accurate value for bootstrap blockHeight, and in which case, what would the value be?

@mergify mergify bot merged commit 748d52c into master Mar 6, 2023
@mergify mergify bot deleted the mhofman/7124-bootstrap-blocktime branch March 6, 2023 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vat-timer thinks time=0 during bootstrap block
2 participants