From fa03286da0070444ea4af5a6e8440fb5c5ab9da0 Mon Sep 17 00:00:00 2001 From: Ian Norden Date: Wed, 25 Mar 2020 01:40:09 -0500 Subject: [PATCH] PR review refactoring and bug fix --- consensus/clique/clique.go | 59 +++++++++++-------------------- consensus/ethash/consensus.go | 52 ++++++++++----------------- consensus/misc/forks.go | 5 +-- core/types/block.go | 28 ++++----------- core/types/transaction_signing.go | 43 +++++++--------------- eth/gasprice/gasprice.go | 57 +++++++++++++++-------------- 6 files changed, 90 insertions(+), 154 deletions(-) diff --git a/consensus/clique/clique.go b/consensus/clique/clique.go index 168d47a2a..46c5040e6 100644 --- a/consensus/clique/clique.go +++ b/consensus/clique/clique.go @@ -730,46 +730,27 @@ func CliqueRLP(header *types.Header) []byte { } func encodeSigHeader(w io.Writer, header *types.Header) { - var err error - if header.BaseFee == nil { - err = rlp.Encode(w, []interface{}{ - header.ParentHash, - header.UncleHash, - header.Coinbase, - header.Root, - header.TxHash, - header.ReceiptHash, - header.Bloom, - header.Difficulty, - header.Number, - header.GasLimit, - header.GasUsed, - header.Time, - header.Extra[:len(header.Extra)-crypto.SignatureLength], // Yes, this will panic if extra is too short - header.MixDigest, - header.Nonce, - }) - } else { - err = rlp.Encode(w, []interface{}{ - header.ParentHash, - header.UncleHash, - header.Coinbase, - header.Root, - header.TxHash, - header.ReceiptHash, - header.Bloom, - header.Difficulty, - header.Number, - header.GasLimit, - header.GasUsed, - header.Time, - header.Extra[:len(header.Extra)-crypto.SignatureLength], // Yes, this will panic if extra is too short - header.MixDigest, - header.Nonce, - header.BaseFee, - }) + headerFields := []interface{}{ + header.ParentHash, + header.UncleHash, + header.Coinbase, + header.Root, + header.TxHash, + header.ReceiptHash, + header.Bloom, + header.Difficulty, + header.Number, + header.GasLimit, + header.GasUsed, + header.Time, + header.Extra[:len(header.Extra)-crypto.SignatureLength], // Yes, this will panic if extra is too short + header.MixDigest, + header.Nonce, } - if err != nil { + if header.BaseFee != nil { + headerFields = append(headerFields, header.BaseFee) + } + if err := rlp.Encode(w, headerFields); err != nil { panic("can't encode: " + err.Error()) } } diff --git a/consensus/ethash/consensus.go b/consensus/ethash/consensus.go index e81488ae8..c5c500eca 100644 --- a/consensus/ethash/consensus.go +++ b/consensus/ethash/consensus.go @@ -613,41 +613,25 @@ func (ethash *Ethash) FinalizeAndAssemble(chain consensus.ChainReader, header *t // SealHash returns the hash of a block prior to it being sealed. func (ethash *Ethash) SealHash(header *types.Header) (hash common.Hash) { hasher := sha3.NewLegacyKeccak256() - - if header.BaseFee == nil { - rlp.Encode(hasher, []interface{}{ - header.ParentHash, - header.UncleHash, - header.Coinbase, - header.Root, - header.TxHash, - header.ReceiptHash, - header.Bloom, - header.Difficulty, - header.Number, - header.GasLimit, - header.GasUsed, - header.Time, - header.Extra, - }) - } else { - rlp.Encode(hasher, []interface{}{ - header.ParentHash, - header.UncleHash, - header.Coinbase, - header.Root, - header.TxHash, - header.ReceiptHash, - header.Bloom, - header.Difficulty, - header.Number, - header.GasLimit, - header.GasUsed, - header.Time, - header.Extra, - header.BaseFee, - }) + encodedHeader := []interface{}{ + header.ParentHash, + header.UncleHash, + header.Coinbase, + header.Root, + header.TxHash, + header.ReceiptHash, + header.Bloom, + header.Difficulty, + header.Number, + header.GasLimit, + header.GasUsed, + header.Time, + header.Extra, } + if header.BaseFee != nil { + encodedHeader = append(encodedHeader, header.BaseFee) + } + rlp.Encode(hasher, encodedHeader) hasher.Sum(hash[:0]) return hash diff --git a/consensus/misc/forks.go b/consensus/misc/forks.go index f1c4e51bd..01cb4968a 100644 --- a/consensus/misc/forks.go +++ b/consensus/misc/forks.go @@ -62,7 +62,8 @@ func VerifyEIP1559BaseFee(config *params.ChainConfig, header, parent *types.Head } return nil } - // Verify the BaseFee is valid if we are past the EIP1559 activation block + // If we are past the EIP1559 activation block verify the header's BaseFee is valid by deriving + // it from the parent header and validating that they are the same if config.IsEIP1559(header.Number) { if parent.BaseFee == nil { return errMissingParentBaseFee @@ -91,7 +92,7 @@ func VerifyEIP1559BaseFee(config *params.ChainConfig, header, parent *types.Head } expectedBaseFee.Set(new(big.Int).Add(parent.BaseFee, max)) } - if expectedBaseFee.Cmp(header.BaseFee) > 0 { + if expectedBaseFee.Cmp(header.BaseFee) != 0 { return errInvalidBaseFee } return nil diff --git a/core/types/block.go b/core/types/block.go index cd856585f..af6a0498f 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -100,26 +100,7 @@ type headerMarshaling struct { // EncodeRLP implements rlp.Encoder func (h *Header) EncodeRLP(w io.Writer) error { - if h.BaseFee == nil { - return rlp.Encode(w, []interface{}{ - h.ParentHash, - h.UncleHash, - h.Coinbase, - h.Root, - h.TxHash, - h.ReceiptHash, - h.Bloom, - h.Difficulty, - h.Number, - h.GasLimit, - h.GasUsed, - h.Time, - h.Extra, - h.MixDigest, - h.Nonce, - }) - } - return rlp.Encode(w, []interface{}{ + encodedHeader := []interface{}{ h.ParentHash, h.UncleHash, h.Coinbase, @@ -135,8 +116,11 @@ func (h *Header) EncodeRLP(w io.Writer) error { h.Extra, h.MixDigest, h.Nonce, - h.BaseFee, - }) + } + if h.BaseFee != nil { + encodedHeader = append(encodedHeader, h.BaseFee) + } + return rlp.Encode(w, encodedHeader) } // DecodeRLP implements rlp.Decoder diff --git a/core/types/transaction_signing.go b/core/types/transaction_signing.go index fdeb7e0e3..c4f23b3bb 100644 --- a/core/types/transaction_signing.go +++ b/core/types/transaction_signing.go @@ -153,28 +153,19 @@ func (s EIP155Signer) SignatureValues(tx *Transaction, sig []byte) (R, S, V *big // Hash returns the hash to be signed by the sender. // It does not uniquely identify the transaction. func (s EIP155Signer) Hash(tx *Transaction) common.Hash { - if tx.data.GasPremium == nil && tx.data.FeeCap == nil { - 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), - }) - } - return rlpHash([]interface{}{ + txFields := []interface{}{ tx.data.AccountNonce, tx.data.Price, tx.data.GasLimit, tx.data.Recipient, tx.data.Amount, tx.data.Payload, - tx.data.GasPremium, - tx.data.FeeCap, - s.chainId, uint(0), uint(0), - }) + } + if tx.data.GasPremium != nil && tx.data.FeeCap != nil { + txFields = append(txFields, tx.data.GasPremium, tx.data.FeeCap) + } + txFields = append(txFields, s.chainId, uint(0), uint(0)) + return rlpHash(txFields) } // HomesteadTransaction implements TransactionInterface using the @@ -218,26 +209,18 @@ func (fs FrontierSigner) SignatureValues(tx *Transaction, sig []byte) (r, s, v * // Hash returns the hash to be signed by the sender. // It does not uniquely identify the transaction. func (fs FrontierSigner) Hash(tx *Transaction) common.Hash { - if tx.data.GasPremium == nil && tx.data.FeeCap == nil { - return rlpHash([]interface{}{ - tx.data.AccountNonce, - tx.data.Price, - tx.data.GasLimit, - tx.data.Recipient, - tx.data.Amount, - tx.data.Payload, - }) - } - return rlpHash([]interface{}{ + txFields := []interface{}{ tx.data.AccountNonce, tx.data.Price, tx.data.GasLimit, tx.data.Recipient, tx.data.Amount, tx.data.Payload, - tx.data.GasPremium, - tx.data.FeeCap, - }) + } + if tx.data.GasPremium != nil && tx.data.FeeCap != nil { + txFields = append(txFields, tx.data.GasPremium, tx.data.FeeCap) + } + return rlpHash(txFields) } func (fs FrontierSigner) Sender(tx *Transaction) (common.Address, error) { diff --git a/eth/gasprice/gasprice.go b/eth/gasprice/gasprice.go index 91041a9cb..bbe5c86c0 100644 --- a/eth/gasprice/gasprice.go +++ b/eth/gasprice/gasprice.go @@ -418,17 +418,18 @@ func (gpo *Oracle) getBlockPrices(ctx context.Context, signer types.Signer, bloc for _, tx := range txs.txs { sender, err := types.Sender(signer, tx) - if err == nil && sender != block.Coinbase() { - price := tx.GasPrice() - if price == nil { - price = new(big.Int).Add(block.BaseFee(), tx.GasPremium()) - if price.Cmp(tx.FeeCap()) > 0 { - price.Set(tx.FeeCap()) - } - } - ch <- getBlockPricesResult{price, nil} - return + if err != nil || sender == block.Coinbase() { + continue } + price := tx.GasPrice() + if price == nil { + price = new(big.Int).Add(block.BaseFee(), tx.GasPremium()) + if price.Cmp(tx.FeeCap()) > 0 { + price.Set(tx.FeeCap()) + } + } + ch <- getBlockPricesResult{price, nil} + return } ch <- getBlockPricesResult{nil, nil} } @@ -451,17 +452,18 @@ func (gpo *Oracle) getBlockPremiums(ctx context.Context, signer types.Signer, bl for _, tx := range txs.txs { sender, err := types.Sender(signer, tx) - if err == nil && sender != block.Coinbase() { - premium := tx.GasPremium() - if premium == nil { - premium = new(big.Int).Sub(tx.GasPrice(), block.BaseFee()) - if premium.Cmp(common.Big0) < 0 { - premium.Set(common.Big0) - } - } - ch <- getBlockPremiumsResult{premium, nil} - return + if err != nil || sender == block.Coinbase() { + continue } + premium := tx.GasPremium() + if premium == nil { + premium = new(big.Int).Sub(tx.GasPrice(), block.BaseFee()) + if premium.Cmp(common.Big0) < 0 { + premium.Set(common.Big0) + } + } + ch <- getBlockPremiumsResult{premium, nil} + return } ch <- getBlockPremiumsResult{nil, nil} } @@ -482,14 +484,15 @@ func (gpo *Oracle) getBlockCaps(ctx context.Context, signer types.Signer, blockN for _, tx := range txs { sender, err := types.Sender(signer, tx) - if err == nil && sender != block.Coinbase() { - cap := tx.FeeCap() - if cap == nil { - cap = tx.GasPrice() - } - ch <- getBlockCapsResult{cap, nil} - return + if err != nil || sender == block.Coinbase() { + continue } + cap := tx.FeeCap() + if cap == nil { + cap = tx.GasPrice() + } + ch <- getBlockCapsResult{cap, nil} + return } ch <- getBlockCapsResult{nil, nil} }