Skip to content

Vm sdk #3909

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Vm sdk #3909

Show file tree
Hide file tree
Changes from all commits
Commits
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
58 changes: 58 additions & 0 deletions cache/fifo_cache.go
Copy link
Contributor

Choose a reason for hiding this comment

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

With the recent cache refactor, I think this should be it its own fifo package like how we have lru

Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Align with the avalanchego license header (applies to all files)

Suggested change
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.

// See the file LICENSE for licensing terms.

package cache

import (
"sync"

"github.com/ava-labs/avalanchego/utils/buffer"
)

type FIFO[K comparable, V any] struct {
l sync.RWMutex
buffer buffer.Queue[K]
m map[K]V
}

// NewFIFO creates a new First-In-First-Out cache of size [limit].
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc links don't work for function arguments

Suggested change
// NewFIFO creates a new First-In-First-Out cache of size [limit].
// NewFIFO creates a new First-In-First-Out cache of size limit.

//
// If a duplicate item is stored, it will not be requeued but its
// value will be changed.
func NewFIFO[K comparable, V any](limit int) (*FIFO[K, V], error) {
c := &FIFO[K, V]{
m: make(map[K]V, limit),
}
buf, err := buffer.NewBoundedQueue(limit, c.remove)
if err != nil {
return nil, err
}
c.buffer = buf
return c, nil
}

func (f *FIFO[K, V]) Put(key K, val V) bool {
f.l.Lock()
defer f.l.Unlock()

_, exists := f.m[key]
if !exists {
f.buffer.Push(key) // Push removes the oldest [K] if we are at the [limit]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f.buffer.Push(key) // Push removes the oldest [K] if we are at the [limit]
f.buffer.Push(key) // Push calls remove with the oldest key if we are at the limit

}
f.m[key] = val
return exists
}

func (f *FIFO[K, V]) Get(key K) (V, bool) {
f.l.RLock()
defer f.l.RUnlock()

v, ok := f.m[key]
return v, ok
}

// remove is used as the callback in [BoundedBuffer]. It is assumed that the
// [WriteLock] is held when this is accessed.
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// remove is used as the callback in [BoundedBuffer]. It is assumed that the
// [WriteLock] is held when this is accessed.
// remove is used as the callback in the bounded buffer. It is assumed that the
// write lock is held when this is called.

func (f *FIFO[K, V]) remove(key K) {
delete(f.m, key)
}
157 changes: 157 additions & 0 deletions cache/fifo_cache_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
// Copyright (C) 2023, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package cache

import (
"errors"
"testing"

"github.com/stretchr/testify/require"
)

func TestFIFOCacheInsertion(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of trivial - but should we test overwriting a key?

type put struct {
kv int
exists bool
}

type get struct {
k int
ok bool
}

tests := []struct {
name string
ops []interface{}
}{
{
name: "insert less than limit",
ops: []interface{}{
put{
kv: 0,
exists: false,
},
get{
k: 0,
ok: true,
},
},
},
{
name: "insert limit",
ops: []interface{}{
put{
kv: 0,
exists: false,
},
put{
kv: 1,
exists: false,
},
get{
k: 0,
ok: true,
},
get{
k: 1,
ok: true,
},
},
},
{
name: "exceed limit",
ops: []interface{}{
put{
kv: 0,
exists: false,
},
put{
kv: 1,
exists: false,
},
put{
kv: 2,
exists: false,
},
get{
k: 0,
ok: false,
},
get{
k: 1,
ok: true,
},
get{
k: 2,
ok: true,
},
},
},
{
name: "exceed limit + get ops maintains FIFO removal order",
ops: []interface{}{
put{
kv: 0,
exists: false,
},
put{
kv: 1,
exists: false,
},
get{
k: 0,
ok: true,
},
put{
kv: 2,
exists: false,
},
get{
k: 0,
ok: false,
},
get{
k: 1,
ok: true,
},
get{
k: 2,
ok: true,
},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require := require.New(t)

cache, err := NewFIFO[int, int](2)
require.NoError(err)

for _, opIntf := range tt.ops {
switch op := opIntf.(type) {
case put:
exists := cache.Put(op.kv, op.kv)
require.Equal(op.exists, exists)
case get:
val, ok := cache.Get(op.k)
require.Equal(op.ok, ok)
if ok {
require.Equal(op.k, val)
}
default:
require.Fail("op can only be a put or a get")
}
}
})
}
}

func TestEmptyCacheSizeInvalid(t *testing.T) {
require := require.New(t)
_, err := NewFIFO[int, int](0)
expectedErr := errors.New("maxSize must be greater than 0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than copying the text - I think we should export the error from NewBoundedQueue. Then we can use ErrorIs.

require.Equal(expectedErr, err)
}
20 changes: 20 additions & 0 deletions snow/engine/snowman/block/state_syncable_vm.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Linting seems to be failing here.

Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,23 @@
// [summaryHeight].
GetStateSummary(ctx context.Context, summaryHeight uint64) (StateSummary, error)
}

type StateSyncableVMDisabled struct{}

func (StateSyncableVMDisabled) StateSyncEnabled(context.Context) (bool, error) { return false, nil }

func (StateSyncableVMDisabled) GetOngoingSyncStateSummary(context.Context) (StateSummary, error) {
return nil, ErrStateSyncableVMNotImplemented
}

func (StateSyncableVMDisabled) GetLastStateSummary(context.Context) (StateSummary, error) {
return nil, ErrStateSyncableVMNotImplemented
}

func (StateSyncableVMDisabled) ParseStateSummary(ctx context.Context, summaryBytes []byte) (StateSummary, error) {

Check failure on line 59 in snow/engine/snowman/block/state_syncable_vm.go

View workflow job for this annotation

GitHub Actions / Lint

unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)

Check failure on line 59 in snow/engine/snowman/block/state_syncable_vm.go

View workflow job for this annotation

GitHub Actions / Lint

unused-parameter: parameter 'summaryBytes' seems to be unused, consider removing or renaming it as _ (revive)
return nil, ErrStateSyncableVMNotImplemented
}

func (StateSyncableVMDisabled) GetStateSummary(ctx context.Context, summaryHeight uint64) (StateSummary, error) {

Check failure on line 63 in snow/engine/snowman/block/state_syncable_vm.go

View workflow job for this annotation

GitHub Actions / Lint

unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)

Check failure on line 63 in snow/engine/snowman/block/state_syncable_vm.go

View workflow job for this annotation

GitHub Actions / Lint

unused-parameter: parameter 'summaryHeight' seems to be unused, consider removing or renaming it as _ (revive)
return nil, ErrStateSyncableVMNotImplemented
}
Loading