From bc0b87ca196f92e5af49bd33cc190ef0ec32b197 Mon Sep 17 00:00:00 2001 From: alex <152680487+bodhi-crypo@users.noreply.github.com> Date: Fri, 26 Jan 2024 15:57:04 +0800 Subject: [PATCH 1/6] internal/flags: fix typo (#28876) --- internal/flags/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/flags/helpers.go b/internal/flags/helpers.go index 369a931e8a..0112724fa1 100644 --- a/internal/flags/helpers.go +++ b/internal/flags/helpers.go @@ -115,7 +115,7 @@ func doMigrateFlags(ctx *cli.Context) { for _, parent := range ctx.Lineage()[1:] { if parent.IsSet(name) { // When iterating across the lineage, we will be served both - // the 'canon' and alias formats of all commmands. In most cases, + // the 'canon' and alias formats of all commands. In most cases, // it's fine to set it in the ctx multiple times (one for each // name), however, the Slice-flags are not fine. // The slice-flags accumulate, so if we set it once as From 2e947b7a0041f087ce4945303f3dd267b6296a14 Mon Sep 17 00:00:00 2001 From: protolambda Date: Sat, 27 Jan 2024 14:16:20 -0600 Subject: [PATCH 2/6] core/types: fix and test handling of faulty nil-returning signer (#28879) This adds an error if the signer returns a nil value for one of the signature value fields. --- core/types/transaction.go | 5 +++ core/types/transaction_signing_test.go | 52 ++++++++++++++++++++++++++ core/types/tx_blob_test.go | 9 ++++- 3 files changed, 64 insertions(+), 2 deletions(-) diff --git a/core/types/transaction.go b/core/types/transaction.go index 9ec0199a03..7d2e9d5325 100644 --- a/core/types/transaction.go +++ b/core/types/transaction.go @@ -19,6 +19,7 @@ package types import ( "bytes" "errors" + "fmt" "io" "math/big" "sync/atomic" @@ -320,6 +321,7 @@ func (tx *Transaction) Cost() *big.Int { // RawSignatureValues returns the V, R, S signature values of the transaction. // The return values should not be modified by the caller. +// The return values may be nil or zero, if the transaction is unsigned. func (tx *Transaction) RawSignatureValues() (v, r, s *big.Int) { return tx.inner.rawSignatureValues() } @@ -508,6 +510,9 @@ func (tx *Transaction) WithSignature(signer Signer, sig []byte) (*Transaction, e if err != nil { return nil, err } + if r == nil || s == nil || v == nil { + return nil, fmt.Errorf("%w: r: %s, s: %s, v: %s", ErrInvalidSig, r, s, v) + } cpy := tx.inner.copy() cpy.setSignatureValues(signer.ChainID(), v, r, s) return &Transaction{inner: cpy, time: tx.time}, nil diff --git a/core/types/transaction_signing_test.go b/core/types/transaction_signing_test.go index 2a9ceb0952..61b78fe029 100644 --- a/core/types/transaction_signing_test.go +++ b/core/types/transaction_signing_test.go @@ -18,11 +18,13 @@ package types import ( "errors" + "fmt" "math/big" "testing" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" + "github.com/ethereum/go-ethereum/params" "github.com/ethereum/go-ethereum/rlp" ) @@ -136,3 +138,53 @@ func TestChainId(t *testing.T) { t.Error("expected no error") } } + +type nilSigner struct { + v, r, s *big.Int + Signer +} + +func (ns *nilSigner) SignatureValues(tx *Transaction, sig []byte) (r, s, v *big.Int, err error) { + return ns.v, ns.r, ns.s, nil +} + +// TestNilSigner ensures a faulty Signer implementation does not result in nil signature values or panics. +func TestNilSigner(t *testing.T) { + key, _ := crypto.GenerateKey() + innerSigner := LatestSignerForChainID(big.NewInt(1)) + for i, signer := range []Signer{ + &nilSigner{v: nil, r: nil, s: nil, Signer: innerSigner}, + &nilSigner{v: big.NewInt(1), r: big.NewInt(1), s: nil, Signer: innerSigner}, + &nilSigner{v: big.NewInt(1), r: nil, s: big.NewInt(1), Signer: innerSigner}, + &nilSigner{v: nil, r: big.NewInt(1), s: big.NewInt(1), Signer: innerSigner}, + } { + t.Run(fmt.Sprintf("signer_%d", i), func(t *testing.T) { + t.Run("legacy", func(t *testing.T) { + legacyTx := createTestLegacyTxInner() + _, err := SignNewTx(key, signer, legacyTx) + if !errors.Is(err, ErrInvalidSig) { + t.Fatal("expected signature values error, no nil result or panic") + } + }) + // test Blob tx specifically, since the signature value types changed + t.Run("blobtx", func(t *testing.T) { + blobtx := createEmptyBlobTxInner(false) + _, err := SignNewTx(key, signer, blobtx) + if !errors.Is(err, ErrInvalidSig) { + t.Fatal("expected signature values error, no nil result or panic") + } + }) + }) + } +} + +func createTestLegacyTxInner() *LegacyTx { + return &LegacyTx{ + Nonce: uint64(0), + To: nil, + Value: big.NewInt(0), + Gas: params.TxGas, + GasPrice: big.NewInt(params.GWei), + Data: nil, + } +} diff --git a/core/types/tx_blob_test.go b/core/types/tx_blob_test.go index 44ac48cc6f..25d09e31ce 100644 --- a/core/types/tx_blob_test.go +++ b/core/types/tx_blob_test.go @@ -65,6 +65,12 @@ var ( ) func createEmptyBlobTx(key *ecdsa.PrivateKey, withSidecar bool) *Transaction { + blobtx := createEmptyBlobTxInner(withSidecar) + signer := NewCancunSigner(blobtx.ChainID.ToBig()) + return MustSignNewTx(key, signer, blobtx) +} + +func createEmptyBlobTxInner(withSidecar bool) *BlobTx { sidecar := &BlobTxSidecar{ Blobs: []kzg4844.Blob{emptyBlob}, Commitments: []kzg4844.Commitment{emptyBlobCommit}, @@ -85,6 +91,5 @@ func createEmptyBlobTx(key *ecdsa.PrivateKey, withSidecar bool) *Transaction { if withSidecar { blobtx.Sidecar = sidecar } - signer := NewCancunSigner(blobtx.ChainID.ToBig()) - return MustSignNewTx(key, signer, blobtx) + return blobtx } From db98cc485e5b8fb060ef3a86b5e64be9d8f0afda Mon Sep 17 00:00:00 2001 From: KeienWang <42377006+keienWang@users.noreply.github.com> Date: Mon, 29 Jan 2024 17:58:43 +0800 Subject: [PATCH 3/6] README.md: fix travis badge (#28889) The hyperlink in the README file that directs to the Travis CI build was broken. This commit updates the link to point to the corrent build page. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 64f272f1a6..1e8dba8090 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ Golang execution layer implementation of the Ethereum protocol. https://pkg.go.dev/badge/github.com/ethereum/go-ethereum )](https://pkg.go.dev/github.com/ethereum/go-ethereum?tab=doc) [![Go Report Card](https://goreportcard.com/badge/github.com/ethereum/go-ethereum)](https://goreportcard.com/report/github.com/ethereum/go-ethereum) -[![Travis](https://travis-ci.com/ethereum/go-ethereum.svg?branch=master)](https://travis-ci.com/ethereum/go-ethereum) +[![Travis](https://app.travis-ci.com/ethereum/go-ethereum.svg?branch=master)](https://app.travis-ci.com/github/ethereum/go-ethereum) [![Discord](https://img.shields.io/badge/discord-join%20chat-blue.svg)](https://discord.gg/nthXNEv) Automated builds are available for stable releases and the unstable master branch. Binary From e2778cd59f04f7587c9aa5983282074026ff6684 Mon Sep 17 00:00:00 2001 From: lightclient <14004106+lightclient@users.noreply.github.com> Date: Mon, 29 Jan 2024 03:53:25 -0700 Subject: [PATCH 4/6] eth/catalyst: allow payload attributes v1 in fcu v2 (#28882) At some point, `ForkchoiceUpdatedV2` stopped working for `PayloadAttributesV1` while `paris` was active. This was causing a few failures in hive. This PR fixes that, and also adds a gate in `ForkchoiceUpdatedV1` to disallow `PayloadAttributesV3`. --- eth/catalyst/api.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 87a9731fdf..c48a7d0e49 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -173,8 +173,8 @@ func newConsensusAPIWithoutHeartbeat(eth *eth.Ethereum) *ConsensusAPI { // and return its payloadID. func (api *ConsensusAPI) ForkchoiceUpdatedV1(update engine.ForkchoiceStateV1, payloadAttributes *engine.PayloadAttributes) (engine.ForkChoiceResponse, error) { if payloadAttributes != nil { - if payloadAttributes.Withdrawals != nil { - return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("withdrawals not supported in V1")) + if payloadAttributes.Withdrawals != nil || payloadAttributes.BeaconRoot != nil { + return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("withdrawals and beacon root not supported in V1")) } if api.eth.BlockChain().Config().IsShanghai(api.eth.BlockChain().Config().LondonBlock, payloadAttributes.Timestamp) { return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("forkChoiceUpdateV1 called post-shanghai")) @@ -183,23 +183,31 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update engine.ForkchoiceStateV1, pa return api.forkchoiceUpdated(update, payloadAttributes, engine.PayloadV1, false) } -// ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload attributes. +// ForkchoiceUpdatedV2 is equivalent to V1 with the addition of withdrawals in the payload +// attributes. It supports both PayloadAttributesV1 and PayloadAttributesV2. func (api *ConsensusAPI) ForkchoiceUpdatedV2(update engine.ForkchoiceStateV1, params *engine.PayloadAttributes) (engine.ForkChoiceResponse, error) { if params != nil { - if params.Withdrawals == nil { - return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("missing withdrawals")) + switch api.eth.BlockChain().Config().LatestFork(params.Timestamp) { + case forks.Paris: + if params.Withdrawals != nil { + return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("withdrawals before shanghai")) + } + case forks.Shanghai: + if params.Withdrawals == nil { + return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("missing withdrawals")) + } + default: + return engine.STATUS_INVALID, engine.UnsupportedFork.With(errors.New("forkchoiceUpdatedV2 must only be called with paris and shanghai payloads")) } if params.BeaconRoot != nil { return engine.STATUS_INVALID, engine.InvalidParams.With(errors.New("unexpected beacon root")) } - if api.eth.BlockChain().Config().LatestFork(params.Timestamp) != forks.Shanghai { - return engine.STATUS_INVALID, engine.UnsupportedFork.With(errors.New("forkchoiceUpdatedV2 must only be called for shanghai payloads")) - } } return api.forkchoiceUpdated(update, params, engine.PayloadV2, false) } -// ForkchoiceUpdatedV3 is equivalent to V2 with the addition of parent beacon block root in the payload attributes. +// ForkchoiceUpdatedV3 is equivalent to V2 with the addition of parent beacon block root +// in the payload attributes. It supports only PayloadAttributesV3. func (api *ConsensusAPI) ForkchoiceUpdatedV3(update engine.ForkchoiceStateV1, params *engine.PayloadAttributes) (engine.ForkChoiceResponse, error) { if params != nil { // TODO(matt): according to https://github.com/ethereum/execution-apis/pull/498, From fc380f52ef9778e988266f776b9593ce719cf79d Mon Sep 17 00:00:00 2001 From: KeienWang <42377006+keienWang@users.noreply.github.com> Date: Mon, 29 Jan 2024 23:40:57 +0800 Subject: [PATCH 5/6] docs/postmortems: fix outdated link (#28893) --- docs/postmortems/2021-08-22-split-postmortem.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/postmortems/2021-08-22-split-postmortem.md b/docs/postmortems/2021-08-22-split-postmortem.md index 962aa51f64..0986f00b65 100644 --- a/docs/postmortems/2021-08-22-split-postmortem.md +++ b/docs/postmortems/2021-08-22-split-postmortem.md @@ -87,7 +87,7 @@ The blocks on the 'bad' chain were investigated, and Tim Beiko reached out to th ### Disclosure decision -The geth-team have an official policy regarding [vulnerability disclosure](https://geth.ethereum.org/docs/vulnerabilities/vulnerabilities). +The geth-team have an official policy regarding [vulnerability disclosure](https://geth.ethereum.org/docs/developers/geth-developer/disclosures). > The primary goal for the Geth team is the health of the Ethereum network as a whole, and the decision whether or not to publish details about a serious vulnerability boils down to minimizing the risk and/or impact of discovery and exploitation. From eaac53ec383342fa6ef9c333659d40f7c5dac108 Mon Sep 17 00:00:00 2001 From: rjl493456442 Date: Tue, 30 Jan 2024 09:34:14 +0800 Subject: [PATCH 6/6] core: reset tx lookup cache if necessary (#28865) This pull request resets the txlookup cache if chain reorg happens, preventing them from remaining reachable. It addresses failures in the hive tests. --- core/blockchain.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/core/blockchain.go b/core/blockchain.go index 93c40591c6..b45ac8e643 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -2188,6 +2188,12 @@ func (bc *BlockChain) reorg(oldHead *types.Header, newHead *types.Block) error { // rewind the canonical chain to a lower point. log.Error("Impossible reorg, please file an issue", "oldnum", oldBlock.Number(), "oldhash", oldBlock.Hash(), "oldblocks", len(oldChain), "newnum", newBlock.Number(), "newhash", newBlock.Hash(), "newblocks", len(newChain)) } + // Reset the tx lookup cache in case to clear stale txlookups. + // This is done before writing any new chain data to avoid the + // weird scenario that canonical chain is changed while the + // stale lookups are still cached. + bc.txLookupCache.Purge() + // Insert the new chain(except the head block(reverse order)), // taking care of the proper incremental order. for i := len(newChain) - 1; i >= 1; i-- { @@ -2202,11 +2208,13 @@ func (bc *BlockChain) reorg(oldHead *types.Header, newHead *types.Block) error { // Delete useless indexes right now which includes the non-canonical // transaction indexes, canonical chain indexes which above the head. - indexesBatch := bc.db.NewBatch() - for _, tx := range types.HashDifference(deletedTxs, addedTxs) { + var ( + indexesBatch = bc.db.NewBatch() + diffs = types.HashDifference(deletedTxs, addedTxs) + ) + for _, tx := range diffs { rawdb.DeleteTxLookupEntry(indexesBatch, tx) } - // Delete all hash markers that are not part of the new canonical chain. // Because the reorg function does not handle new chain head, all hash // markers greater than or equal to new chain head should be deleted.