Skip to content
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

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
e5ecf69
eliminate emapChunk
tsachiherman Nov 22, 2024
ba644c3
update
tsachiherman Nov 22, 2024
2939543
update
tsachiherman Nov 25, 2024
08f0d59
Merge branch 'main' into tsachi/refactor_validity_window3
tsachiherman Nov 25, 2024
659f1fb
update per CR
tsachiherman Nov 25, 2024
921b908
lint
tsachiherman Nov 25, 2024
508cc92
apply requested changes.
tsachiherman Nov 25, 2024
7a3fed6
fix code + test.
tsachiherman Nov 25, 2024
b312a3c
rollback unneeded changes.
tsachiherman Nov 25, 2024
1238460
rename testing chain indexer.
tsachiherman Nov 25, 2024
8c1ab98
update
tsachiherman Nov 25, 2024
f8e42a8
add unit test.
tsachiherman Nov 26, 2024
b000edf
update unit test.
tsachiherman Nov 26, 2024
1d2536e
Merge branch 'main' into tsachi/refactor_validity_window3
tsachiherman Nov 27, 2024
6911f26
lint
tsachiherman Nov 27, 2024
f2816e7
update
tsachiherman Nov 28, 2024
e78f847
lint
tsachiherman Nov 28, 2024
270245a
update
tsachiherman Nov 28, 2024
144ddd4
Merge branch 'main' into tsachi/refactor_validity_window3
tsachiherman Dec 5, 2024
2fa701d
fix few CR feedback.
tsachiherman Dec 5, 2024
2220b5e
add certSet
tsachiherman Dec 6, 2024
39905b9
remove type declaration block.
tsachiherman Dec 6, 2024
d54e1a0
fix tests
tsachiherman Dec 6, 2024
8e83b6d
update
tsachiherman Dec 6, 2024
fdd4639
update
tsachiherman Dec 6, 2024
9e127f1
update
tsachiherman Dec 9, 2024
0f3fe73
create a wrapper for the block in dsmr (#1829)
tsachiherman Dec 9, 2024
fc87f31
update
tsachiherman Dec 10, 2024
cf82891
update
tsachiherman Dec 10, 2024
864acc1
updatge
tsachiherman Dec 10, 2024
d96160e
fix typo
tsachiherman Dec 10, 2024
9ea09b4
undo unwanted changes
tsachiherman Dec 10, 2024
30bcdce
update
tsachiherman Dec 10, 2024
8d52c05
update
tsachiherman Dec 10, 2024
32207b9
update unit test
tsachiherman Dec 12, 2024
5d62af7
update test
tsachiherman Dec 12, 2024
7ca779b
update
tsachiherman Dec 12, 2024
383a7ef
update
tsachiherman Dec 12, 2024
7902a6a
update per review feedback,.
tsachiherman Dec 13, 2024
1f143ea
address feedback.
tsachiherman Dec 19, 2024
6d4f94c
update per cr
tsachiherman Dec 20, 2024
dd419bb
Merge branch 'main' into tsachi/refactor_validity_window3
tsachiherman Dec 20, 2024
c87b033
update
tsachiherman Dec 23, 2024
293c6b9
Merge branch 'tsachi/refactor_validity_window3' of github.com:ava-lab…
tsachiherman Dec 23, 2024
b501cec
update per CR feedback.
tsachiherman Dec 23, 2024
68603db
update per CR
tsachiherman Dec 23, 2024
7e72626
update per feedback.
tsachiherman Dec 23, 2024
7edef6e
update
tsachiherman Dec 23, 2024
185fbf0
Merge branch 'main' into tsachi/refactor_validity_window3
tsachiherman Dec 23, 2024
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
2 changes: 1 addition & 1 deletion x/dsmr/assembler.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (b *BlockHandler[T, S, B, R]) Accept(ctx context.Context, block *Block) err
}

// Assemble and execute the block
innerBlock, result, state, err := b.Assembler.AssembleBlock(ctx, b.lastAcceptedState, b.lastAcceptedBlock, block.Timestamp, block.Height+1, txs)
innerBlock, result, state, err := b.Assembler.AssembleBlock(ctx, b.lastAcceptedState, b.lastAcceptedBlock, block.Tmstmp, block.Hght+1, txs)
if err != nil {
return err
}
Expand Down
43 changes: 38 additions & 5 deletions x/dsmr/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@

