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

Separate snow package from snow vm refactor #1846

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 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
202 changes: 202 additions & 0 deletions chainstore/chain_store.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package chainstore
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Is this intended to be the index? We should be consistent with whatever name we choose - in some parts of this PR we're referring to this as chainstore, ChainIndex, and BlockChainIndex.
  2. Should this live in the same package as Chain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Renaming the package to chainindex
  2. Are you referring to snow.Chain or chain/ ? I think this can be its own package that fulfills the dependency required by snow.Chain

Going with the following breakdown:

  • snow.Application provides the application logic exposed to the chain developer
  • snow.ConsensusIndex provides the cached index from consensus + output/accepted types of the preferred/last accepted block
  • ChainIndex provides an on-disk index for a single type ie. input block type


import (
"context"
"encoding/binary"
"errors"
"fmt"
"math/rand"
"time"

"github.com/ava-labs/avalanchego/database"
"github.com/ava-labs/avalanchego/ids"
"github.com/ava-labs/avalanchego/utils/logging"
"go.uber.org/zap"

"github.com/ava-labs/hypersdk/consts"

hcontext "github.com/ava-labs/hypersdk/context"
)

const (
blockPrefix byte = 0x0 // TODO: move to flat files (https://github.com/ava-labs/hypersdk/issues/553)
blockIDHeightPrefix byte = 0x1 // ID -> Height
blockHeightIDPrefix byte = 0x2 // Height -> ID (don't always need full block from disk)
lastAcceptedByte byte = 0x3 // lastAcceptedByte -> lastAcceptedHeight
)

type Config struct {
AcceptedBlockWindow uint64 `json:"acceptedBlockWindow"`
BlockCompactionAverageFrequency int `json:"blockCompactionFrequency"`
}

func NewDefaultConfig() Config {
return Config{
AcceptedBlockWindow: 50_000, // ~3.5hr with 250ms block time (100GB at 2MB)
BlockCompactionAverageFrequency: 32, // 64 MB of deletion if 2 MB blocks
}
}

type ChainStore[T Block] struct {
Copy link
Contributor

@joshua-kim joshua-kim Jan 2, 2025

Choose a reason for hiding this comment

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

If this type is going to be the default implementation for the index it might make sense for us to also include unit tests on it as part of this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add tests for this

config Config
metrics *metrics
log logging.Logger
db database.Database
parser Parser[T]
}

type Block interface {
ID() ids.ID
Height() uint64
Bytes() []byte
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
}

type Parser[T Block] interface {
ParseBlock(context.Context, []byte) (T, error)
}

func New[T Block](
hctx *hcontext.Context,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should avoid passing hcontext.Context and just pass in the required dependencies individually. I feel like hcontext.Context makes it too easy to pass around global state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm fine with passing in explicit dependencies. I think this can provide a nice-to-have convenience and added it after a few conversation with @darioush , but not necessary here.

namespace string,
parser Parser[T],
db database.Database,
) (*ChainStore[T], error) {
registry, err := hctx.MakeRegistry(namespace)
if err != nil {
return nil, err
}
metrics, err := newMetrics(registry)
if err != nil {
return nil, err
}
config, err := hcontext.GetConfigFromContext(hctx, namespace, NewDefaultConfig())
if err != nil {
return nil, err
}

return &ChainStore[T]{
config: config,
metrics: metrics,
log: hctx.Log(),
db: db,
parser: parser,
}, nil
}

func (c *ChainStore[T]) GetLastAcceptedHeight(_ context.Context) (uint64, error) {
lastAcceptedHeightBytes, err := c.db.Get([]byte{lastAcceptedByte})
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it might make sense to just make this byte array a var since we're frequently re-using it

if err != nil {
return 0, err
}
return database.ParseUInt64(lastAcceptedHeightBytes)
}

