refactor marshalActions implementation #1631

tsachiherman · 2024-10-03T20:32:06Z

What's in this PR ?

This PR refactor the transaction's marshaling implementation, and provide separation between the transaction and the inner actions. ( the latter is needed for #1610 )

What are the changes in this PR ?

a pseudo-type was created Actions that wraps []Action.
Actions supports Size() and marshalInto().
The transaction marshaling was unified so that the actual marshaling always takes place in transaction.marshal() which is being called by either transaction.Digest() or transaction.Marshal(). The difference is whether it's being called without/with the signature.
The transaction.Sign method would no longer change the Auth member of the receiver object. As before, the returned transaction would contain the Auth.

Additional notes:

Making of UnmarshalActions exported was to support upcoming #1610.

aaronbuchwald · 2024-10-03T20:44:23Z

chain/transaction_test.go

+	require.Nil(tx.Auth)
+	require.NotNil(signedTx.Auth)


Adding a check that we did not mutate the original tx type

aaronbuchwald · 2024-10-03T20:47:14Z

chain/transaction.go

+	if actionsSize, err := t.Actions.Size(); err != nil {
+		return nil, err
+	} else {
+		size += actionsSize
 	}


Can we remove the if else block here, so that we have an if block for handling the error and move size += actionsSize outside the else block as it's just the expected flow

aaronbuchwald · 2024-10-03T20:48:35Z

chain/transaction.go

+// Digest returns a byte slice containing the marshaled transaction without the auth.
+// method name might be misleading, as it returns the preimage of what typically
+// considered to be "the message digest"


Suggested change

// Digest returns a byte slice containing the marshaled transaction without the auth.

// method name might be misleading, as it returns the preimage of what typically

// considered to be "the message digest"

// Digest returns the byte slice representation of the unsigned tx

(optional for this PR) - would it make sense to rename this appropriately rather than documenting the fact that it's a misnomer? Looking at where we call this function, looks like we should be able to eliminate some duplicate code as well, since we call this function just to call verify on the auth that's already part of this tx in a few places.

aaronbuchwald · 2024-10-03T20:49:08Z

chain/transaction.go

+type (
+	Actions     []Action
+	Transaction struct {
+		Base *Base `json:"base"`

-	Actions []Action `json:"actions"`
-	Auth    Auth     `json:"auth"`
+		Actions Actions `json:"actions"`
+		Auth    Auth    `json:"auth"`

-	digest    []byte
-	bytes     []byte
-	size      int
-	id        ids.ID
-	stateKeys state.Keys
-}
+		digest    []byte
+		bytes     []byte
+		size      int
+		id        ids.ID
+		stateKeys state.Keys
+	}
+)


Can we remove the type block and put the Actions type declaration directly above its associated functions?

aaronbuchwald · 2024-10-03T20:51:04Z

chain/transaction.go

+// Sign sign the transaction using the provided AuthFactory. The pointer receiver is not being modified by this method,
+// and a new Transaction object would be returned. On success, the returned transaction is the signed transaction.


Suggested change

// Sign sign the transaction using the provided AuthFactory. The pointer receiver is not being modified by this method,

// and a new Transaction object would be returned. On success, the returned transaction is the signed transaction.

// Sign returns a new signed transaction with the unsigned tx copied from

// the original and a signature provided by the authFactory

aaronbuchwald · 2024-10-03T20:51:39Z

chain/transaction.go

+	// grab the preimage of the transaction we would want to sign.
 	msg, err := t.Digest()
 	if err != nil {
 		return nil, err
 	}
+	// sign the preimage
 	auth, err := factory.Sign(msg)
 	if err != nil {
 		return nil, err
 	}
-	t.Auth = auth
+
+	// fill in the auth on the signed transaction


could we remove the short comments here? With the added comment to Digest() I think this code is simple enough to follow w/o added comments.

aaronbuchwald · 2024-10-03T20:57:24Z

chain/transaction.go

 	auth, err := factory.Sign(msg)
 	if err != nil {
 		return nil, err
 	}
-	t.Auth = auth


hmm - previous code added auth to the original AND created a copy w/ the signature

I figured it would be safer to create a new object for the marshaling rather than (set auth/marshal/clear auth).
i.e. the Sign method parameters suggest it would be a readonly method, as it returns the signed transaction.

