-
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 24 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 |
---|---|---|
|
@@ -39,10 +39,11 @@ func (s *Syncer[Container]) start(ctx context.Context, lastAcceptedBlock Executi | |
seenValidityWindow = false | ||
validityWindow = s.getValidityWindow(lastAcceptedBlock.Timestamp()) | ||
err error | ||
hasBlock bool | ||
) | ||
for { | ||
parent, err = s.chainIndex.GetExecutionBlock(ctx, parent.Parent()) | ||
if err != nil { | ||
parent, hasBlock, err = s.chainIndex.GetExecutionBlock(ctx, parent.Parent()) | ||
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: |
||
if err != nil || !hasBlock { | ||
break // If we can't fetch far enough back or we've gone past genesis, execute what we can | ||
} | ||
parents = append(parents, parent) | ||
|
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" | ||
|
@@ -57,8 +58,8 @@ func (v *TimeValidityWindow[Container]) VerifyExpiryReplayProtection( | |
if blk.Height() <= v.lastAcceptedBlockHeight { | ||
return nil | ||
} | ||
parent, err := v.chainIndex.GetExecutionBlock(ctx, blk.Parent()) | ||
if err != nil { | ||
parent, hasBlock, err := v.chainIndex.GetExecutionBlock(ctx, blk.Parent()) | ||
if err != nil || !hasBlock { | ||
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 think we should return a boolean 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. you're correct. I'll be adding an error case here which would be returned in case the hasBlock is false ( since it shouldn't happen ). |
||
return err | ||
} | ||
|
||
|
@@ -69,6 +70,16 @@ 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. | ||
// 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 |
||
blkTxsIDs := set.NewSet[ids.ID](len(blk.Txs())) | ||
for _, tx := range blk.Txs() { | ||
id := tx.GetID() | ||
if blkTxsIDs.Contains(id) { | ||
return fmt.Errorf("%w: duplicate in block", ErrDuplicateContainer) | ||
} | ||
blkTxsIDs.Add(id) | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -97,6 +108,7 @@ func (v *TimeValidityWindow[Container]) isRepeat( | |
defer v.lock.Unlock() | ||
|
||
var err error | ||
var hasBlock bool | ||
for { | ||
if ancestorBlk.Timestamp() < oldestAllowed { | ||
return marker, nil | ||
|
@@ -110,16 +122,16 @@ 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 | ||
} | ||
} | ||
} | ||
|
||
ancestorBlk, err = v.chainIndex.GetExecutionBlock(ctx, ancestorBlk.Parent()) | ||
if err != nil { | ||
ancestorBlk, hasBlock, err = v.chainIndex.GetExecutionBlock(ctx, ancestorBlk.Parent()) | ||
if err != nil || !hasBlock { | ||
return marker, err | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,19 +6,20 @@ 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" | ||
|
||
"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 +52,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,16 +115,54 @@ 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"` | ||
|
||
blkID ids.ID | ||
blkBytes []byte | ||
certSet set.Set[ids.ID] | ||
} | ||
|
||
func NewBlock(parentID ids.ID, height uint64, timestamp int64, chunkCerts []*ChunkCertificate) Block { | ||
blk := Block{ | ||
ParentID: parentID, | ||
Hght: height, | ||
Tmstmp: timestamp, | ||
ChunkCerts: chunkCerts, | ||
} | ||
blk.init() | ||
return blk | ||
} | ||
|
||
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 { | ||
return b.certSet.Contains(id) | ||
} | ||
|
||
func (b Block) init() { | ||
for _, c := range b.ChunkCerts { | ||
b.certSet.Add(c.ChunkID) | ||
} | ||
} | ||
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 move the cert set to the execution block type? |
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.
Why are we adding the boolean here?
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.
My guess is that this is to return
true/false
, and use theerror
if something goes wrong instead of just returning the block type + an error, and you have to introspect the error using a check againstdatabase.ErrNotFound
.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.
@aaronbuchwald see @joshua-kim's request above to change the function signature to
GetExecutionBlock() (ExecutionBlock[T], bool, error)
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.
When is it valid to not have an execution block that the validity window requests?
We should always have a window of blocks that goes past the time validity window. I am not sure of any case where not having a block ie. returning false should not also be treated as an error. Unless there is such a case, I think we're better off just returning an error and not adding to the return type here.