func (c *ChainStore[T]) UpdateLastAccepted(_ context.Context, blk T) error {
batch := c.db.NewBatch()

var (
blkID = blk.ID()
height = blk.Height()
blkBytes = blk.Bytes()
)
heightBytes := binary.BigEndian.AppendUint64(nil, height)
err := errors.Join(
batch.Put([]byte{lastAcceptedByte}, heightBytes),
batch.Put(PrefixBlockIDHeightKey(blkID), heightBytes),
batch.Put(PrefixBlockHeightIDKey(blk.Height()), blkID[:]),
batch.Put(PrefixBlockKey(height), blkBytes),
)
if err != nil {
return err
}
expiryHeight := height - c.config.AcceptedBlockWindow
var expired bool
if expiryHeight > 0 && expiryHeight < height { // ensure we don't free genesis
if err := batch.Delete(PrefixBlockKey(expiryHeight)); err != nil {
return err
}
blkID, err := c.db.Get(PrefixBlockHeightIDKey(expiryHeight))
if err != nil {
return fmt.Errorf("unable to fetch blockID at height %d: %w", expiryHeight, err)
}
if err := batch.Delete(PrefixBlockIDHeightKey(ids.ID(blkID))); err != nil {
return err
}
if err := batch.Delete(PrefixBlockHeightIDKey(expiryHeight)); err != nil {
return err
}
expired = true
c.metrics.deletedBlocks.Inc()
}
//nolint:gosec
if expired && rand.Intn(c.config.BlockCompactionAverageFrequency) == 0 {
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe for multiple Compact operations to happen concurrently? We don't block between them but I'm not sure what the invariant is on Compact since we don't mention much on the avalanchego interface comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a great question. I don't see anything in leveldb/pebbledb that explicitly disallows it.

I would say this is not changed in this PR and therefore out of scope, but I actually changed from randomly selecting an offset to to compact every time at to choosing a random number each time, which could lead to multiple goroutines happening where it did not before. I'll revert to the previous behavior for now, so it's not changed in this PR.

Would need to dig deeper into leveldb/pebbledb to see how this will be handled.

Copy link
Collaborator Author

@aaronbuchwald aaronbuchwald Jan 3, 2025

Choose a reason for hiding this comment

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

ref

compactionOffset = rand.Intn(vm.config.BlockCompactionFrequency) //nolint:gosec

start := time.Now()
if err := c.db.Compact([]byte{blockPrefix}, PrefixBlockKey(expiryHeight)); err != nil {
c.log.Error("failed to compact block store", zap.Error(err))
return
}
c.log.Info("compacted disk blocks", zap.Uint64("end", expiryHeight), zap.Duration("t", time.Since(start)))
}()
}

return batch.Write()
}

func (c *ChainStore[T]) GetBlock(ctx context.Context, blkID ids.ID) (T, error) {
var emptyT T
height, err := c.GetBlockIDHeight(ctx, blkID)
if err != nil {
return emptyT, err
}
return c.GetBlockByHeight(ctx, height)
}

func (c *ChainStore[T]) GetBlockIDAtHeight(_ context.Context, blkHeight uint64) (ids.ID, error) {
blkIDBytes, err := c.db.Get(PrefixBlockHeightIDKey(blkHeight))
if err != nil {
return ids.Empty, err
}
return ids.ID(blkIDBytes), nil
}

func (c *ChainStore[T]) GetBlockIDHeight(_ context.Context, blkID ids.ID) (uint64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this + GetBlockIDAtHeight? Seems like we can get this from GetBlock since the height is recorded as part of the block metadata

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This reads a different index with smaller key-value pairs and is migrating existing functionality, so would prefer not to change in this PR.

blkHeightBytes, err := c.db.Get(PrefixBlockIDHeightKey(blkID))
if err != nil {
return 0, err
}
return database.ParseUInt64(blkHeightBytes)
}

func (c *ChainStore[T]) GetBlockByHeight(ctx context.Context, blkHeight uint64) (T, error) {
var emptyT T
blkBytes, err := c.db.Get(PrefixBlockKey(blkHeight))
if err != nil {
return emptyT, err
}
return c.parser.ParseBlock(ctx, blkBytes)
}

