Allow calls to Options before Verify
#2363
Conversation
| if parentState.initiallyPreferCommit { | ||
| a.metrics.MarkOptionVoteLost() | ||
| } else { | ||
| a.metrics.MarkOptionVoteWon() | ||
| } |
There was a problem hiding this comment.
We don't set this value during execution, so the acceptor doesn't really have access to it anymore.
| } | ||
|
|
||
| func (a *acceptor) BanffCommitBlock(b *block.BanffCommitBlock) error { | ||
| return a.commitBlock(b, "apricot commit") | ||
| return a.optionBlock(b, "banff commit") |
There was a problem hiding this comment.
This was the wrong block previously
| o.preferredBlock, err = block.NewApricotCommitBlock(blkID, nextHeight) | ||
| if err != nil { | ||
| return fmt.Errorf( | ||
| "failed to create commit block: %w", | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| o.abortBlock, err = block.NewApricotAbortBlock(blkID, nextHeight) | ||
| o.alternateBlock, err = block.NewApricotAbortBlock(blkID, nextHeight) |
There was a problem hiding this comment.
We don't really care about the preferences for Apricot blocks at this point, because they have all been accepted already. We could probably change some code to actually remove this logic... But it's out of scope for this PR.
| @@ -20,7 +20,7 @@ type Backend struct { | |||
| Clk *mockable.Clock | |||
| Fx fx.Fx | |||
| FlowChecker utxo.Verifier | |||
| Uptimes uptime.Manager | |||
| Uptimes uptime.Calculator | |||
There was a problem hiding this comment.
This change could be factored out - it's needed to ease testing.
| // Marks if this validator should be rewarded according to this node. | ||
| ShouldPreferCommit bool `json:"-"` | ||
|
|
There was a problem hiding this comment.
This was already dead code.
StephenButtolph
requested review from
abi87,
danlaine and
gyuho
as code owners
| } | ||
|
|
||
| nodeID := staker.NodeID() | ||
| primaryNetworkValidator, err := o.state.GetCurrentValidator( |
There was a problem hiding this comment.
Although these state checks rely on accepted state (rather than parent), I'm ok with them here (just looking up Txs).
| @@ -18,6 +18,5 @@ var ErrNotOracle = errors.New("block isn't an oracle") | |||
| type OracleBlock interface { | |||
| // Options returns the possible children of this block in the order this | |||
| // validator prefers the blocks. | |||
| // Options is guaranteed to only be called on a verified block. | |||
There was a problem hiding this comment.
This is the whole point of the PR
| // Mark that an option vote that we initially preferred was accepted. | ||
| MarkOptionVoteWon() | ||
| // Mark that an option vote that we initially preferred was rejected. | ||
| MarkOptionVoteLost() |
There was a problem hiding this comment.
Why are we removing these metrics in this PR?
There was a problem hiding this comment.
see: #2363 (comment)
| @@ -814,35 +814,6 @@ func TestAdvanceTimeTxDelegatorStakers(t *testing.T) { | |||
| require.Equal(env.config.MinDelegatorStake+env.config.MinValidatorStake, vdrWeight) | |||
| } | |||
|
|
|||
| // Test method InitiallyPrefersCommit | |||
| func TestAdvanceTimeTxInitiallyPrefersCommit(t *testing.T) { | |||
There was a problem hiding this comment.
q: why simply removing it? Shouldn't we update it so that, calling options on the block, the commit is the prefered one?
There was a problem hiding this comment.
This test was removed because the functionality was removed. The txs package no longer does anything w.r.t. option voting. I think the added tests in vms/platformvm/block/executor/block_test.go should now cover what this was previously testing.
Why this should be merged
In order to support deferred verification, we must register our preferences before calling
Verify.How this works
Replaces usage of the parent state with the last accepted state. If the validator/delegator being removed is still processing, then we default to rewarding them. In practice this should never happen because the minimum staking duration is 2 weeks.
How this was tested