Yup, much better w/ this change

aaronbuchwald · 2024-10-03T20:59:21Z

chain/transaction.go

+	signedTransaction := Transaction{
+		Base:    t.Base,
+		Actions: t.Actions,
+		Auth:    auth,
+	}


(musing) hmm so we create a signedTransaction here, but only use it to marshal and will actually never return it.

Much prefer this version where we no longer modify the original tx and return a new fully populated value. Further improvement (for a separate PR) may be to separate an unsigned and signed tx type.

…abs/hypersdk into tsachi/refactor-actions-marshaling

aaronbuchwald

Could we update from Preimage to UnsignedBytes() ? Preimage() is a bit unclear because this type includes both the unsigned tx and signed tx, so we can be more explicit with UnsignedBytes.

Other than that, I think we should update the names of the local variables from txDigest or similar to unsignedTxBytes so that it matches the function name.

aaronbuchwald · 2024-10-04T14:14:38Z

chain/transaction.go


-	digest    []byte
+	preimage  []byte


could we rename this field to unsignedBytes and the accessor function to UnsignedBytes() ?

aaronbuchwald · 2024-10-04T14:14:53Z

chain/transaction.go

-func (t *Transaction) Digest() ([]byte, error) {
-	if len(t.digest) > 0 {
-		return t.digest, nil
+// Digest returns the byte slice representation of the unsigned tx


nit: update the comment to match the function name

aaronbuchwald · 2024-10-04T14:15:14Z

chain/transaction.go

 	auth, err := factory.Sign(msg)
 	if err != nil {
 		return nil, err
 	}
-	t.Auth = auth


Yup, much better w/ this change

aaronbuchwald · 2024-10-04T14:16:30Z

internal/gossiper/proposer.go

@@ -193,7 +193,7 @@ func (g *Proposer) HandleAppGossip(ctx context.Context, nodeID ids.NodeID, msg [
 	var seen int
 	for _, tx := range txs {
 		// Verify signature async
-		txDigest, err := tx.Digest()
+		txDigest, err := tx.Preimage()


can we update the local variable names where we're updating the function name so that they match and we get rid of the misnomer? Same comment throughout.

aaronbuchwald · 2024-10-04T14:17:30Z

chain/transaction_test.go

+	txBeforeSign := tx
+	require.Nil(tx.Auth)
 	signedTx, err := tx.Sign(factory, actionRegistry, authRegistry)
 	require.NoError(err)
-
+	require.Equal(txBeforeSign, tx)


Is it possible for require.Equal(txBeforeSign, tx) to fail even if it were still modifying the tx? I think this is checking the same pointer

given that we don't have a deep clone method for the Transaction, I'll just create the "expected" copy of the transaction definition and compare against that.

aaronbuchwald · 2024-10-04T14:22:11Z

chain/block.go

@@ -236,7 +236,7 @@ func (b *StatefulBlock) populateTxs(ctx context.Context) error {

 		// Verify signature async
 		if b.vm.GetVerifyAuth() {
-			txDigest, err := tx.Digest()
+			txDigest, err := tx.Preimage()


Could we rename the local variable to align with the updated function name?

aaronbuchwald

Small rename request for the results of UnsignedBytes(), then this LGTM

aaronbuchwald · 2024-10-04T14:48:18Z

chain/block.go

@@ -236,7 +236,7 @@ func (b *StatefulBlock) populateTxs(ctx context.Context) error {

 		// Verify signature async
 		if b.vm.GetVerifyAuth() {
-			txDigest, err := tx.Digest()
+			txDigest, err := tx.UnsignedBytes()


Can we rename txDigest to unsignedTxBytes ?

aaronbuchwald · 2024-10-04T14:49:25Z

internal/gossiper/proposer.go

@@ -193,7 +193,7 @@ func (g *Proposer) HandleAppGossip(ctx context.Context, nodeID ids.NodeID, msg [
 	var seen int
 	for _, tx := range txs {
 		// Verify signature async
-		txDigest, err := tx.Digest()
+		txDigest, err := tx.UnsignedBytes()


Can we rename txDigest to unsignedTxBytes ?

aaronbuchwald · 2024-10-04T14:49:35Z

tests/integration/integration.go

@@ -356,7 +356,7 @@ var _ = ginkgo.Describe("[Tx Processing]", ginkgo.Serial, func() {
 			)
 			// Must do manual construction to avoid `tx.Sign` error (would fail with
 			// 0 timestamp)
-			msg, err := tx.Digest()
+			msg, err := tx.UnsignedBytes()


Can we rename msg to unsignedTxBytes ?

aaronbuchwald · 2024-10-04T14:50:15Z

vm/vm.go

@@ -924,7 +924,7 @@ func (vm *VM) Submit(

 		// Verify auth if not already verified by caller
 		if verifyAuth && vm.config.VerifyAuth {
-			msg, err := tx.Digest()
+			msg, err := tx.UnsignedBytes()


Can we use tx.Verify(ctx) here since the next step is tx.Auth.Verify(ctx, msg) ?

Feel free to ignore this one so it can be cleaned up when we switch to separate types for signed / unsigned txs

aaronbuchwald · 2024-10-04T14:53:36Z

chain/transaction_test.go

+	signedDigest, err := signedTx.UnsignedBytes()
 	require.NoError(err)
-	txDigest, err := tx.Digest()
+	txDigest, err := tx.UnsignedBytes()


Can we rename these to unsignedTxBytes and originalUnsignedTxBytes ?

aaronbuchwald

LGTM

commit fee360f Author: Tsachi Herman <[email protected]> Date: Fri Oct 4 11:23:58 2024 -0400 refactor marshalActions implementation (#1631) commit 2cb5530 Author: aaronbuchwald <[email protected]> Date: Thu Oct 3 18:49:12 2024 -0400 Add block indexing to Indexer API (#1606) commit f1bcc59 Author: aaronbuchwald <[email protected]> Date: Thu Oct 3 18:31:39 2024 -0400 Add JSON marshalling to Result (#1627) commit 2ea784a Author: Richard Pringle <[email protected]> Date: Thu Oct 3 12:54:10 2024 -0400 Implement multisig example (#1581) commit 6bbf236 Author: Richard Pringle <[email protected]> Date: Thu Oct 3 12:43:41 2024 -0400 Update codeowners (#1629) commit 47849d8 Author: aaronbuchwald <[email protected]> Date: Thu Oct 3 08:34:51 2024 -0400 Re-mark pubsub tests as flaky (#1625) commit d847132 Author: aaronbuchwald <[email protected]> Date: Wed Oct 2 14:23:24 2024 -0400 Reduce tests.unit.sh timeout value less than CI job (#1623) commit 404e74f Author: aaronbuchwald <[email protected]> Date: Wed Oct 2 14:07:04 2024 -0400 Add JSON marshalling for fee dimensions (#1622) commit 7e53003 Author: Joshua Kim <[email protected]> Date: Wed Oct 2 12:38:34 2024 -0400 Replace usage of `internal/network` with `p2p` (#1613) Signed-off-by: Joshua Kim <[email protected]> commit 79f363e Author: containerman17 <[email protected]> Date: Wed Oct 2 11:53:59 2024 +0900 Standardize decimals (#1620) commit e191c88 Author: Joshua Kim <[email protected]> Date: Tue Oct 1 20:02:30 2024 -0400 Update to [email protected] (#1614) Signed-off-by: Joshua Kim <[email protected]> commit 59c84d7 Author: aaronbuchwald <[email protected]> Date: Tue Oct 1 17:19:17 2024 -0400 Add ExecutedBlock type (#1601) commit 165a455 Author: Tsachi Herman <[email protected]> Date: Tue Oct 1 09:26:32 2024 -0400 validate buffer length prior to calling Uint64 (#1588) commit c24f0d8 Author: rodrigo <[email protected]> Date: Tue Oct 1 09:25:18 2024 -0400 Fix VM-With-Contracts Unit Tests (#1602) commit 8ab9d83 Author: aaronbuchwald <[email protected]> Date: Mon Sep 30 16:57:29 2024 -0400 Minor nits on pubsub implementation (#1593) commit 592cd7a Author: rodrigo <[email protected]> Date: Mon Sep 30 12:46:35 2024 -0400 Remove `StateKeysMaxChunks()` (#1607) commit 3e358c1 Author: aaronbuchwald <[email protected]> Date: Mon Sep 30 12:46:20 2024 -0400 Update ActionBenchmark to use single ExpectedOutput (#1608) commit e78841e Author: Franfran <[email protected]> Date: Mon Sep 30 16:42:00 2024 +0200 Write FIFO cache unit tests (#1201) * write FIFO cache unit tests * invert expected / actual * add limit parameter and cache fail on 0 size * simplify test table and create new test for empty cache * simpler tests by having a limit of 2 * fix test cases * typos * backout uselesss change * add a "not an LRU" test case * use switch in fifo unit tests commit f1150a1 Author: aaronbuchwald <[email protected]> Date: Fri Sep 27 17:46:28 2024 -0400 Update tx indexer to include tx action outputs (#1597) commit 6a3fd63 Author: Joshua Kim <[email protected]> Date: Thu Sep 26 15:44:56 2024 -0400 Remove usage of mockgen (#1591) Signed-off-by: Joshua Kim <[email protected]> commit 49376bd Author: rodrigo <[email protected]> Date: Thu Sep 26 14:39:59 2024 -0400 Fix benchmarks (#1590) commit 77a73ba Author: Richard Pringle <[email protected]> Date: Tue Sep 24 12:38:27 2024 -0400 Ignore gas on mock-function-call (#1585) commit 1bbab7b Author: containerman17 <[email protected]> Date: Tue Sep 24 23:19:18 2024 +0900 Remove special cases for Bytes, add arrays support (#1587) commit 3410599 Author: aaronbuchwald <[email protected]> Date: Mon Sep 23 12:34:55 2024 -0400 Add spam cmd to morpheus readme (#1577) commit dfefd27 Author: aaronbuchwald <[email protected]> Date: Mon Sep 23 12:34:32 2024 -0400 Improve chain comments (#1579) commit 6c15244 Author: Richard Pringle <[email protected]> Date: Fri Sep 20 16:10:21 2024 -0400 Series of changes that improve development (#1583) commit e0a3324 Author: Richard Pringle <[email protected]> Date: Fri Sep 20 13:12:58 2024 -0400 Use repr(packed) with state-keys (#1580) commit 75c6cfd Author: aaronbuchwald <[email protected]> Date: Thu Sep 19 07:58:39 2024 -0400 Separate proposer monitor from vm into internal package (#1574) Signed-off-by: Joshua Kim <[email protected]>

tsachiherman added 2 commits October 3, 2024 16:11

update

3e0facc

update

5839827

tsachiherman self-assigned this Oct 3, 2024

Merge branch 'main' into tsachi/refactor-actions-marshaling

7887d15

tsachiherman marked this pull request as ready for review October 3, 2024 20:32

tsachiherman requested review from aaronbuchwald and joshua-kim as code owners October 3, 2024 20:32

aaronbuchwald requested changes Oct 3, 2024

View reviewed changes

tsachiherman added 3 commits October 4, 2024 09:04

update per CR

22c8650

Merge branch 'tsachi/refactor-actions-marshaling' of github.com:ava-l…

4965f38

…abs/hypersdk into tsachi/refactor-actions-marshaling

Merge branch 'main' into tsachi/refactor-actions-marshaling

4d91c9c

aaronbuchwald requested changes Oct 4, 2024

View reviewed changes

update

239541d

aaronbuchwald requested changes Oct 4, 2024

View reviewed changes

tsachiherman added 2 commits October 4, 2024 11:00

update per CR.

81e2d88

update

3f47797

aaronbuchwald approved these changes Oct 4, 2024

View reviewed changes

aaronbuchwald enabled auto-merge (squash) October 4, 2024 15:06

aaronbuchwald merged commit fee360f into main Oct 4, 2024
17 checks passed

		// Sign sign the transaction using the provided AuthFactory. The pointer receiver is not being modified by this method,
		// and a new Transaction object would be returned. On success, the returned transaction is the signed transaction.

refactor marshalActions implementation #1631

refactor marshalActions implementation #1631

Conversation

tsachiherman commented Oct 3, 2024 • edited Loading

What's in this PR ?

What are the changes in this PR ?

Additional notes:

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

aaronbuchwald left a 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

aaronbuchwald left a 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

aaronbuchwald left a comment

Choose a reason for hiding this comment

tsachiherman commented Oct 3, 2024 •

edited

Loading