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.

Sign up for GitHub

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

signer: implement blob txs sendtxargs, enable blobtx-signing #28976

Merged
merged 9 commits into from
Apr 5, 2024
Merged

signer: implement blob txs sendtxargs, enable blobtx-signing #28976

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da67944
signer: implement blob txs sendtxargs, enable blobtx-signing
holiman Feb 12, 2024
43f29d5
internal/ethapi: remove unused param
holiman Feb 12, 2024
91fa8df
accounts/external: pass on blob-tx to backend signer
holiman Feb 12, 2024
a39e697
signer: enable signing of blob transactions
holiman Feb 12, 2024
1fd10ad
accounts/external: minor
holiman Feb 12, 2024
75f03ce
core/types, internal/ethapi: add back blobs after signing
holiman Mar 8, 2024
8a35841
Merge branch 'master' into sign_blobtxs
holiman Mar 8, 2024
bcd9790
internal/ethapi, signer/core/apitypes: updates to tests
holiman Mar 8, 2024
9c2bb33
accounts, internal, signer: fix linter nits
holiman Mar 9, 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
6 changes: 0 additions & 6 deletions internal/ethapi/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,9 +499,6 @@ func (s *PersonalAccountAPI) SignTransaction(ctx context.Context, args Transacti
if args.GasPrice == nil && (args.MaxFeePerGas == nil || args.MaxPriorityFeePerGas == nil) {
return nil, errors.New("missing gasPrice or maxFeePerGas/maxPriorityFeePerGas")
}
if args.IsEIP4844() {
return nil, errBlobTxNotSupported
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this is annoying. Can we keep this check somehow if the signer is not "external"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the vein of deprecating Sign/SendTransaction methods I think we shouldn't add new features (i.e. here blob txes).

Copy link

Choose a reason for hiding this comment

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

Oh right. So, on personal, I think we can leave this be. That is: my change here can be undone

}
if args.Nonce == nil {
return nil, errors.New("nonce not specified")
}
Expand Down Expand Up @@ -1878,9 +1875,6 @@ func (s *TransactionAPI) SignTransaction(ctx context.Context, args TransactionAr
if args.GasPrice == nil && (args.MaxPriorityFeePerGas == nil || args.MaxFeePerGas == nil) {
return nil, errors.New("missing gasPrice or maxFeePerGas/maxPriorityFeePerGas")
}
if args.IsEIP4844() {
return nil, errBlobTxNotSupported
}
if args.Nonce == nil {
return nil, errors.New("nonce not specified")
}
Expand Down
63 changes: 52 additions & 11 deletions signer/core/apitypes/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import (
"github.com/ethereum/go-ethereum/common/math"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/crypto/kzg4844"
"github.com/holiman/uint256"
)

var typedDataReferenceTypeRegexp = regexp.MustCompile(`^[A-Za-z](\w*)(\[\])?$`)
Expand Down Expand Up @@ -92,12 +94,21 @@ type SendTxArgs struct {
// We accept "data" and "input" for backwards-compatibility reasons.
// "input" is the newer name and should be preferred by clients.
// Issue detail: https://github.com/ethereum/go-ethereum/issues/15628
Data *hexutil.Bytes `json:"data"`
Data *hexutil.Bytes `json:"data,omitempty"`
Input *hexutil.Bytes `json:"input,omitempty"`

// For non-legacy transactions
AccessList *types.AccessList `json:"accessList,omitempty"`
ChainID *hexutil.Big `json:"chainId,omitempty"`

// For BlobTxType
BlobFeeCap *hexutil.Big `json:"maxFeePerBlobGas,omitempty"`
BlobHashes []common.Hash `json:"blobVersionedHashes,omitempty"`

// For BlobTxType transactions with blob sidecar
Blobs []kzg4844.Blob `json:"blobs,omitempty"`
Commitments []kzg4844.Commitment `json:"commitments,omitempty"`
Proofs []kzg4844.Proof `json:"proofs,omitempty"`
}

func (args SendTxArgs) String() string {
Expand All @@ -108,6 +119,17 @@ func (args SendTxArgs) String() string {
return err.Error()
}

// data retrieves the transaction calldata. Input field is preferred.
func (args *SendTxArgs) data() []byte {
if args.Input != nil {
return *args.Input
}
if args.Data != nil {
return *args.Data
}
return nil
}

// ToTransaction converts the arguments to a transaction.
func (args *SendTxArgs) ToTransaction() *types.Transaction {
// Add the To-field, if specified
Expand All @@ -117,15 +139,34 @@ func (args *SendTxArgs) ToTransaction() *types.Transaction {
to = &dstAddr
}

var input []byte
if args.Input != nil {
input = *args.Input
} else if args.Data != nil {
input = *args.Data
}

var data types.TxData
switch {
case args.BlobHashes != nil:
al := types.AccessList{}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always add an empty access list? Could be simpler by assigning to data.AccessList only if args.AccessList non-nil.

Copy link
Contributor

@fjl fjl Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, wait, so the issue is actually that args.AccessList is defined as pointer. That's not necessary because AccessList is a slice type, which already has a nil state. So if you change it to non-pointer, this code here can be removed.

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 think I just copy-pasted this from the TransactionArgs

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so maybe it's wrong there too.

if args.AccessList != nil {
al = *args.AccessList
}
data = &types.BlobTx{
To: *to,
ChainID: uint256.MustFromBig((*big.Int)(args.ChainID)),
Nonce: uint64(args.Nonce),
Gas: uint64(args.Gas),
GasFeeCap: uint256.MustFromBig((*big.Int)(args.MaxFeePerGas)),
GasTipCap: uint256.MustFromBig((*big.Int)(args.MaxPriorityFeePerGas)),
Value: uint256.MustFromBig((*big.Int)(&args.Value)),
Data: args.data(),
AccessList: al,
BlobHashes: args.BlobHashes,
BlobFeeCap: uint256.MustFromBig((*big.Int)(args.BlobFeeCap)),
}
if args.Blobs != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this needs to be changed to

  • require Blobs to be set
  • validate the blobs against hashes
  • validate commitments against blobs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is done in validateTxSidecar, which is invoked from ToTransaction.

data.(*types.BlobTx).Sidecar = &types.BlobTxSidecar{
Blobs: args.Blobs,
Commitments: args.Commitments,
Proofs: args.Proofs,
}
}

case args.MaxFeePerGas != nil:
al := types.AccessList{}
if args.AccessList != nil {
Expand All @@ -139,7 +180,7 @@ func (args *SendTxArgs) ToTransaction() *types.Transaction {
GasFeeCap: (*big.Int)(args.MaxFeePerGas),
GasTipCap: (*big.Int)(args.MaxPriorityFeePerGas),
Value: (*big.Int)(&args.Value),
Data: input,
Data: args.data(),
AccessList: al,
}
case args.AccessList != nil:
Expand All @@ -150,7 +191,7 @@ func (args *SendTxArgs) ToTransaction() *types.Transaction {
Gas: uint64(args.Gas),
GasPrice: (*big.Int)(args.GasPrice),
Value: (*big.Int)(&args.Value),
Data: input,
Data: args.data(),
AccessList: *args.AccessList,
}
default:
Expand All @@ -160,7 +201,7 @@ func (args *SendTxArgs) ToTransaction() *types.Transaction {
Gas: uint64(args.Gas),
GasPrice: (*big.Int)(args.GasPrice),
Value: (*big.Int)(&args.Value),
Data: input,
Data: args.data(),
}
}
return types.NewTx(data)
Expand Down
61 changes: 60 additions & 1 deletion signer/core/apitypes/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@

package apitypes

import "testing"
import (
"encoding/json"
"testing"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
)

func TestIsPrimitive(t *testing.T) {
t.Parallel()
Expand All @@ -39,3 +45,56 @@ func TestIsPrimitive(t *testing.T) {
}
}
}

func TestTxArgs(t *testing.T) {
for i, tc := range []struct {
data []byte
want common.Hash
wantType uint8
}{
{
data: []byte(`{"from":"0x1b442286e32ddcaa6e2570ce9ed85f4b4fc87425","accessList":[],"blobVersionedHashes":["0x010657f37554c781402a22917dee2f75def7ab966d7b770905398eba3c444014"],"chainId":"0x7","gas":"0x124f8","gasPrice":"0x693d4ca8","input":"0x","maxFeePerBlobGas":"0x3b9aca00","maxFeePerGas":"0x6fc23ac00","maxPriorityFeePerGas":"0x3b9aca00","nonce":"0x0","r":"0x2a922afc784d07e98012da29f2f37cae1f73eda78aa8805d3df6ee5dbb41ec1","s":"0x4f1f75ae6bcdf4970b4f305da1a15d8c5ddb21f555444beab77c9af2baab14","to":"0x1b442286e32ddcaa6e2570ce9ed85f4b4fc87425","type":"0x1","v":"0x0","value":"0x0","yParity":"0x0"}`),
s1na marked this conversation as resolved.
Show resolved Hide resolved
want: common.HexToHash("0x7d53234acc11ac5b5948632c901a944694e228795782f511887d36fd76ff15c4"),
wantType: types.BlobTxType,
},
{
// on input, we don't read the type, but infer the type from the arguments present
data: []byte(`{"from":"0x1b442286e32ddcaa6e2570ce9ed85f4b4fc87425","accessList":[],"chainId":"0x7","gas":"0x124f8","gasPrice":"0x693d4ca8","input":"0x","maxFeePerBlobGas":"0x3b9aca00","maxFeePerGas":"0x6fc23ac00","maxPriorityFeePerGas":"0x3b9aca00","nonce":"0x0","r":"0x2a922afc784d07e98012da29f2f37cae1f73eda78aa8805d3df6ee5dbb41ec1","s":"0x4f1f75ae6bcdf4970b4f305da1a15d8c5ddb21f555444beab77c9af2baab14","to":"0x1b442286e32ddcaa6e2570ce9ed85f4b4fc87425","type":"0x12","v":"0x0","value":"0x0","yParity":"0x0"}`),
want: common.HexToHash("0x7919e2b0b9b543cb87a137b6ff66491ec7ae937cb88d3c29db4d9b28073dce53"),
wantType: types.DynamicFeeTxType,
},
} {

var txArgs SendTxArgs
if err := json.Unmarshal(tc.data, &txArgs); err != nil {
t.Fatal(err)
}
if have := txArgs.ToTransaction().Type(); have != tc.wantType {
t.Errorf("test %d, have type %d, want type %d", i, have, tc.wantType)
}
if have := txArgs.ToTransaction().Hash(); have != tc.want {
t.Errorf("test %d: have %v, want %v", i, have, tc.want)
}
d2, err := json.Marshal(txArgs)
if err != nil {
t.Fatal(err)
}
var txArgs2 SendTxArgs
if err := json.Unmarshal(d2, &txArgs2); err != nil {
t.Fatal(err)
}
if have, want := txArgs.ToTransaction().Hash(), txArgs2.ToTransaction().Hash(); have != want {
t.Errorf("test %d: have %v, want %v", i, have, want)
}
}
/*
End to end testing:

$ go run ./cmd/clef --advanced --suppress-bootwarn

$ go run ./cmd/geth --nodiscover --maxpeers 0 --signer /home/user/.clef/clef.ipc console

> tx={"from":"0x1b442286e32ddcaa6e2570ce9ed85f4b4fc87425","to":"0x1b442286e32ddcaa6e2570ce9ed85f4b4fc87425","gas":"0x124f8","maxFeePerGas":"0x6fc23ac00","maxPriorityFeePerGas":"0x3b9aca00","value":"0x0","nonce":"0x0","input":"0x","accessList":[],"maxFeePerBlobGas":"0x3b9aca00","blobVersionedHashes":["0x010657f37554c781402a22917dee2f75def7ab966d7b770905398eba3c444014"]}
> eth.signTransaction(tx)
*/
}
Loading