-
Notifications
You must be signed in to change notification settings - Fork 118
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
Generalize expiry based de-duplication, dsmr #1810
base: main
Are you sure you want to change the base?
Changes from 40 commits
e5ecf69
ba644c3
2939543
08f0d59
659f1fb
921b908
508cc92
7a3fed6
b312a3c
1238460
8c1ab98
f8e42a8
b000edf
1d2536e
6911f26
f2816e7
e78f847
270245a
144ddd4
2fa701d
2220b5e
39905b9
d54e1a0
8e83b6d
fdd4639
9e127f1
0f3fe73
fc87f31
cf82891
864acc1
d96160e
9ea09b4
30bcdce
8d52c05
32207b9
5d62af7
7ca779b
383a7ef
7902a6a
1f143ea
6d4f94c
dd419bb
c87b033
293c6b9
b501cec
68603db
7e72626
7edef6e
185fbf0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import ( | |
"fmt" | ||
"sync" | ||
|
||
"github.com/ava-labs/avalanchego/ids" | ||
"github.com/ava-labs/avalanchego/trace" | ||
"github.com/ava-labs/avalanchego/utils/logging" | ||
"github.com/ava-labs/avalanchego/utils/set" | ||
|
@@ -45,7 +46,7 @@ func (v *TimeValidityWindow[Container]) Accept(blk ExecutionBlock[Container]) { | |
|
||
evicted := v.seen.SetMin(blk.Timestamp()) | ||
v.log.Debug("txs evicted from seen", zap.Int("len", len(evicted))) | ||
v.seen.Add(blk.Txs()) | ||
v.seen.Add(blk.Containers()) | ||
v.lastAcceptedBlockHeight = blk.Height() | ||
} | ||
|
||
|
@@ -62,13 +63,23 @@ func (v *TimeValidityWindow[Container]) VerifyExpiryReplayProtection( | |
return err | ||
} | ||
|
||
dup, err := v.isRepeat(ctx, parent, oldestAllowed, blk.Txs(), true) | ||
dup, err := v.isRepeat(ctx, parent, oldestAllowed, blk.Containers(), true) | ||
if err != nil { | ||
return err | ||
} | ||
if dup.Len() > 0 { | ||
return fmt.Errorf("%w: duplicate in ancestry", ErrDuplicateContainer) | ||
} | ||
// make sure we have no repeats within the block itself. | ||
// set.Set | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: typo? Looks like we meant to remove this line |
||
blkContainerIDs := set.NewSet[ids.ID](len(blk.Containers())) | ||
for _, container := range blk.Containers() { | ||
id := container.GetID() | ||
if blkContainerIDs.Contains(id) { | ||
return fmt.Errorf("%w: duplicate in block", ErrDuplicateContainer) | ||
} | ||
blkContainerIDs.Add(id) | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -85,7 +96,7 @@ func (v *TimeValidityWindow[Container]) isRepeat( | |
ctx context.Context, | ||
ancestorBlk ExecutionBlock[Container], | ||
oldestAllowed int64, | ||
txs []Container, | ||
containers []Container, | ||
stop bool, | ||
) (set.Bits, error) { | ||
marker := set.NewBits() | ||
|
@@ -103,14 +114,14 @@ func (v *TimeValidityWindow[Container]) isRepeat( | |
} | ||
|
||
if ancestorBlk.Height() <= v.lastAcceptedBlockHeight || ancestorBlk.Height() == 0 { | ||
return v.seen.Contains(txs, marker, stop), nil | ||
return v.seen.Contains(containers, marker, stop), nil | ||
} | ||
|
||
for i, tx := range txs { | ||
for i, container := range containers { | ||
if marker.Contains(i) { | ||
continue | ||
} | ||
if ancestorBlk.ContainsTx(tx.GetID()) { | ||
if ancestorBlk.Contains(container.GetID()) { | ||
marker.Add(i) | ||
if stop { | ||
return marker, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ package dsmr | |
import ( | ||
"github.com/ava-labs/avalanchego/ids" | ||
"github.com/ava-labs/avalanchego/utils/crypto/bls" | ||
"github.com/ava-labs/avalanchego/utils/set" | ||
"github.com/ava-labs/avalanchego/utils/wrappers" | ||
"github.com/ava-labs/avalanchego/vms/platformvm/warp" | ||
|
||
|
@@ -105,6 +106,47 @@ func ParseChunk[T Tx](chunkBytes []byte) (Chunk[T], error) { | |
return c, c.init() | ||
} | ||
|
||
// ExecutionBlock bridge the gap between the dsmr's block implementation and the validity window's execution block interface. | ||
type ExecutionBlock struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, wasn't aware of that, and would gladly rename the data structure. |
||
innerBlock Block | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be fine to just embed |
||
certSet set.Set[ids.ID] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: rename to |
||
} | ||
|
||
func (e ExecutionBlock) Timestamp() int64 { | ||
return e.innerBlock.Timestamp | ||
} | ||
|
||
func (e ExecutionBlock) Height() uint64 { | ||
return e.innerBlock.Height | ||
} | ||
|
||
func (e ExecutionBlock) Contains(id ids.ID) bool { | ||
return e.certSet.Contains(id) | ||
} | ||
|
||
func (e ExecutionBlock) Parent() ids.ID { | ||
return e.innerBlock.ParentID | ||
} | ||
|
||
func (e ExecutionBlock) Containers() []*emapChunkCertificate { | ||
emapChunkCert := make([]*emapChunkCertificate, len(e.innerBlock.ChunkCerts)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: rename to |
||
for i := range emapChunkCert { | ||
emapChunkCert[i] = &emapChunkCertificate{*e.innerBlock.ChunkCerts[i]} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind to change it to "append" style, just be aware of https://stackoverflow.com/questions/38654729/golang-slice-append-vs-assign-performance |
||
} | ||
return emapChunkCert | ||
} | ||
|
||
func NewExecutionBlock(innerBlock Block) ExecutionBlock { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: unexport, this should never be used outside of the package because this is a type internal to this package |
||
certSet := set.Set[ids.ID]{} | ||
for _, c := range innerBlock.ChunkCerts { | ||
certSet.Add(c.ChunkID) | ||
} | ||
return ExecutionBlock{ | ||
innerBlock: innerBlock, | ||
certSet: certSet, | ||
} | ||
} | ||
|
||
type Block struct { | ||
ParentID ids.ID `serialize:"true"` | ||
Height uint64 `serialize:"true"` | ||
|
@@ -116,6 +158,15 @@ type Block struct { | |
blkBytes []byte | ||
} | ||
|
||
func NewBlock(parentID ids.ID, height uint64, timestamp int64, chunkCerts []*ChunkCertificate) Block { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I don't think we need this diff? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that you don't like intuitive constructors... I'll roll back this change. ( we use it in only a single place ). |
||
return Block{ | ||
ParentID: parentID, | ||
Height: height, | ||
Timestamp: timestamp, | ||
ChunkCerts: chunkCerts, | ||
} | ||
} | ||
|
||
func (b Block) GetID() ids.ID { | ||
return b.blkID | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,13 +14,15 @@ import ( | |
"github.com/ava-labs/avalanchego/ids" | ||
"github.com/ava-labs/avalanchego/network/p2p" | ||
"github.com/ava-labs/avalanchego/network/p2p/acp118" | ||
"github.com/ava-labs/avalanchego/trace" | ||
"github.com/ava-labs/avalanchego/utils/crypto/bls" | ||
"github.com/ava-labs/avalanchego/utils/logging" | ||
"github.com/ava-labs/avalanchego/utils/wrappers" | ||
"github.com/ava-labs/avalanchego/vms/platformvm/warp" | ||
|
||
"github.com/ava-labs/hypersdk/codec" | ||
"github.com/ava-labs/hypersdk/consts" | ||
"github.com/ava-labs/hypersdk/internal/validitywindow" | ||
"github.com/ava-labs/hypersdk/proto/pb/dsmr" | ||
"github.com/ava-labs/hypersdk/utils" | ||
|
||
|
@@ -36,6 +38,7 @@ var ( | |
|
||
ErrEmptyChunk = errors.New("empty chunk") | ||
ErrNoAvailableChunkCerts = errors.New("no available chunk certs") | ||
ErrAllChunkCertsDuplicate = errors.New("all chunk certs are duplicated") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is there a specific error type to all chunk certs being duplicated? Shouldn't the verification fail if any chunk cert is a duplicate? Also I don't think this error type is used |
||
ErrTimestampNotMonotonicallyIncreasing = errors.New("block timestamp must be greater than parent timestamp") | ||
ErrEmptyBlock = errors.New("block must reference chunks") | ||
ErrInvalidBlockParent = errors.New("invalid referenced block parent") | ||
|
@@ -54,6 +57,7 @@ type Validator struct { | |
|
||
func New[T Tx]( | ||
log logging.Logger, | ||
tracer trace.Tracer, | ||
nodeID ids.NodeID, | ||
networkID uint32, | ||
chainID ids.ID, | ||
|
@@ -70,6 +74,8 @@ func New[T Tx]( | |
lastAccepted Block, | ||
quorumNum uint64, | ||
quorumDen uint64, | ||
chainIndex validitywindow.ChainIndex[*emapChunkCertificate], | ||
validityWindowDuration time.Duration, | ||
) (*Node[T], error) { | ||
return &Node[T]{ | ||
ID: nodeID, | ||
|
@@ -88,6 +94,10 @@ func New[T Tx]( | |
GetChunkSignatureHandler: getChunkSignatureHandler, | ||
ChunkCertificateGossipHandler: chunkCertificateGossipHandler, | ||
storage: chunkStorage, | ||
log: log, | ||
tracer: tracer, | ||
validityWindow: validitywindow.NewTimeValidityWindow(log, tracer, chainIndex), | ||
validityWindowDuration: validityWindowDuration, | ||
}, nil | ||
} | ||
|
||
|
@@ -109,6 +119,10 @@ type Node[T Tx] struct { | |
GetChunkSignatureHandler p2p.Handler | ||
ChunkCertificateGossipHandler p2p.Handler | ||
storage *ChunkStorage[T] | ||
log logging.Logger | ||
tracer trace.Tracer | ||
validityWindowDuration time.Duration | ||
validityWindow *validitywindow.TimeValidityWindow[*emapChunkCertificate] | ||
} | ||
|
||
// BuildChunk builds transactions into a Chunk | ||
|
@@ -217,31 +231,41 @@ func (n *Node[T]) BuildChunk( | |
return chunk, chunkCert, n.storage.AddLocalChunkWithCert(chunk, &chunkCert) | ||
} | ||
|
||
func (n *Node[T]) BuildBlock(parent Block, timestamp int64) (Block, error) { | ||
func (n *Node[T]) BuildBlock(ctx context.Context, parent Block, timestamp int64) (Block, error) { | ||
if timestamp <= parent.Timestamp { | ||
return Block{}, ErrTimestampNotMonotonicallyIncreasing | ||
} | ||
|
||
chunkCerts := n.storage.GatherChunkCerts() | ||
oldestAllowed := max(0, timestamp-int64(n.validityWindowDuration)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spacing is inconsistent, delete line |
||
emapChunkCert := make([]*emapChunkCertificate, len(chunkCerts)) | ||
for i := range emapChunkCert { | ||
emapChunkCert[i] = &emapChunkCertificate{*chunkCerts[i]} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar comment to before of using |
||
dup, err := n.validityWindow.IsRepeat(ctx, NewExecutionBlock(parent), emapChunkCert, oldestAllowed) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I had to inspect the implementation to realize this is a bitset... can we just call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok changing this, just please note that the same applied to other placed in the codebase that are already using the |
||
if err != nil { | ||
return Block{}, err | ||
} | ||
|
||
availableChunkCerts := make([]*ChunkCertificate, 0) | ||
for _, chunkCert := range chunkCerts { | ||
// avoid building blocks with expired chunk certs | ||
if chunkCert.Expiry < timestamp { | ||
for i, chunkCert := range chunkCerts { | ||
// avoid building blocks with duplicate or expired chunk certs | ||
if chunkCert.Expiry < timestamp || dup.Contains(i) { | ||
continue | ||
} | ||
|
||
availableChunkCerts = append(availableChunkCerts, chunkCert) | ||
} | ||
if len(availableChunkCerts) == 0 { | ||
return Block{}, ErrNoAvailableChunkCerts | ||
} | ||
|
||
blk := Block{ | ||
ParentID: parent.GetID(), | ||
Height: parent.Height + 1, | ||
Timestamp: timestamp, | ||
ChunkCerts: availableChunkCerts, | ||
} | ||
blk := NewBlock( | ||
parent.GetID(), | ||
parent.Height+1, | ||
timestamp, | ||
availableChunkCerts, | ||
) | ||
|
||
packer := wrappers.Packer{Bytes: make([]byte, 0, InitialChunkSize), MaxSize: consts.NetworkSizeLimit} | ||
if err := codec.LinearCodec.MarshalInto(blk, &packer); err != nil { | ||
|
@@ -281,6 +305,13 @@ func (n *Node[T]) Verify(ctx context.Context, parent Block, block Block) error { | |
return fmt.Errorf("%w: %s", ErrEmptyBlock, block.GetID()) | ||
} | ||
|
||
// Find repeats | ||
oldestAllowed := max(0, block.Timestamp-int64(n.validityWindowDuration)) | ||
|
||
if err := n.validityWindow.VerifyExpiryReplayProtection(ctx, NewExecutionBlock(block), oldestAllowed); err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should be wrapping this error to check for the duplicate case in tests |
||
} | ||
|
||
for _, chunkCert := range block.ChunkCerts { | ||
if err := chunkCert.Verify( | ||
ctx, | ||
|
@@ -340,6 +371,8 @@ func (n *Node[T]) Accept(ctx context.Context, block Block) error { | |
} | ||
} | ||
} | ||
// update the validity window with the accepted block. | ||
n.validityWindow.Accept(NewExecutionBlock(block)) | ||
|
||
if err := n.storage.SetMin(block.Timestamp, chunkIDs); err != nil { | ||
return fmt.Errorf("failed to prune chunks: %w", err) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this rename needed as part of this change? The increased usage of the term container is confusing to me - it's a new term we're using internally in the codebase, and it also is weird because even though it's a generic type
T
of a evict-able item we refer to it as a container even though it's the emap interface doesn't imply that it contains anything at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requested by @aaronbuchwald