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

Jump to bottom

core/types: support yParity field in JSON transactions #27744

Merged
merged 8 commits into from Aug 4, 2023
Merged

core/types: support yParity field in JSON transactions #27744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0fab0ae
core/types: support yParity field in JSON transactions
fjl Jul 18, 2023
6521309
core/types: sig -> signature
fjl Jul 18, 2023
4ac5f8c
core/types: remove nested condition
fjl Jul 19, 2023
5e96fd7
core/types: put back special handling of all zero signature values
fjl Jul 19, 2023
2a3bc6f
internal/ethapi: fix issue with yparity
fjl Jul 19, 2023
eb2188c
core/types: fix newline issue
fjl Aug 1, 2023
00679a8
internal/ethapi: fix test vectors
fjl Aug 4, 2023
ea208a3
internal/ethapi: JSON-format other test vectors as well
fjl Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 79 additions & 22 deletions core/types/transaction_marshalling.go
View file
Open in desktop
Expand Up @@ -45,11 +45,32 @@ type txJSON struct {
V *hexutil.Big `json:"v"`
R *hexutil.Big `json:"r"`
S *hexutil.Big `json:"s"`
YParity *hexutil.Uint64 `json:"yParity,omitempty"`

// Only used for encoding:
Hash common.Hash `json:"hash"`
}

// yParityValue returns the YParity value from JSON. For backwards-compatibility reasons,
// this can be given in the 'v' field or the 'yParity' field. If both exist, they must match.
func (tx *txJSON) yParityValue() (*big.Int, error) {
if tx.YParity != nil {
val := uint64(*tx.YParity)
if val != 0 && val != 1 {
return nil, errors.New("'yParity' field must be 0 or 1")
}
bigval := new(big.Int).SetUint64(val)
if tx.V != nil && tx.V.ToInt().Cmp(bigval) != 0 {
return nil, errors.New("'v' and 'yParity' fields do not match")
}
return bigval, nil
}
if tx.V != nil {
return tx.V.ToInt(), nil
}
return nil, errors.New("missing 'yParity' or 'v' field in transaction")
}

