x/dsmr: Replace the avalanchego codec with canoto #1849

StephenButtolph · 2025-01-03T04:37:40Z

Benchmarks look nice (as expected):

goos: darwin
goarch: arm64
pkg: github.com/ava-labs/hypersdk/x/dsmr
cpu: Apple M2 Max
              │      old       │                 new                 │
              │     sec/op     │   sec/op     vs base                │
ChunkInit-12    6654.0n ± 252%   187.3n ± 1%  -97.18% (p=0.000 n=10)
ParseChunk-12   7943.0n ±   1%   367.8n ± 0%  -95.37% (p=0.000 n=10)
geomean          7.270µ          262.5n       -96.39%

              │      old      │                new                 │
              │     B/op      │    B/op     vs base                │
ChunkInit-12    262209.0 ± 0%   144.0 ± 0%  -99.95% (p=0.000 n=10)
ParseChunk-12   262961.0 ± 0%   248.0 ± 0%  -99.91% (p=0.000 n=10)
geomean          256.4Ki        189.0       -99.93%

              │     old     │                new                 │
              │  allocs/op  │ allocs/op   vs base                │
ChunkInit-12     2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.000 n=10)
ParseChunk-12   14.000 ± 0%   3.000 ± 0%  -78.57% (p=0.000 n=10)
geomean          5.292        1.732       -67.27%
¹ all samples are equal

StephenButtolph · 2025-01-03T04:39:43Z

tools.go

+// TODO: Once we update to go1.24, we should register canoto as a tool in the
+// go.mod file.
+import (
+	_ "github.com/StephenButtolph/canoto/generate"
+)


This allows us to inherit the canoto version from the go.mod file - rather than needing to specify the version in every file.

StephenButtolph · 2025-01-03T04:41:05Z

x/dsmr/block.go

@@ -1,69 +1,65 @@
 // Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
 // See the file LICENSE for licensing terms.

+//go:generate go run github.com/StephenButtolph/canoto/canoto --concurrent=false $GOFILE


We specify --concurrent=false to allow passing structs by value.

StephenButtolph · 2025-01-03T04:42:39Z

x/dsmr/block.go

