Skip to content

Apply state modifications once #2473

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Apply state modifications once #2473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be60513
Apply state modifications once
StephenButtolph Dec 12, 2023
fc0c2f8
simplify
StephenButtolph Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions vms/platformvm/block/executor/acceptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,16 @@ func (a *acceptor) optionBlock(b, parent block.Block, blockType string) error {
return err
}

parentState, ok := a.blkIDToState[parentID]
if !ok {
return fmt.Errorf("%w %s", errMissingBlockState, parentID)
}
if parentState.onDecisionState != nil {
if err := parentState.onDecisionState.Apply(a.state); err != nil {
Copy link
Contributor

@patrick-ogrady patrick-ogrady Dec 12, 2023

Choose a reason for hiding this comment

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

To confirm, just calling the onAcceptState doesn't commit all diffs under it (for example the wrapped decision state)?

Copy link
Contributor Author

@StephenButtolph StephenButtolph Dec 12, 2023

Choose a reason for hiding this comment

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

That's correct

Copy link
Contributor Author

@StephenButtolph StephenButtolph Dec 12, 2023

Choose a reason for hiding this comment

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

We could apply the onAcceptState to onDecisionState and then apply onDecisionState to a.state if you felt that was cleaner... But this is the (slightly) more efficient way to do the same thing.

Copy link
Contributor

@patrick-ogrady patrick-ogrady Dec 12, 2023

Choose a reason for hiding this comment

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

I think what we have here is fine. Makes sense (should be the same thing).

return err
}
}

