-
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 18 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 |
---|---|---|
|
@@ -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" | ||
|
@@ -69,6 +70,15 @@ func (v *TimeValidityWindow[Container]) VerifyExpiryReplayProtection( | |
if dup.Len() > 0 { | ||
return fmt.Errorf("%w: duplicate in ancestry", ErrDuplicateContainer) | ||
} | ||
// make sure we have no repeats within the block itself. | ||
blkTxsIDs := make(map[ids.ID]bool, len(blk.Txs())) | ||
for _, tx := range blk.Txs() { | ||
id := tx.GetID() | ||
if _, has := blkTxsIDs[id]; has { | ||
return fmt.Errorf("%w: duplicate in block", ErrDuplicateContainer) | ||
} | ||
blkTxsIDs[id] = 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. We do this check twice now in chain since we previously handled this within We should only apply the check once, which do you think is the better place for it? The other addition within 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 see why testing this twice is an issue on it's own, and I think that I have a better solution here; func (b *ExecutionBlock) ValidateDuplicateTransactions() error {
if len(b.Txs) != b.txsSet.Len() {
ErrDuplicateTx
}
return nil than in validitywindow.go ...
if err := blk.ValidateDuplicateTransactions(); err != nil {
return err
}
... and remove the test from |
||
return nil | ||
} | ||
|
||
|
@@ -110,7 +120,7 @@ func (v *TimeValidityWindow[Container]) isRepeat( | |
if marker.Contains(i) { | ||
continue | ||
} | ||
if ancestorBlk.ContainsTx(tx.GetID()) { | ||
if ancestorBlk.Contains(tx.GetID()) { | ||
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. same comment on |
||
marker.Add(i) | ||
if stop { | ||
return marker, nil | ||
|
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,22 +14,31 @@ 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" | ||
) | ||
|
||
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") | ||
) | ||
|
||
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 +55,10 @@ 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, | ||
validityWindowDuration int64, | ||
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. Should we use |
||
) (*Node[T], error) { | ||
storage, err := newChunkStorage[T](NoVerifier[T]{}, memdb.New()) | ||
if err != nil { | ||
|
@@ -79,26 +92,36 @@ func New[T Tx]( | |
ChunkCertificateGossipHandler: &ChunkCertificateGossipHandler[T]{ | ||
storage: storage, | ||
}, | ||
storage: storage, | ||
storage: storage, | ||
log: log, | ||
tracer: tracer, | ||
validityWindow: validitywindow.NewTimeValidityWindow(log, tracer, chainIndex), | ||
validityWindowDuration: validityWindowDuration, | ||
}, 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 | ||
validityWindowDuration int64 | ||
} | ||
) | ||
aaronbuchwald marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// BuildChunk builds transactions into a Chunk | ||
// TODO handle frozen sponsor + validator assignments | ||
|
@@ -172,29 +195,40 @@ 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 { | ||
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 - n.validityWindowDuration | ||
if oldestAllowed < 0 { | ||
oldestAllowed = 0 | ||
} | ||
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: |
||
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 { | ||
if dup.Len() == len(chunkCerts) && len(chunkCerts) > 0 { | ||
return Block{}, ErrAllChunkCertsDuplicate | ||
} | ||
return Block{}, ErrNoAvailableChunkCerts | ||
aaronbuchwald marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
blk := Block{ | ||
ParentID: parent.GetID(), | ||
Height: parent.Height + 1, | ||
Timestamp: timestamp, | ||
Hght: parent.Hght + 1, | ||
Tmstmp: timestamp, | ||
ChunkCerts: availableChunkCerts, | ||
} | ||
|
||
|
@@ -208,9 +242,22 @@ 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) | ||
if block.Tmstmp <= parentBlock.Tmstmp && parentBlock.Hght > 0 { | ||
return ErrTimestampNotMonotonicallyIncreasing | ||
} | ||
|
||
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 remove this check from this PR? We should enforce timestamp >= parent timestamp (should allow them to be equal in case a block builder pushes the timestamp ahead of wall clock time for some nodes. We should not allow the case that a malicious node builds a block with a timestamp that we consider valid less than 1s ahead of our wall clock time, but still ahead of our wall clock time, such that when we build a block on top of it, we fail because the current timestamp is ahead of our local timestamp. We should update the check applied in We should also never execute the genesis block, so the check for |
||
// Find repeats | ||
|
||
oldestAllowed := block.Timestamp() - n.validityWindowDuration | ||
if oldestAllowed < 0 { | ||
oldestAllowed = 0 | ||
} | ||
if err := n.validityWindow.VerifyExpiryReplayProtection(ctx, 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 { | ||
// TODO: verify chunks within a provided context | ||
if err := chunkCert.Verify(ctx, struct{}{}); err != nil { | ||
|
@@ -263,6 +310,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.
Can we use
set.Set
here?