-
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
Changes from 9 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
9759244
44a0e8a
6b8692f
a43f23a
b4b9267
4fe3602
b900fd1
962cb57
745fea9
aad23ac
3540422
84157c2
58ed219
8f243b2
69c6996
2e879bc
43d5d4c
c088d7e
fd641ae
2db87c4
fa3fc67
c513464
4dfe80b
f56f017
919e852
e0a910c
b0f1782
7f21c2b
23219b0
081381b
3f81853
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 |
---|---|---|
|
@@ -11,14 +11,14 @@ import ( | |
|
||
"github.com/ava-labs/hypersdk/codec" | ||
"github.com/ava-labs/hypersdk/consts" | ||
"github.com/ava-labs/hypersdk/internal/emap" | ||
"github.com/ava-labs/hypersdk/utils" | ||
) | ||
|
||
const InitialChunkSize = 250 * 1024 | ||
|
||
type Tx interface { | ||
GetID() ids.ID | ||
GetExpiry() int64 | ||
emap.Item | ||
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. This exposes 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. once merged, we won't need wrapping interface anymore. . unless I'm missing something ? 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 @joshua-kim 's point is to have the internal type implemented as makes sense in this package and then wrap it with another type that converts between that structure and the required interface when we need to use it in the validity window or expiry map where we need a specific interface. Correct me if I'm wrong @joshua-kim 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. done |
||
GetSponsor() codec.Address | ||
} | ||
|
||
|
@@ -51,6 +51,14 @@ func (c *Chunk[T]) init() error { | |
return nil | ||
} | ||
|
||
func (c Chunk[T]) GetID() ids.ID { | ||
return c.id | ||
} | ||
|
||
func (c Chunk[T]) GetExpiry() int64 { | ||
return c.Expiry | ||
} | ||
|
||
func signChunk[T Tx]( | ||
chunk UnsignedChunk[T], | ||
networkID uint32, | ||
|
@@ -106,9 +114,9 @@ func ParseChunk[T Tx](chunkBytes []byte) (Chunk[T], error) { | |
} | ||
|
||
type Block struct { | ||
ParentID ids.ID `serialize:"true"` | ||
Height uint64 `serialize:"true"` | ||
Timestamp int64 `serialize:"true"` | ||
ParentID ids.ID `serialize:"true"` | ||
Hght uint64 `serialize:"true"` | ||
Tmstmp int64 `serialize:"true"` | ||
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 dislike us naming fields like this because of the interface signature we're trying to implement... is there a way we can get around this? |
||
|
||
ChunkCerts []*ChunkCertificate `serialize:"true"` | ||
|
||
|
@@ -119,3 +127,28 @@ type Block struct { | |
func (b Block) GetID() ids.ID { | ||
return b.blkID | ||
} | ||
|
||
func (b Block) Parent() 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. I don't care which we pick, but we should be consistent on the naming of either 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 going to dodge this one by saying that this method is no longer needed. |
||
return b.ParentID | ||
} | ||
|
||
func (b Block) Timestamp() int64 { | ||
return b.Tmstmp | ||
} | ||
|
||
func (b Block) Height() uint64 { | ||
return b.Hght | ||
} | ||
|
||
func (b Block) Txs() []*ChunkCertificate { | ||
return b.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. This seems weird because these are not returning txs 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. yes, I agree. let's discuss this as a group ? 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. Can we use the term containers? |
||
|
||
func (b Block) Contains(id ids.ID) bool { | ||
for _, c := range b.ChunkCerts { | ||
if c.ChunkID == id { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
aaronbuchwald marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,7 +54,8 @@ type ChunkCertificate struct { | |
Signature NoVerifyChunkSignature `serialize:"true"` | ||
} | ||
|
||
func (c *ChunkCertificate) GetChunkID() ids.ID { return c.ChunkID } | ||
func (c ChunkCertificate) GetID() ids.ID { return c.ChunkID } | ||
func (c ChunkCertificate) GetExpiry() int64 { return c.Expiry } | ||
|
||
func (c *ChunkCertificate) GetSlot() int64 { return c.Expiry } | ||
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 these are duplicated - |
||
|
||
|
@@ -209,7 +210,7 @@ func ParseWarpChunkCertificate(b []byte) (*WarpChunkCertificate, error) { | |
}, nil | ||
} | ||
|
||
func (c *WarpChunkCertificate) GetChunkID() ids.ID { return c.UnsignedCertificate.ChunkID() } | ||
func (c *WarpChunkCertificate) GetID() ids.ID { return c.UnsignedCertificate.ChunkID() } | ||
|
||
func (c *WarpChunkCertificate) GetSlot() int64 { return c.UnsignedCertificate.Slot() } | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,12 +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" | ||
) | ||
|
@@ -30,6 +33,13 @@ var ( | |
ErrTimestampNotMonotonicallyIncreasing = errors.New("block timestamp must be greater than parent timestamp") | ||
) | ||
|
||
const validityWindowDuration = int64(5) | ||
|
||
type ( | ||
ChainIndex = validitywindow.ChainIndex[*ChunkCertificate] | ||
timeValidityWindow = *validitywindow.TimeValidityWindow[*ChunkCertificate] | ||
) | ||
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. Seems like we're doing this to avoid the caller depending on an internal package. I'm wondering if it even makes sense for 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. @aaronbuchwald asked to remove this section completely. |
||
|
||
type Validator struct { | ||
NodeID ids.NodeID | ||
Weight uint64 | ||
|
@@ -46,6 +56,9 @@ func New[T Tx]( | |
getChunkSignatureClient *p2p.Client, | ||
chunkCertificateGossipClient *p2p.Client, | ||
validators []Validator, | ||
log logging.Logger, | ||
tracer trace.Tracer, | ||
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: make these (logger + tracer) the first args in this fn |
||
chainIndex ChainIndex, | ||
) (*Node[T], error) { | ||
storage, err := newChunkStorage[T](NoVerifier[T]{}, memdb.New()) | ||
if err != nil { | ||
|
@@ -79,26 +92,34 @@ func New[T Tx]( | |
ChunkCertificateGossipHandler: &ChunkCertificateGossipHandler[T]{ | ||
storage: storage, | ||
}, | ||
storage: storage, | ||
storage: storage, | ||
log: log, | ||
tracer: tracer, | ||
validityWindow: validitywindow.NewTimeValidityWindow(log, tracer, chainIndex), | ||
}, nil | ||
} | ||
|
||
type Node[T Tx] struct { | ||
nodeID ids.NodeID | ||
networkID uint32 | ||
chainID ids.ID | ||
pk *bls.PublicKey | ||
signer warp.Signer | ||
getChunkClient *TypedClient[*dsmr.GetChunkRequest, Chunk[T], []byte] | ||
getChunkSignatureClient *TypedClient[*dsmr.GetChunkSignatureRequest, *dsmr.GetChunkSignatureResponse, []byte] | ||
chunkCertificateGossipClient *TypedClient[[]byte, []byte, *dsmr.ChunkCertificateGossip] | ||
validators []Validator | ||
|
||
GetChunkHandler *GetChunkHandler[T] | ||
GetChunkSignatureHandler *acp118.Handler | ||
ChunkCertificateGossipHandler *ChunkCertificateGossipHandler[T] | ||
storage *chunkStorage[T] | ||
} | ||
type ( | ||
Node[T Tx] struct { | ||
nodeID ids.NodeID | ||
networkID uint32 | ||
chainID ids.ID | ||
pk *bls.PublicKey | ||
signer warp.Signer | ||
getChunkClient *TypedClient[*dsmr.GetChunkRequest, Chunk[T], []byte] | ||
getChunkSignatureClient *TypedClient[*dsmr.GetChunkSignatureRequest, *dsmr.GetChunkSignatureResponse, []byte] | ||
chunkCertificateGossipClient *TypedClient[[]byte, []byte, *dsmr.ChunkCertificateGossip] | ||
validators []Validator | ||
validityWindow timeValidityWindow | ||
|
||
GetChunkHandler *GetChunkHandler[T] | ||
GetChunkSignatureHandler *acp118.Handler | ||
ChunkCertificateGossipHandler *ChunkCertificateGossipHandler[T] | ||
storage *chunkStorage[T] | ||
log logging.Logger | ||
tracer trace.Tracer | ||
} | ||
) | ||
|
||
// BuildChunk builds transactions into a Chunk | ||
// TODO handle frozen sponsor + validator assignments | ||
|
@@ -172,19 +193,28 @@ func (n *Node[T]) BuildChunk( | |
return chunk, n.storage.AddLocalChunkWithCert(chunk, &chunkCert) | ||
} | ||
|
||
func (n *Node[T]) BuildBlock(parent Block, timestamp int64) (Block, error) { | ||
if timestamp <= parent.Timestamp { | ||
// BuildBlock(ctx context.Context, parentView state.View, parent *ExecutionBlock) (*ExecutionBlock, *ExecutedBlock, merkledb.View, error) | ||
aaronbuchwald marked this conversation as resolved.
Show resolved
Hide resolved
|
||
func (n *Node[T]) BuildBlock(ctx context.Context, parent Block, timestamp int64) (Block, error) { | ||
if timestamp <= parent.Tmstmp { | ||
return Block{}, ErrTimestampNotMonotonicallyIncreasing | ||
} | ||
|
||
chunkCerts := n.storage.GatherChunkCerts() | ||
oldestAllowed := timestamp - validityWindowDuration | ||
if oldestAllowed < 0 { | ||
oldestAllowed = 0 | ||
} | ||
dup, err := n.validityWindow.IsRepeat(ctx, parent, chunkCerts, oldestAllowed) | ||
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 { | ||
|
@@ -193,8 +223,8 @@ func (n *Node[T]) BuildBlock(parent Block, timestamp int64) (Block, error) { | |
|
||
blk := Block{ | ||
ParentID: parent.GetID(), | ||
Height: parent.Height + 1, | ||
Timestamp: timestamp, | ||
Hght: parent.Hght + 1, | ||
Tmstmp: timestamp, | ||
ChunkCerts: availableChunkCerts, | ||
} | ||
|
||
|
@@ -208,9 +238,14 @@ func (n *Node[T]) BuildBlock(parent Block, timestamp int64) (Block, error) { | |
return blk, nil | ||
} | ||
|
||
func (*Node[T]) Execute(ctx context.Context, _ Block, block Block) error { | ||
func (n *Node[T]) Execute(ctx context.Context, parentBlock Block, block Block) error { | ||
// TODO: Verify header fields | ||
// TODO: de-duplicate chunk certificates (internal to block and across history) | ||
|
||
// Find repeats | ||
if err := n.validityWindow.VerifyExpiryReplayProtection(ctx, block, parentBlock.Tmstmp); 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 { | ||
// TODO: verify chunks within a provided context | ||
if err := chunkCert.Verify(ctx, struct{}{}); err != nil { | ||
|
@@ -263,6 +298,8 @@ func (n *Node[T]) Accept(ctx context.Context, block Block) error { | |
} | ||
} | ||
} | ||
// update the validity window with the accepted block. | ||
n.validityWindow.Accept(block) | ||
|
||
return n.storage.SetMin(block.Timestamp, chunkIDs) | ||
return n.storage.SetMin(block.Tmstmp, chunkIDs) | ||
} |
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.
same comment on
tx.GetID()
here