"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
Copy link
Contributor

Choose a reason for hiding this comment

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

This exposes the internal/emap package into the caller. I think the previous wrapping pattern where we wrapped this interface w/ a type that implemented the emap interface actually looked cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

GetSponsor() codec.Address
}

Expand Down Expand Up @@ -51,6 +51,14 @@
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,
Expand Down Expand Up @@ -106,9 +114,9 @@
}

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"`
Copy link
Contributor

Choose a reason for hiding this comment

The 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"`

Expand All @@ -119,3 +127,28 @@
func (b Block) GetID() ids.ID {
return b.blkID
}

func (b Block) Parent() ids.ID {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Foo() or GetFoo().

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 uint64(b.Hght)

Check failure on line 140 in x/dsmr/block.go

View workflow job for this annotation

GitHub Actions / hypersdk-lint

unnecessary conversion (unconvert)
}

func (b Block) Txs() []*ChunkCertificate {
return b.ChunkCerts
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems weird because these are not returning txs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I agree. let's discuss this as a group ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the term containers?


func (b Block) ContainsTx(id ids.ID) bool {
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
for _, c := range b.ChunkCerts {
if c.ChunkID == id {
return true
}
}
return false
}
5 changes: 3 additions & 2 deletions x/dsmr/certificate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are duplicated - GetSlot is meant to be Expiry


Expand Down Expand Up @@ -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() }

Expand Down
100 changes: 68 additions & 32 deletions x/dsmr/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@
"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"
)
Expand Down Expand Up @@ -52,7 +55,7 @@
return nil, err
}

return &Node[T]{
node := &Node[T]{
nodeID: nodeID,
networkID: networkID,
chainID: chainID,
Expand Down Expand Up @@ -80,25 +83,36 @@
storage: storage,
},
storage: storage,
}, nil
log: logging.NewLogger("dsmr"),
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
}
node.tracer, err = trace.New(trace.Config{})
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
node.validityWindow = validitywindow.NewTimeValidityWindow(node.log, node.tracer, node)
return node, err
}

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 (
timeValidityWindow = *validitywindow.TimeValidityWindow[*ChunkCertificate]

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
Expand Down Expand Up @@ -172,29 +186,40 @@
return chunk, n.storage.AddLocalChunkWithCert(chunk, &chunkCert)
}

func (n *Node[T]) BuildBlock(parent Block, timestamp int64) (Block, error) {
if timestamp <= parent.Timestamp {
const validityWindowDuration = int64(5)

// 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
}

if dup.Len() == len(chunkCerts) {
return Block{}, ErrNoAvailableChunkCerts
}
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved

availableChunkCerts := make([]*ChunkCertificate, 0)
for _, chunkCert := range chunkCerts {
// avoid building blocks with expired chunk certs
if chunkCert.Expiry < timestamp {
for i, chunkCert := range chunkCerts {
if 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,
Hght: parent.Hght + 1,
Tmstmp: timestamp,
ChunkCerts: availableChunkCerts,
}

Expand All @@ -208,9 +233,14 @@
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 {
Expand Down Expand Up @@ -263,6 +293,12 @@
}
}
}
// update the validity window with the accepted block.
n.validityWindow.Accept(block)

return n.storage.SetMin(block.Tmstmp, chunkIDs)
}

return n.storage.SetMin(block.Timestamp, chunkIDs)
func (n *Node[T]) GetExecutionBlock(ctx context.Context, blkID ids.ID) (validitywindow.ExecutionBlock[*ChunkCertificate], error) {

Check failure on line 302 in x/dsmr/node.go

View workflow job for this annotation

GitHub Actions / hypersdk-lint

unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)

Check failure on line 302 in x/dsmr/node.go

View workflow job for this annotation

GitHub Actions / hypersdk-lint

unused-parameter: parameter 'blkID' seems to be unused, consider removing or renaming it as _ (revive)

Check failure on line 302 in x/dsmr/node.go

View workflow job for this annotation

GitHub Actions / hypersdk-lint

unused-receiver: method receiver 'n' is not referenced in method's body, consider removing or renaming it as _ (revive)
aaronbuchwald marked this conversation as resolved.
Show resolved Hide resolved
return nil, nil
}
Loading
Loading