blkState, ok := a.blkIDToState[blkID]
if !ok {
return fmt.Errorf("%w %s", errMissingBlockState, blkID)
Expand All @@ -191,11 +201,11 @@ func (a *acceptor) optionBlock(b, parent block.Block, blockType string) error {
}

// Note that this method writes [batch] to the database.
if err := a.ctx.SharedMemory.Apply(blkState.atomicRequests, batch); err != nil {
if err := a.ctx.SharedMemory.Apply(parentState.atomicRequests, batch); err != nil {
return fmt.Errorf("failed to apply vm's state to shared memory: %w", err)
}

if onAcceptFunc := blkState.onAcceptFunc; onAcceptFunc != nil {
if onAcceptFunc := parentState.onAcceptFunc; onAcceptFunc != nil {
onAcceptFunc()
}

Expand Down
8 changes: 4 additions & 4 deletions vms/platformvm/block/executor/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,20 @@ func (b *backend) GetState(blkID ids.ID) (state.Chain, bool) {
return b.state, blkID == b.state.GetLastAccepted()
}

func (b *backend) getBlkWithOnAbortState(blkID ids.ID) (*blockState, bool) {
func (b *backend) getOnAbortState(blkID ids.ID) (state.Diff, bool) {
state, ok := b.blkIDToState[blkID]
if !ok || state.onAbortState == nil {
return nil, false
}
return state, true
return state.onAbortState, true
}

func (b *backend) getBlkWithOnCommitState(blkID ids.ID) (*blockState, bool) {
func (b *backend) getOnCommitState(blkID ids.ID) (state.Diff, bool) {
state, ok := b.blkIDToState[blkID]
if !ok || state.onCommitState == nil {
return nil, false
}
return state, true
return state.onCommitState, true
}

func (b *backend) GetBlock(blkID ids.ID) (block.Block, error) {
Expand Down
2 changes: 1 addition & 1 deletion vms/platformvm/block/executor/block_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import (

type proposalBlockState struct {
initiallyPreferCommit bool
onDecisionState state.Diff
onCommitState state.Diff
onAbortState state.Diff
onDecisionState state.Diff
}

// The state of a block.
Expand Down
68 changes: 25 additions & 43 deletions vms/platformvm/block/executor/verifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,7 @@ func (v *verifier) BanffProposalBlock(b *block.BanffProposalBlock) error {
}

parentID := b.Parent()
onCommitState, err := state.NewDiff(parentID, v.backend)
if err != nil {
return err
}
onAbortState, err := state.NewDiff(parentID, v.backend)
onDecisionState, err := state.NewDiff(parentID, v.backend)
if err != nil {
return err
}
Expand All @@ -72,36 +68,40 @@ func (v *verifier) BanffProposalBlock(b *block.BanffProposalBlock) error {
nextChainTime := b.Timestamp()
changes, err := executor.AdvanceTimeTo(
v.txExecutorBackend,
onCommitState,
onDecisionState,
nextChainTime,
)
if err != nil {
return err
}

onCommitState.SetTimestamp(nextChainTime)
changes.Apply(onCommitState)
onDecisionState.SetTimestamp(nextChainTime)
changes.Apply(onDecisionState)

onAbortState.SetTimestamp(nextChainTime)
changes.Apply(onAbortState)
inputs, atomicRequests, onAcceptFunc, err := v.processStandardTxs(b.Transactions, onDecisionState, b.Parent())
if err != nil {
return err
}

// Apply the changes, if any, from processing the decision txs.
// [onCommitState] = [onAbortState] here, either one can be used. It is
// only used to ensure that [onDecisionState] contains only the change diff
// of all the standard txs *after* the timestamp advancement changes are
// applied. [onDecisionState] will be applied to [onCommitState] or
// [onAbortState] depending on if the proposal is committed or aborted.
onDecisionState, err := wrapState(onCommitState)
onCommitState, err := wrapState(onDecisionState)
if err != nil {
return err
}

inputs, atomicRequests, onAcceptFunc, err := v.processStandardTxs(b.Transactions, onDecisionState, b.Parent())
onAbortState, err := wrapState(onDecisionState)
if err != nil {
return err
}

return v.proposalBlock(&b.ApricotProposalBlock, onDecisionState, onCommitState, onAbortState, inputs, atomicRequests, onAcceptFunc)
return v.proposalBlock(
&b.ApricotProposalBlock,
onDecisionState,
onCommitState,
onAbortState,
inputs,
atomicRequests,
onAcceptFunc,
)
}

func (v *verifier) BanffStandardBlock(b *block.BanffStandardBlock) error {
Expand Down Expand Up @@ -335,51 +335,33 @@ func (v *verifier) commonBlock(b block.Block) error {
// abortBlock populates the state of this block if [nil] is returned
func (v *verifier) abortBlock(b block.Block) error {
parentID := b.Parent()
parentState, ok := v.getBlkWithOnAbortState(parentID)
onAbortState, ok := v.getOnAbortState(parentID)
if !ok {
return fmt.Errorf("%w: %s", state.ErrMissingParentState, parentID)
}

if err := parentState.onDecisionState.Apply(parentState.onAbortState); err != nil {
return err
}

blkID := b.ID()
v.blkIDToState[blkID] = &blockState{
statelessBlock: b,

onAcceptState: parentState.onAbortState,
onAcceptFunc: parentState.onAcceptFunc,

inputs: parentState.inputs,
timestamp: parentState.onAbortState.GetTimestamp(),
atomicRequests: parentState.atomicRequests,
onAcceptState: onAbortState,
timestamp: onAbortState.GetTimestamp(),
}
return nil
}

// commitBlock populates the state of this block if [nil] is returned
func (v *verifier) commitBlock(b block.Block) error {
parentID := b.Parent()
parentState, ok := v.getBlkWithOnCommitState(parentID)
onCommitState, ok := v.getOnCommitState(parentID)
if !ok {
return fmt.Errorf("%w: %s", state.ErrMissingParentState, parentID)
}

if err := parentState.onDecisionState.Apply(parentState.onCommitState); err != nil {
return err
}

blkID := b.ID()
v.blkIDToState[blkID] = &blockState{
statelessBlock: b,

onAcceptState: parentState.onCommitState,
onAcceptFunc: parentState.onAcceptFunc,

inputs: parentState.inputs,
timestamp: parentState.onCommitState.GetTimestamp(),
atomicRequests: parentState.atomicRequests,
onAcceptState: onCommitState,
timestamp: onCommitState.GetTimestamp(),
}
return nil
}
Expand Down
2 changes: 0 additions & 2 deletions vms/platformvm/block/executor/verifier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,6 @@ func TestVerifierVisitCommitBlock(t *testing.T) {
timestamp := time.Now()
gomock.InOrder(
parentStatelessBlk.EXPECT().Height().Return(uint64(1)).Times(1),
parentOnDecisionState.EXPECT().Apply(parentOnCommitState).Return(nil),
parentOnCommitState.EXPECT().GetTimestamp().Return(timestamp).Times(1),
)

Expand Down Expand Up @@ -428,7 +427,6 @@ func TestVerifierVisitAbortBlock(t *testing.T) {
timestamp := time.Now()
gomock.InOrder(
parentStatelessBlk.EXPECT().Height().Return(uint64(1)).Times(1),
parentOnDecisionState.EXPECT().Apply(parentOnAbortState).Return(nil),
parentOnAbortState.EXPECT().GetTimestamp().Return(timestamp).Times(1),
)

Expand Down