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

Add test for proposervm BuildBlock after bootstrapping #2876

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Mar 27, 2024

Why this should be merged

  1. https://github.com/ava-labs/avalanchego/blob/master/vms/proposervm/scheduler/scheduler.go#L52. The initial build block time is specified when starting the scheduler.
  2. https://github.com/ava-labs/avalanchego/blob/master/vms/proposervm/vm.go#L181. The initial build block time is just passed in as time.Now() - so the scheduler is initialized to allow BuildBlock requests from the inner vm.
  3. https://github.com/ava-labs/avalanchego/blob/master/snow/engine/snowman/transitive.go#L500. When the node is transitioning into normal operations, we initialize the preference.
    a. https://github.com/ava-labs/avalanchego/blob/master/snow/engine/snowman/transitive.go#L513. Notice that this is before SetState is called, so we wouldn't expect the inner VM to have requested block building yet.
  4. https://github.com/ava-labs/avalanchego/blob/master/vms/proposervm/vm.go#L340-L343. It was known that in this situation the P-chain may have been behind this chain due to blocks being force accepted during bootstrapping.
    a. What I had forgotten was that the scheduler was actually initialized to allow the BuildBlock calls through from the inner VM (by initially passing in time.Now()).
  5. At this point the flow feels fairly clear to get to the error:
    a. https://github.com/ava-labs/avalanchego/blob/master/vms/proposervm/vm.go#L279
    b. https://github.com/ava-labs/avalanchego/blob/master/vms/proposervm/post_fork_block.go#L148
    c. https://github.com/ava-labs/avalanchego/blob/master/vms/proposervm/block.go#L203
    d. https://github.com/ava-labs/avalanchego/blob/master/vms/proposervm/block.go#L413

Now, I'm not exactly sure how we want to handle this... Normally we'd re-add the the PendingTxs notification to the inner VM's toEngine channel... But in this case that would just be a passthrough to the proposer VM's toEngine channel... So we'd just spin calling BuildBlock.

Alternatively, rather than passing in time.Now() to the scheduler initialization, we could pass in something like MaxTime. But if SetPreference isn't isn't called after the P-chain advances... then we'd never build the block. We could just pick some random time in the future... But that seems arbitrary and fragile.

Perhaps we should add some method to the validators.State interface that blocks until the requested height is accepted (or just periodically poll the existing functions) so that we could then manually re-calculate the scheduler window at that time.

How this works

How this was tested

@StephenButtolph StephenButtolph added the antithesis Related to an issue reported by Antithesis label Mar 27, 2024
@StephenButtolph StephenButtolph added the bug Something isn't working label Apr 4, 2024
Copy link

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
antithesis Related to an issue reported by Antithesis bug Something isn't working lifecycle/stale
Projects
Status: Backlog 🗄️
Development

Successfully merging this pull request may close these issues.

None yet

2 participants