+type Tx[T any] interface {
+	canoto.FieldMaker[T]


This is basically the only real change. Rather than expecting the tx to be serializable with the avalanchego codec (which is an undocumented assumption) the interface now specifies canoto.

I had talked with aaron super briefly, but we may want to replace these with []byte so that the transactions can specify their own encoding.

StephenButtolph · 2025-01-03T04:44:18Z

x/dsmr/block.go

+func (c *Chunk[T]) init() {
+	c.bytes = c.MarshalCanoto()
 	c.id = utils.ToID(c.bytes)
-
-	return nil
 }


This diff warms my heart.

Dropping the error on the signature is very nice here

StephenButtolph · 2025-01-03T04:46:06Z

x/dsmr/block.go

-	Timestamp int64  `serialize:"true"`
+	ParentID  ids.ID `canoto:"fixed bytes,1"`
+	Height    uint64 `canoto:"int,2"`
+	Timestamp int64  `canoto:"int,3"`


I decided to use int rather than sint here - because I don't think we ever actually expect Timestamp to be negative.

StephenButtolph · 2025-01-03T04:47:04Z

x/dsmr/block.go

 	BlockHeader
 	ID     ids.ID
-	Chunks []Chunk[T] `serialize:"true"`
+	Chunks []Chunk[T] `canoto:"repeated value,1"`


Similarly to Block - is it correct for us to only serialize the Chunks here?

We probably don't have to technically serialize this... we know the ID from the Block this is derived from

StephenButtolph · 2025-01-03T04:50:44Z

x/dsmr/certificate.go

-const (
-	CodecVersion = 0
-
-	MaxMessageSize = units.KiB


Was MaxMessageSize used to actually enforce something important? Or was this just used to conform to the avalanchego codec?

This isn't used for anything important afaik. I think hypersdk originally had a 1 kb max message size specified everywhere in earlier parts of the codebase and we ended up continuing to do this for consistency, but afaik it's not needed because avalanchego will handle it for us and IMO we shouldn't be specifying it

StephenButtolph · 2025-01-03T04:51:46Z

x/dsmr/certificate.go

+type Signature struct {
+	// Signers is a big-endian byte slice encoding which validators signed this
+	// message.
+	Signers   []byte                 `canoto:"bytes,1"`
+	Signature [bls.SignatureLen]byte `canoto:"fixed bytes,2"`
+
+	canotoData canotoData_Signature
 }


Because *warp.BitSetSignature is implemented in avalanchego - we needed to re-implement it here to be able to serialize it differently.

StephenButtolph · 2025-01-03T04:54:11Z

x/dsmr/p2p.go

-	_ acp118.Verifier = (*ChunkSignatureRequestVerifier[Tx])(nil)
-	_ p2p.Handler     = (*GetChunkHandler[Tx])(nil)
-	_ p2p.Handler     = (*ChunkCertificateGossipHandler[Tx])(nil)


I don't think we have a good type to parameterize Tx with here to allow for these assertions... So I removed them. (Similar comment for other interface checks)

StephenButtolph · 2025-01-03T04:57:26Z

x/dsmr/typed_client.go

-	chunk := Chunk[T]{}
-	if err := codec.LinearCodec.UnmarshalFrom(
-		&wrappers.Packer{Bytes: response.Chunk},
-		&chunk,
-	); err != nil {
-		return Chunk[T]{}, err
-	}
-
-	return chunk, chunk.init()
+	return ParseChunk[T](response.Chunk)
 }


This is almost an unrelated change... This could have previously called to ParseChunk as well... But with removal of the codec.LinearCodec, I felt like this change made sense here.

joshua-kim · 2025-01-03T05:37:42Z

go.mod


 require (
+	github.com/StephenButtolph/canoto v0.9.0


Is there a reason this lives under a personal github account as opposed to the org repo? It seems like this will be annoying to manage because it won't inherit org-level perms/settings/teams/etc.

I had made this as a personal project while OOO. If we wanted to move it under the ava-labs org we could 🤷.

fwiw - I hope we never need to make any changes to it 😅

joshua-kim · 2025-01-03T05:39:56Z

x/dsmr/block.go

+func (c *Chunk[T]) init() {
+	c.bytes = c.MarshalCanoto()
 	c.id = utils.ToID(c.bytes)
-
-	return nil
 }


Dropping the error on the signature is very nice here

joshua-kim · 2025-01-03T05:46:32Z

x/dsmr/certificate.go

-const (
-	CodecVersion = 0
-
-	MaxMessageSize = units.KiB


This isn't used for anything important afaik. I think hypersdk originally had a 1 kb max message size specified everywhere in earlier parts of the codebase and we ended up continuing to do this for consistency, but afaik it's not needed because avalanchego will handle it for us and IMO we shouldn't be specifying it

joshua-kim · 2025-01-03T07:07:05Z

x/dsmr/block.go

+	Height    uint64 `canoto:"int,2"`
+	Timestamp int64  `canoto:"int,3"`
+
+	canotoData canotoData_BlockHeader
 }

 type Block struct {
 	BlockHeader


I think this will technically pass tests but we should serialize here because the block id/bytes should be derived from the BlockHeader

joshua-kim · 2025-01-03T07:08:03Z

x/dsmr/block.go

 	BlockHeader
 	ID     ids.ID
-	Chunks []Chunk[T] `serialize:"true"`
+	Chunks []Chunk[T] `canoto:"repeated value,1"`


We probably don't have to technically serialize this... we know the ID from the Block this is derived from

StephenButtolph · 2025-01-03T17:47:53Z

x/dsmr/block.go

 	"github.com/ava-labs/hypersdk/utils"
 )

-const InitialChunkSize = 250 * 1024


This is why so much memory was being allocated when initializing the chunk.

StephenButtolph added 6 commits January 2, 2025 18:00

ChunkReference

1b07113

ChunkCertificate

56a1881

Integrate canoto into dsmr

3a11a72

nit

2e67e20

reduce diff

bfa743f

Remove unused code

21cbd2c

StephenButtolph commented Jan 3, 2025

View reviewed changes

StephenButtolph added 4 commits January 2, 2025 23:58

nit

0917df9

nit

6e5e8f0

Add benchmarks

5a7b1cb

Update canoto

f6f807b

StephenButtolph marked this pull request as ready for review January 3, 2025 06:55

StephenButtolph requested review from aaronbuchwald and joshua-kim as code owners January 3, 2025 06:55

joshua-kim reviewed Jan 3, 2025

View reviewed changes

Remove unused constant

cb67479

StephenButtolph commented Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/dsmr: Replace the avalanchego codec with canoto #1849

x/dsmr: Replace the avalanchego codec with canoto #1849

StephenButtolph commented Jan 3, 2025 •

edited

Loading

StephenButtolph Jan 3, 2025

StephenButtolph Jan 3, 2025

StephenButtolph Jan 3, 2025

StephenButtolph Jan 3, 2025

joshua-kim Jan 3, 2025

StephenButtolph Jan 3, 2025

StephenButtolph Jan 3, 2025

joshua-kim Jan 3, 2025

StephenButtolph Jan 3, 2025

joshua-kim Jan 3, 2025

StephenButtolph Jan 3, 2025

StephenButtolph Jan 3, 2025

StephenButtolph Jan 3, 2025

joshua-kim Jan 3, 2025

StephenButtolph Jan 3, 2025

joshua-kim Jan 3, 2025

joshua-kim Jan 3, 2025

joshua-kim Jan 3, 2025

joshua-kim Jan 3, 2025

StephenButtolph Jan 3, 2025

x/dsmr: Replace the avalanchego codec with canoto #1849

Are you sure you want to change the base?

x/dsmr: Replace the avalanchego codec with canoto #1849

Conversation

StephenButtolph commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenButtolph commented Jan 3, 2025 •

edited

Loading