func PrefixBlockKey(height uint64) []byte {
k := make([]byte, 1+consts.Uint64Len)
k[0] = blockPrefix
binary.BigEndian.PutUint64(k[1:], height)
return k
}

func PrefixBlockIDHeightKey(id ids.ID) []byte {
k := make([]byte, 1+ids.IDLen)
k[0] = blockIDHeightPrefix
copy(k[1:], id[:])
return k
}

func PrefixBlockHeightIDKey(height uint64) []byte {
k := make([]byte, 1+consts.Uint64Len)
k[0] = blockHeightIDPrefix
binary.BigEndian.PutUint64(k[1:], height)
return k
}
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 26 additions & 0 deletions chainstore/metrics.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package chainstore

import "github.com/prometheus/client_golang/prometheus"

type metrics struct {
deletedBlocks prometheus.Counter
}

func newMetrics(registry prometheus.Registerer) (*metrics, error) {
m := &metrics{
deletedBlocks: prometheus.NewCounter(prometheus.CounterOpts{
Namespace: "chainstore",
Name: "deleted_blocks",
Help: "Number of blocks deleted from the chain",
}),
}

if err := registry.Register(m.deletedBlocks); err != nil {
return nil, err
}

return m, nil
}
38 changes: 38 additions & 0 deletions context/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright (C) 2024, Ava Labs, Inv. All rights reserved.
// See the file LICENSE for licensing terms.

package context

import "encoding/json"

type Config map[string]json.RawMessage

func NewConfig(b []byte) (Config, error) {
c := Config{}
if len(b) > 0 {
if err := json.Unmarshal(b, &c); err != nil {
return nil, err
}
}
return c, nil
}

func (c Config) Get(key string) ([]byte, bool) {
if val, ok := c[key]; ok {
return val, true
}
return nil, false
}

func GetConfig[T any](c Config, key string, defaultConfig T) (T, error) {
val, ok := c[key]
if !ok {
return defaultConfig, nil
}

var emptyConfig T
if err := json.Unmarshal(val, &defaultConfig); err != nil {
return emptyConfig, err
}
return defaultConfig, nil
}
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
79 changes: 79 additions & 0 deletions context/config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package context

import (
"testing"

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

type testConfig struct {
TxFee uint64 `json:"txFee"`
MinFee uint64 `json:"minFee"`
}

func TestConfigC(t *testing.T) {
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
type test struct {
name string
providedStr string
defaultConfig testConfig
wantConfig testConfig
}
for _, test := range []test{
{
name: "default want non-zero values",
providedStr: "",
defaultConfig: testConfig{TxFee: 100},
wantConfig: testConfig{TxFee: 100},
},
{
name: "default want zero values",
providedStr: "",
defaultConfig: testConfig{},
wantConfig: testConfig{},
},
{
name: "override default with zero values",
providedStr: `{
"test": {
"txFee": 0,
"minFee": 0
}
}`,
defaultConfig: testConfig{TxFee: 100, MinFee: 100},
wantConfig: testConfig{TxFee: 0, MinFee: 0},
},
{
name: "override non-zero defaults",
providedStr: `{
"test": {
"txFee": 1000,
"minFee": 1000
}
}`,
defaultConfig: testConfig{TxFee: 100, MinFee: 100},
wantConfig: testConfig{TxFee: 1000, MinFee: 1000},
},
{
name: "override one default value",
providedStr: `{
"test": {
"txFee": 1000
}
}`,
defaultConfig: testConfig{TxFee: 100, MinFee: 100},
wantConfig: testConfig{TxFee: 1000, MinFee: 100},
},
} {
t.Run(test.name, func(t *testing.T) {
r := require.New(t)
c, err := NewConfig([]byte(test.providedStr))
r.NoError(err)
testConfig, err := GetConfig(c, "test", test.defaultConfig)
r.NoError(err)
r.Equal(test.wantConfig, testConfig)
})
}
}
Loading
Loading