Skip to content
Open
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
105 changes: 101 additions & 4 deletions blockchain/blockchain.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here I worry about the tight window where db is updated and cache is not, we might have to synchronize them, or invalidate cache before starting writing to DB, so we don't read stale cache value, at the moment nothing hints cache is stale. We will still have concurrent DB read/write case but it might still worth

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A reader does a lock-free atomic load, so a reader is safe as it is. The pair {database commit, cache refresh} in a writer is not atomic, but the node has only one writer.

Store and RevertHead are stream callbacks on the same stream, and storeTask calls revertTask on that same goroutine. There is only one caller of Store in the tree.

Thus a mutex would protect an interleave that no call path can cause, and it would tell a future reader that this package expects concurrent writers. I prefer to keep the rule in the field comment, where a grep can check it. It would make sense adding a mutex when we'll get a second writer.

You also asked to set the cache directly instead of a read of the database. Store and Finalise can do this, because it writes exactly block.Number. RevertHead cannot: Blockchain does not know the new height unless it reads the database or repeats the rule "delete at genesis, else N-1" in a second place. RevertHead also cannot calculate the new height when the cache is already nil. Thus the choice is one read-back rule for all three methods, or a mixed scheme with the height rule in two places. I decided to keep the read-back.

"errors"
"iter"
"sync/atomic"

"github.com/NethermindEth/juno/blockchain/networks"
"github.com/NethermindEth/juno/blockchain/statebackend"
Expand Down Expand Up @@ -104,6 +105,14 @@
cachedFilters *AggregatedBloomFilterCache
runningFilter *core.RunningEventFilter
stateBackend statebackend.StateBackend

// chainHeight and l1Head mirror their database entries. Nil means "unknown", so a failed
// refresh sends readers to the database instead of serving a stale value. Only valid while
// db.ChainHeight and db.L1Height are written through Blockchain by one writer at a time: a
// migration on the raw db.KeyValueStore, or a concurrent store and revert, leave it stale.
chainHeight atomic.Pointer[uint64]
l1Head atomic.Pointer[core.L1Head]
cacheHeads bool
}

// options holds configuration for constructing a Blockchain.
Expand All @@ -112,6 +121,7 @@
stateVersion bool
runningFilterInitialize core.RunningEventFilterInitializer
retentionFloor *pruner.RetentionFloor
remoteDatabase bool
}

// Option is a functional option for configuring Blockchain options.
Expand Down Expand Up @@ -139,6 +149,15 @@
}
}

// WithRemoteDatabase marks the database as one another process writes, as `--remote-db` followers
// do. Such a node never stores or reverts, so it cannot know when the heads move and must read
// them back instead of caching them.
func WithRemoteDatabase() Option {
return func(o *options) {
o.remoteDatabase = true
}
}

// WithRetentionFloor shares a seeded retention floor (see
// [pruner.NewRetentionFloor]) with the state backend, so retention checks
// skip the database. The default unseeded floor probes the database instead.
Expand Down Expand Up @@ -167,7 +186,7 @@

runningFilter := core.NewRunningEventFilterLazy(database, o.runningFilterInitialize)

return &Blockchain{
chain := &Blockchain{
database: database,
network: network,
listener: o.listener,
Expand All @@ -181,7 +200,16 @@
o.retentionFloor,
o.stateVersion,
),
cacheHeads: !o.remoteDatabase,
}
if chain.cacheHeads {
chain.cacheChainHeight()
if l1Head, err := core.GetL1Head(database); err == nil {
chain.l1Head.Store(&l1Head)
}
}

return chain
}

