Skip to content
This repository was archived by the owner on Apr 11, 2021. It is now read-only.

cleanup: Remove OVM signer #278

Open
smartcontracts wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cleanup: Remove OVM signer #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
153 changes: 10 additions & 143 deletions core/types/transaction_signing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,67 +17,16 @@
package types

import (
"bytes"
"crypto/ecdsa"
"encoding/binary"
"errors"
"fmt"
"math/big"
"strings"

"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/params"
"golang.org/x/crypto/sha3"
)

var codec abi.ABI

func init() {
const abidata = `
[
{
"type": "function",
"name": "encode",
"constant": true,
"inputs": [
{
"name": "nonce",
"type": "uint256"
},
{
"name": "gasLimit",
"type": "uint256"
},
{
"name": "gasPrice",
"type": "uint256"
},
{
"name": "chainId",
"type": "uint256"
},
{
"name": "to",
"type": "address"
},
{
"name": "data",
"type": "bytes"
}
]
}
]
`

var err error
codec, err = abi.JSON(strings.NewReader(abidata))
if err != nil {
panic(fmt.Errorf("unable to create Eth Sign abi reader: %v", err))
}
}

var (
ErrInvalidChainId = errors.New("invalid chain id for signer")
)
Expand All @@ -91,7 +40,16 @@ type sigCache struct {

// MakeSigner returns a Signer based on the given chain config and block number.
func MakeSigner(config *params.ChainConfig, blockNumber *big.Int) Signer {
return NewOVMSigner(config.ChainID)
var signer Signer
switch {
case config.IsEIP155(blockNumber):
signer = NewEIP155Signer(config.ChainID)
case config.IsHomestead(blockNumber):
signer = HomesteadSigner{}
default:
signer = FrontierSigner{}
Copy link
Copy Markdown
Collaborator

@tynes tynes Mar 11, 2021

Choose a reason for hiding this comment

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

we never want to return anything besides EIP155Signer. This would result in a diff but would give extra assurance that we never use the wrong signer

Copy link
Copy Markdown
Contributor Author

@smartcontracts smartcontracts Mar 13, 2021

Choose a reason for hiding this comment

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

What impact would using the wrong signer have?

Copy link
Copy Markdown
Collaborator

@tynes tynes Mar 17, 2021

Choose a reason for hiding this comment

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

Allowing transactions that are not protected with a chainid. All transactions must be protected with a chainid

Copy link
Copy Markdown
Contributor Author

@smartcontracts smartcontracts Mar 17, 2021

Choose a reason for hiding this comment

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

Transactions w/o chain ID should fail inside the OVM_ECDSAContractAccount.

}
return signer
}

// SignTx signs the transaction using the given signer and private key
Expand Down Expand Up @@ -144,97 +102,6 @@ type Signer interface {
Equal(Signer) bool
}

// OVMSigner implements Signers using the EIP155 rules along with a new
// `eth_sign` based signature hash.
type OVMSigner struct {
EIP155Signer
}

func NewOVMSigner(chainId *big.Int) OVMSigner {
signer := NewEIP155Signer(chainId)
return OVMSigner{signer}
}

func (s OVMSigner) Equal(s2 Signer) bool {
ovm, ok := s2.(OVMSigner)
return ok && ovm.chainId.Cmp(s.chainId) == 0
}

// Hash returns the hash to be signed by the sender.
// It does not uniquely identify the transaction.
func (s OVMSigner) Hash(tx *Transaction) common.Hash {
if tx.IsEthSignSighash() {
msg := s.OVMSignerTemplateSighashPreimage(tx)

hasher := sha3.NewLegacyKeccak256()
hasher.Write(msg[:])
digest := hasher.Sum(nil)

return common.BytesToHash(digest)
}

return rlpHash([]interface{}{
tx.data.AccountNonce,
tx.data.Price,
tx.data.GasLimit,
tx.data.Recipient,
tx.data.Amount,
tx.data.Payload,
s.chainId, uint(0), uint(0),
})
}

// Sender will ecrecover the public key that created the signature
// and then hash the public key to create an address. In the
// case of L1ToL2 transactions, Layer One did the authentication
// for us so there is no signature involved. The concept of a "from"
// is only required for bookkeeping within this codebase
func (s OVMSigner) Sender(tx *Transaction) (common.Address, error) {
qo := tx.QueueOrigin()
if qo != nil && qo.Uint64() == uint64(QueueOriginL1ToL2) {
return common.Address{}, nil
}
if !tx.Protected() {
return HomesteadSigner{}.Sender(tx)
}
if tx.ChainId().Cmp(s.chainId) != 0 {
return common.Address{}, ErrInvalidChainId
}
V := new(big.Int).Sub(tx.data.V, s.chainIdMul)
V.Sub(V, big8)
return recoverPlain(s.Hash(tx), tx.data.R, tx.data.S, V, true)
}

// OVMSignerTemplateSighashPreimage creates the preimage for the `eth_sign` like
// signature hash. The transaction is `ABI.encodePacked`.
func (s OVMSigner) OVMSignerTemplateSighashPreimage(tx *Transaction) []byte {
data := []interface{}{
big.NewInt(int64(tx.data.AccountNonce)),
big.NewInt(int64(tx.data.GasLimit)),
tx.data.Price,
s.chainId,
*tx.data.Recipient,
tx.data.Payload,
}

ret, err := codec.Pack("encode", data...)
if err != nil {
panic(fmt.Errorf("unable to pack Eth Sign data: %v", err))
}

hasher := sha3.NewLegacyKeccak256()
// Slice off the function selector before hashing
hasher.Write(ret[4:])
digest := hasher.Sum(nil)

preimage := new(bytes.Buffer)
prefix := []byte("\x19Ethereum Signed Message:\n32")
binary.Write(preimage, binary.BigEndian, prefix)
binary.Write(preimage, binary.BigEndian, digest)

return preimage.Bytes()
}

// EIP155Transaction implements Signer using the EIP155 rules.
type EIP155Signer struct {
chainId, chainIdMul *big.Int
Expand Down
97 changes: 0 additions & 97 deletions core/types/transaction_signing_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,100 +136,3 @@ func TestChainId(t *testing.T) {
t.Error("expected no error")
}
}

func TestOVMSigner(t *testing.T) {
key, _ := defaultTestKey()

tx := NewTransaction(0, common.Address{}, new(big.Int), 0, new(big.Int), nil)
txMeta := NewTransactionMeta(nil, 0, nil, SighashEthSign, QueueOriginSequencer, nil, nil, nil)
tx.SetTransactionMeta(txMeta)

var err error
tx, err = SignTx(tx, NewOVMSigner(big.NewInt(1)), key)
if err != nil {
t.Fatal(err)
}

_, err = Sender(NewOVMSigner(big.NewInt(2)), tx)
if err != ErrInvalidChainId {
t.Error("expected error:", ErrInvalidChainId)
}

_, err = Sender(NewOVMSigner(big.NewInt(1)), tx)
if err != nil {
t.Error("expected no error")
}
}

func TestOVMSignerHash(t *testing.T) {
signer := NewOVMSigner(big.NewInt(1))

txNil := NewTransaction(0, common.Address{}, new(big.Int), 0, new(big.Int), nil)
txEIP155 := NewTransaction(0, common.Address{}, new(big.Int), 0, new(big.Int), nil)

hashNil := signer.Hash(txNil)
hashEIP155 := signer.Hash(txEIP155)
if hashNil != hashEIP155 {
t.Errorf("Signature hashes should be equal: %s != %s", hashNil.Hex(), hashEIP155.Hex())
}

// The signature hash should be different when using `SighashEthSign`
txEthSign := NewTransaction(0, common.Address{}, new(big.Int), 0, new(big.Int), nil)
txMeta := NewTransactionMeta(nil, 0, nil, SighashEthSign, QueueOriginSequencer, nil, nil, nil)
txEthSign.SetTransactionMeta(txMeta)

hashEthSign := signer.Hash(txEthSign)
if hashEIP155 == hashEthSign {
t.Errorf("Signature hashes should not be equal: %s == %s", hashEIP155.Hex(), hashEthSign.Hex())
}
}

func TestOVMSignerSender(t *testing.T) {
// Create a keypair to sign transactions with and the corresponding address
// from the public key.
key, _ := crypto.GenerateKey()
addr := crypto.PubkeyToAddress(key.PublicKey)

// This test makes sure that both the EIP155 and EthSign signature hash
// codepaths work when using the OVMSigner.
signer := NewOVMSigner(big.NewInt(1))
var err error

// Create a transaction with EIP155 signature hash, sign the transaction,
// recover the address and assert that the address matches the key.
txEIP155 := NewTransaction(0, addr, new(big.Int), 0, new(big.Int), nil)

txEIP155, err = SignTx(txEIP155, signer, key)
if err != nil {
t.Errorf("No error expected")
}

recEIP155, err := signer.Sender(txEIP155)
if err != nil {
t.Errorf("No error expected")
}

if addr != recEIP155 {
t.Errorf("Recovered address doesn't match. Got %s, expected %s", recEIP155.Hex(), addr.Hex())
}

// Create a transaction with EthSign signature hash, sign the transaction,
// recover the address and assert that the address matches the key.
txEthSign := NewTransaction(0, addr, new(big.Int), 0, new(big.Int), nil)
txMeta := NewTransactionMeta(nil, 0, nil, SighashEthSign, QueueOriginSequencer, nil, nil, nil)
txEthSign.SetTransactionMeta(txMeta)

txEthSign, err = SignTx(txEthSign, signer, key)
if err != nil {
t.Errorf("No error expected")
}

recEthSign, err := signer.Sender(txEthSign)
if err != nil {
t.Errorf("No error expected")
}

if addr != recEthSign {
t.Errorf("Recovered address doesn't match. Got %s, expected %s", recEthSign.Hex(), addr.Hex())
}
}
2 changes: 1 addition & 1 deletion core/types/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ func TestTransactionJSON(t *testing.T) {
if err != nil {
t.Fatalf("could not generate key: %v", err)
}
signer := NewOVMSigner(common.Big1)
signer := NewEIP155Signer(common.Big1)

transactions := make([]*Transaction, 0, 50)
for i := uint64(0); i < 25; i++ {
Expand Down
11 changes: 7 additions & 4 deletions internal/ethapi/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,7 +1183,10 @@ type RPCTransaction struct {
// newRPCTransaction returns a transaction that will serialize to the RPC
// representation, with the given location metadata set (if available).
func newRPCTransaction(tx *types.Transaction, blockHash common.Hash, blockNumber uint64, index uint64) *RPCTransaction {
var signer types.Signer = types.NewOVMSigner(tx.ChainId())
var signer types.Signer = types.FrontierSigner{}
Copy link
Copy Markdown
Collaborator

@tynes tynes Mar 11, 2021

Choose a reason for hiding this comment

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

This is reintroducing the types.FrontierSigner{} that was removed in a recent commit

if tx.Protected() {
signer = types.NewEIP155Signer(tx.ChainId())
}
from, _ := types.Sender(signer, tx)
v, r, s := tx.RawSignatureValues()

Expand Down Expand Up @@ -1411,7 +1414,7 @@ func (s *PublicTransactionPoolAPI) GetTransactionReceipt(ctx context.Context, ha

var signer types.Signer = types.FrontierSigner{}
if tx.Protected() {
signer = types.NewOVMSigner(tx.ChainId())
signer = types.NewEIP155Signer(tx.ChainId())
}
from, _ := types.Sender(signer, tx)

Expand Down Expand Up @@ -1756,7 +1759,7 @@ func (s *PublicTransactionPoolAPI) PendingTransactions() ([]*RPCTransaction, err
for _, tx := range pending {
var signer types.Signer = types.HomesteadSigner{}
if tx.Protected() {
signer = types.NewOVMSigner(tx.ChainId())
signer = types.NewEIP155Signer(tx.ChainId())
}
from, _ := types.Sender(signer, tx)
if _, exists := accounts[from]; exists {
Expand Down Expand Up @@ -1784,7 +1787,7 @@ func (s *PublicTransactionPoolAPI) Resend(ctx context.Context, sendArgs SendTxAr
for _, p := range pending {
var signer types.Signer = types.HomesteadSigner{}
if p.Protected() {
signer = types.NewOVMSigner(p.ChainId())
signer = types.NewEIP155Signer(p.ChainId())
}
wantSigHash := signer.Hash(matchTx)

Expand Down
2 changes: 1 addition & 1 deletion miner/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ func (w *worker) makeCurrent(parent *types.Block, header *types.Header) error {
return err
}
env := &environment{
signer: types.NewOVMSigner(w.chainConfig.ChainID),
signer: types.NewEIP155Signer(w.chainConfig.ChainID),
state: state,
ancestors: mapset.NewSet(),
family: mapset.NewSet(),
Expand Down
6 changes: 3 additions & 3 deletions rollup/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ type RollupClient interface {

type Client struct {
client *resty.Client
signer *types.OVMSigner
signer *types.EIP155Signer
}

type TransactionResponse struct {
Expand All @@ -107,7 +107,7 @@ type TransactionResponse struct {
func NewClient(url string, chainID *big.Int) *Client {
client := resty.New()
client.SetHostURL(url)
signer := types.NewOVMSigner(chainID)
signer := types.NewEIP155Signer(chainID)

return &Client{
client: client,
Expand Down Expand Up @@ -217,7 +217,7 @@ func (c *Client) GetLatestEnqueue() (*types.Transaction, error) {
return tx, nil
}

func transactionResponseToTransaction(res *TransactionResponse, signer *types.OVMSigner) (*types.Transaction, error) {
func transactionResponseToTransaction(res *TransactionResponse, signer *types.EIP155Signer) (*types.Transaction, error) {
// `nil` transactions are not found
if res.Transaction == nil {
return nil, nil
Expand Down