// MarshalJSON marshals as JSON with a hash.
func (tx *Transaction) MarshalJSON() ([]byte, error) {
var enc txJSON
Expand Down Expand Up @@ -85,6 +106,8 @@ func (tx *Transaction) MarshalJSON() ([]byte, error) {
enc.V = (*hexutil.Big)(itx.V)
enc.R = (*hexutil.Big)(itx.R)
enc.S = (*hexutil.Big)(itx.S)
yparity := itx.V.Uint64()
enc.YParity = (*hexutil.Uint64)(&yparity)

case *DynamicFeeTx:
enc.ChainID = (*hexutil.Big)(itx.ChainID)
Expand All @@ -99,6 +122,8 @@ func (tx *Transaction) MarshalJSON() ([]byte, error) {
enc.V = (*hexutil.Big)(itx.V)
enc.R = (*hexutil.Big)(itx.R)
enc.S = (*hexutil.Big)(itx.S)
yparity := itx.V.Uint64()
enc.YParity = (*hexutil.Uint64)(&yparity)

case *BlobTx:
enc.ChainID = (*hexutil.Big)(itx.ChainID.ToBig())
Expand All @@ -115,14 +140,17 @@ func (tx *Transaction) MarshalJSON() ([]byte, error) {
enc.V = (*hexutil.Big)(itx.V.ToBig())
enc.R = (*hexutil.Big)(itx.R.ToBig())
enc.S = (*hexutil.Big)(itx.S.ToBig())
yparity := itx.V.Uint64()
enc.YParity = (*hexutil.Uint64)(&yparity)
}
return json.Marshal(&enc)
}

// UnmarshalJSON unmarshals from JSON.
func (tx *Transaction) UnmarshalJSON(input []byte) error {
var dec txJSON
if err := json.Unmarshal(input, &dec); err != nil {
err := json.Unmarshal(input, &dec)
if err != nil {
return err
}

Expand Down Expand Up @@ -155,20 +183,23 @@ func (tx *Transaction) UnmarshalJSON(input []byte) error {
return errors.New("missing required field 'input' in transaction")
}
itx.Data = *dec.Input
if dec.V == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why change the ordering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seemed to look better to me.

Copy link
Member

Choose a reason for hiding this comment

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

Feels weird to have them out of order. v, r, s ordering is pretty widely understood.

return errors.New("missing required field 'v' in transaction")
}
itx.V = (*big.Int)(dec.V)

// signature R
if dec.R == nil {
return errors.New("missing required field 'r' in transaction")
}
itx.R = (*big.Int)(dec.R)
// signature S
if dec.S == nil {
return errors.New("missing required field 's' in transaction")
}
itx.S = (*big.Int)(dec.S)
withSignature := itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0
if withSignature {
Copy link
Member

Choose a reason for hiding this comment

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

So before this change, sanityCheckSignature would only run when any value v, r, or s was non-zero. Now it runs always, meaning users who were unmarshalling txs with all zeros will now have an error?

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 have restored this behavior now, but I don't really get it. The signature values are mandatory, but all-zero is OK?

Copy link
Member

Choose a reason for hiding this comment

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

The only place I am aware this functionality is used is in t8n to denote that the transaction should be signed using the provided secretKey:

go-ethereum/cmd/evm/testdata/8/txs.json

Lines 19 to 21 in 7f756dc

"v": "0x0",
"r": "0x0",
"s": "0x0",

Would probably be good to get rid of all-zero values at some point, but I'm not sure what else depends on this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way it doesn't matter since an all zero signature will be invalid anyway.

// signature V
if dec.V == nil {
return errors.New("missing required field 'v' in transaction")
}
itx.V = (*big.Int)(dec.V)
if itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0 {
if err := sanityCheckSignature(itx.V, itx.R, itx.S, true); err != nil {
return err
}
Expand Down Expand Up @@ -204,23 +235,26 @@ func (tx *Transaction) UnmarshalJSON(input []byte) error {
return errors.New("missing required field 'input' in transaction")
}
itx.Data = *dec.Input
if dec.V == nil {
return errors.New("missing required field 'v' in transaction")
}
if dec.AccessList != nil {
itx.AccessList = *dec.AccessList
}
itx.V = (*big.Int)(dec.V)

// signature R
if dec.R == nil {
return errors.New("missing required field 'r' in transaction")
}
itx.R = (*big.Int)(dec.R)
// signature S
if dec.S == nil {
return errors.New("missing required field 's' in transaction")
}
itx.S = (*big.Int)(dec.S)
withSignature := itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0
if withSignature {
// signature V
itx.V, err = dec.yParityValue()
if err != nil {
return err
}
if itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0 {
if err := sanityCheckSignature(itx.V, itx.R, itx.S, false); err != nil {
return err
}
Expand Down Expand Up @@ -266,17 +300,23 @@ func (tx *Transaction) UnmarshalJSON(input []byte) error {
if dec.AccessList != nil {
itx.AccessList = *dec.AccessList
}
itx.V = (*big.Int)(dec.V)

// signature R
if dec.R == nil {
return errors.New("missing required field 'r' in transaction")
}
itx.R = (*big.Int)(dec.R)
// signature S
if dec.S == nil {
return errors.New("missing required field 's' in transaction")
}
itx.S = (*big.Int)(dec.S)
withSignature := itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0
if withSignature {
// signature V
itx.V, err = dec.yParityValue()
if err != nil {
return err
}
if itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0 {
if err := sanityCheckSignature(itx.V, itx.R, itx.S, false); err != nil {
return err
}
Expand Down Expand Up @@ -331,18 +371,35 @@ func (tx *Transaction) UnmarshalJSON(input []byte) error {
return errors.New("missing required field 'blobVersionedHashes' in transaction")
}
itx.BlobHashes = dec.BlobVersionedHashes
itx.V = uint256.MustFromBig((*big.Int)(dec.V))

// signature R
var ok bool
if dec.R == nil {
return errors.New("missing required field 'r' in transaction")
}
itx.R = uint256.MustFromBig((*big.Int)(dec.R))
itx.R, ok = uint256.FromBig((*big.Int)(dec.R))
if !ok {
return errors.New("'r' value overflows uint256")
}
// signature S
if dec.S == nil {
return errors.New("missing required field 's' in transaction")
}
itx.S = uint256.MustFromBig((*big.Int)(dec.S))
withSignature := itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0
if withSignature {
if err := sanityCheckSignature(itx.V.ToBig(), itx.R.ToBig(), itx.S.ToBig(), false); err != nil {
itx.S, ok = uint256.FromBig((*big.Int)(dec.S))
if !ok {
return errors.New("'s' value overflows uint256")
}
// signature V
vbig, err := dec.yParityValue()
if err != nil {
return err
}
itx.V, ok = uint256.FromBig(vbig)
if !ok {
return errors.New("'v' value overflows uint256")
}
if itx.V.Sign() != 0 || itx.R.Sign() != 0 || itx.S.Sign() != 0 {
if err := sanityCheckSignature(vbig, itx.R.ToBig(), itx.S.ToBig(), false); err != nil {
return err
}
}
Expand Down
8 changes: 8 additions & 0 deletions internal/ethapi/api.go
View file
Open in desktop
Expand Up @@ -1375,6 +1375,7 @@ type RPCTransaction struct {
V *hexutil.Big `json:"v"`
R *hexutil.Big `json:"r"`
S *hexutil.Big `json:"s"`
YParity *hexutil.Uint64 `json:"yParity,omitempty"`
}

// newRPCTransaction returns a transaction that will serialize to the RPC
Expand Down Expand Up @@ -1402,20 +1403,27 @@ func newRPCTransaction(tx *types.Transaction, blockHash common.Hash, blockNumber
result.BlockNumber = (*hexutil.Big)(new(big.Int).SetUint64(blockNumber))
result.TransactionIndex = (*hexutil.Uint64)(&index)
}

switch tx.Type() {
case types.LegacyTxType:
// if a legacy transaction has an EIP-155 chain id, include it explicitly
if id := tx.ChainId(); id.Sign() != 0 {
result.ChainID = (*hexutil.Big)(id)
}

case types.AccessListTxType:
al := tx.AccessList()
yparity := hexutil.Uint64(v.Sign())
result.Accesses = &al
result.ChainID = (*hexutil.Big)(tx.ChainId())
result.YParity = &yparity

case types.DynamicFeeTxType:
al := tx.AccessList()
yparity := hexutil.Uint64(v.Sign())
result.Accesses = &al
result.ChainID = (*hexutil.Big)(tx.ChainId())
result.YParity = &yparity
result.GasFeeCap = (*hexutil.Big)(tx.GasFeeCap())
result.GasTipCap = (*hexutil.Big)(tx.GasTipCap())
// if the transaction has been mined, compute the effective gas price
Expand Down