Replace custom serialization and asn.1 with Canoto by StephenButtolph · Pull Request #46 · ava-labs/Simplex

StephenButtolph · 2025-01-11T15:37:08Z

This PR should not be reviewed in its current state. This is just a spike to show that it can be done.

There were a number of almost unrelated changes that were made in this pass that should be reviewed / discussed in their own right unrelated to the introduction of Canoto.

Ignoring auto-generated code, the diff is (currently) +507 / -769

StephenButtolph

Going to factor the non-serialization related changes out of this PR.

StephenButtolph · 2025-01-11T15:38:19Z

metadata.go

This file is now almost entirely replaced with auto-generated code.

StephenButtolph · 2025-01-11T15:43:06Z

msg.go

This PR merged a number of types (rather than implementing custom types, the variables imply the context). I felt like this reduced duplicate code and made things easier to understand... But I suppose this may have been intentional to always enforce passing the correct messages based on type.

StephenButtolph · 2025-01-11T15:43:28Z

record/consts.go

These are replaced with the Record type

StephenButtolph · 2025-01-11T15:54:25Z

encoding.go

This file is now essentially entirely auto-generated.

StephenButtolph · 2025-01-11T15:57:03Z

encoding_test.go

-func TestBlockRecord(t *testing.T) {
-	bh := BlockHeader{
-		ProtocolMetadata: ProtocolMetadata{
-			Version: 1,
-			Round:   2,
-			Seq:     3,
-			Epoch:   4,
-		},
-	}
-
-	_, err := rand.Read(bh.Prev[:])
-	require.NoError(t, err)
-
-	_, err = rand.Read(bh.Digest[:])
-	require.NoError(t, err)
-
-	payload := []byte{11, 12, 13, 14, 15, 16}
-
-	record := blockRecord(bh, payload)
-
-	md2, payload2, err := blockFromRecord(record)
-	require.NoError(t, err)
-
-	require.Equal(t, bh, md2)
-	require.Equal(t, payload, payload2)
-}
-
-func FuzzBlockRecord(f *testing.F) {
-	f.Fuzz(func(t *testing.T, version uint8, round, seq, epoch uint64, prevPreimage, digestPreimage []byte, payload []byte) {
-		prev := sha256.Sum256(prevPreimage)
-		digest := sha256.Sum256(digestPreimage)
-		bh := BlockHeader{
-			ProtocolMetadata: ProtocolMetadata{
-				Version: version,
-				Round:   round,
-				Seq:     seq,
-				Epoch:   epoch,
-				Prev:    prev,
-			},
-			Digest: digest,
-		}
-
-		record := blockRecord(bh, payload)
-
-		md2, payload2, err := blockFromRecord(record)
-		require.NoError(t, err)
-
-		require.Equal(t, bh, md2)
-		require.Equal(t, payload, payload2)
-	})
-}


This is also a change unrelated to the Canoto inclusion. The block record previously included the protocol metadata multiple times (because the payload is expected to be able to be parsed and the result is then expected to provide the metadata as well)

StephenButtolph · 2025-01-11T15:59:28Z

encoding_test.go

Should this whole file be deleted? It is testing canoto more than this library.

StephenButtolph · 2025-01-11T16:00:27Z

epoch.go

+	case msg.Proposal != nil:
+		return e.handleBlockMessage(msg.Proposal, from)
+	case msg.Vote != nil:
+		return e.handleVoteMessage(msg.Vote, from)
 	case msg.Notarization != nil:
-		return e.handleNotarizationMessage(msg, from)
+		return e.handleNotarizationMessage(msg.Notarization, from)
+	case msg.Finalize != nil:
+		return e.handleFinalizeMessage(msg.Finalize, from)
 	case msg.Finalization != nil:
-		return e.handleFinalizationMessage(msg, from)
-	case msg.FinalizationCertificate != nil:
-		return e.handleFinalizationCertificateMessage(msg, from)
+		return e.handleFinalizationMessage(msg.Finalization, from)


These are unrelated changes.

StephenButtolph · 2025-01-11T16:02:47Z

epoch.go

-	if bh.Version != 0 {
-		e.Logger.Debug("Got block message with wrong version number, expected 0", zap.Uint8("version", bh.Version))
-		return false
-	}


Canoto supports changes to the serialized format by adding/removing fields. So there isn't a need for an explicit version in the format anymore.

StephenButtolph · 2025-01-11T16:04:33Z

epoch_multinode_test.go

@@ -185,15 +182,25 @@ func (c *testComm) SendMessage(msg *Message, destination NodeID) {
 }

 func (c *testComm) Broadcast(msg *Message) {


This previously assumed that messages were read-only. So now it copies the message so that receiving epoch instances can modify them.

StephenButtolph · 2025-01-11T16:06:02Z

global.go

-type NodeID []byte
-
-func (node NodeID) String() string {
-	return hex.EncodeToString(node)
-}
-
-func (node NodeID) Equals(otherNode NodeID) bool {
-	return bytes.Equal(node, otherNode)
-}


Moved into the types file

yacovm · 2025-01-12T23:38:56Z

types.go

+	Message struct {
+		Proposal     *Proposal `canoto:"pointer,1,message"`
+		Vote         *Vote     `canoto:"pointer,2,message"`
+		Notarization *Quorum   `canoto:"pointer,3,message"`


can we keep the original types? A type for notarization and a type for finalization.

I don't want any chance of notarizations and finalizations being mixed by mistake.

yacovm · 2025-01-12T23:42:44Z

types.go

+		Payload: msg,
+		Context: context,
+	}
+	toBeSigned := sm.MarshalCanoto()


i would rather prefer ASN1 to be used when marshaling anything that is being signed.

I understand Canoto is supposed to be deterministic, but for the sake of security I'd prefer if we use a package that had more mileage and adopts a standard that is widely used for signatures.

yacovm · 2025-01-12T23:45:37Z

types.go

+	NodeID []byte
+)
+
+func (v *Vote) Sign(nodeID NodeID, signer Signer, context string) error {


I think that passing a free and arbitrary context string is too much freedom and is too error prone from a security point of view. That's why I intentionally had different message types, so it isn't possible to sign a notarization as a finalization and vice versa.

yacovm · 2025-01-12T23:49:32Z

types.go

+
+	Message struct {
+		Proposal     *Proposal `canoto:"pointer,1,message"`
+		Vote         *Vote     `canoto:"pointer,2,message"`


This message struct is not supposed to be serialized. It's only supposed to be used to be passed along function calls.

I think we should only define separately the types we need to persist to disk and not mix them with consensus logic.

I don't want us to leak implementation details to the API that the application uses to interact with Simplex.

Therefore, the Message and all its underlying fields should be clean of anything the API doesn't use.

StephenButtolph added 8 commits January 11, 2025 08:01

wip

68b727b

wip

36db7d5

wip

beeacec

nit

6b778f2

Remove dependency

20845b6

reduce diff

3fea3de

nit

1683857

nit

6a2ebd9

StephenButtolph commented Jan 11, 2025

View reviewed changes

nits

0b9bc36

yacovm reviewed Jan 12, 2025

View reviewed changes

		@@ -185,15 +182,25 @@ func (c testComm) SendMessage(msg Message, destination NodeID) {
		}

		func (c testComm) Broadcast(msg Message) {

Comments

Conversation

StephenButtolph commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StephenButtolph left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yacovm Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yacovm Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

StephenButtolph commented Jan 11, 2025 •

edited

Loading

yacovm Jan 12, 2025 •

edited

Loading

yacovm Jan 12, 2025 •

edited

Loading