func (b *Blockchain) Network() *networks.Network {
Expand All @@ -191,12 +219,36 @@
// Height returns the latest block height. If blockchain is empty nil is returned.
func (b *Blockchain) Height() (uint64, error) {
b.listener.OnRead("Height")
return b.height()
}

func (b *Blockchain) height() (uint64, error) {
if height := b.chainHeight.Load(); height != nil {
return *height, nil
}
return core.GetChainHeight(b.database)
}
Comment thread
brbrr marked this conversation as resolved.

// cacheChainHeight refreshes the cached height from the database rather than from the caller's
// block, so the cache stays derived from what was committed: reverting the genesis block removes
// the entry entirely instead of decrementing it. A read failure caches "unknown", which sends
// readers to the database.
func (b *Blockchain) cacheChainHeight() {
if !b.cacheHeads {
return

Check warning on line 238 in blockchain/blockchain.go

View check run for this annotation

Codecov / codecov/patch

blockchain/blockchain.go#L238

Added line #L238 was not covered by tests
}

height, err := core.GetChainHeight(b.database)
if err != nil {
b.chainHeight.Store(nil)
return
}
b.chainHeight.Store(&height)
}
Comment thread
brbrr marked this conversation as resolved.

func (b *Blockchain) Head() (*core.Block, error) {
b.listener.OnRead("Head")
curHeight, err := core.GetChainHeight(b.database)
curHeight, err := b.height()
if err != nil {
return nil, err
}
Expand All @@ -206,7 +258,7 @@

func (b *Blockchain) HeadsHeader() (*core.Header, error) {
b.listener.OnRead("HeadsHeader")
height, err := core.GetChainHeight(b.database)
height, err := b.height()
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -358,14 +410,47 @@
return L1HeadSubscription{b.l1HeadFeed.Subscribe()}
}

// L1Head returns the latest L1 head. The returned felts are shared with every other caller and
// must not be mutated in place.
func (b *Blockchain) L1Head() (core.L1Head, error) {
b.listener.OnRead("L1Head")
if l1Head := b.l1Head.Load(); l1Head != nil {
return *l1Head, nil
}
return core.GetL1Head(b.database)
}
Comment thread
brbrr marked this conversation as resolved.

func (b *Blockchain) SetL1Head(update *core.L1Head) error {
if err := core.WriteL1Head(b.database, update); err != nil {
return err
}
b.cacheL1Head(update)
b.l1HeadFeed.Send(update)
return core.WriteL1Head(b.database, update)

return nil
}

func (b *Blockchain) cacheL1Head(update *core.L1Head) {
if !b.cacheHeads {
return

Check warning on line 435 in blockchain/blockchain.go

View check run for this annotation

Codecov / codecov/patch

blockchain/blockchain.go#L435

Added line #L435 was not covered by tests
}

if update == nil {
b.l1Head.Store(nil)
return
}

// Deep copy: update and the felts it points at are shared with the feed's subscribers and
// outlive this call, while every later L1Head reader hands out what is cached here. Both
// felts are optional, so neither can be cloned unconditionally.
cached := core.L1Head{BlockNumber: update.BlockNumber}
Comment thread
brbrr marked this conversation as resolved.
if update.BlockHash != nil {
cached.BlockHash = update.BlockHash.Clone()
}
if update.StateRoot != nil {
cached.StateRoot = update.StateRoot.Clone()
}
b.l1Head.Store(&cached)
}

// Store takes a block and state update and performs sanity checks before putting in the database.
Expand All @@ -375,6 +460,8 @@
stateUpdate *core.StateUpdate,
newClasses map[felt.Felt]core.ClassDefinition,
) error {
defer b.cacheChainHeight()
Comment thread
brbrr marked this conversation as resolved.

return b.stateBackend.Store(block, blockCommitments, stateUpdate, newClasses)
}

Expand Down Expand Up @@ -433,6 +520,8 @@
preConfirmedFn func() (PreConfirmedReader, error),
) (EventFilterer, error) {
b.listener.OnRead("EventFilter")
// Do not use b.height() here. Events reads the height from the database on each call. Thus
// this bound and the range logic in Events use the same source.
latest, err := core.GetChainHeight(b.database)
if err != nil {
return nil, err
Expand All @@ -452,6 +541,12 @@

// RevertHead reverts the head block
func (b *Blockchain) RevertHead() error {
defer b.cacheChainHeight()

// Drop the cached height before the batch commits. A stale height outlives the block it names
// and would point readers at one that is already deleted; an unknown height sends them to the
// database, which is correct on both sides of the commit.
b.chainHeight.Store(nil)
return b.stateBackend.RevertHead()
}

Expand All @@ -477,6 +572,8 @@
newClasses map[felt.Felt]core.ClassDefinition,
sign core.BlockSignFunc,
) error {
defer b.cacheChainHeight()

return b.stateBackend.Finalise(block, stateUpdate, newClasses, sign)
}

Expand Down
Loading
Loading