From 1a571b5ebd99d73c493676e888b6c98195f5cb57 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Thu, 2 Mar 2023 13:33:35 +0100 Subject: [PATCH 1/7] core, eth, graphql: fix race condition --- core/types/block.go | 1 + eth/catalyst/api_test.go | 9 +++++---- graphql/graphql.go | 16 ++++++++++------ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/core/types/block.go b/core/types/block.go index 82ad3ce99cb7..e2c71abebd70 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -279,6 +279,7 @@ func CopyHeader(h *Header) *Header { copy(cpy.Extra, h.Extra) } if h.WithdrawalsHash != nil { + cpy.WithdrawalsHash = new(common.Hash) *cpy.WithdrawalsHash = *h.WithdrawalsHash } return &cpy diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index f1af087cf00a..4a330bda9b71 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -443,14 +443,15 @@ func startEthService(t *testing.T, genesis *core.Genesis, blocks []*types.Block) if err != nil { t.Fatal("can't create eth service:", err) } - if err := n.Start(); err != nil { - t.Fatal("can't start node:", err) - } if _, err := ethservice.BlockChain().InsertChain(blocks); err != nil { n.Close() t.Fatal("can't import test blocks:", err) } + if err := n.Start(); err != nil { + t.Fatal("can't start node:", err) + } + ethservice.SetEtherbase(testAddr) ethservice.SetSynced() return n, ethservice @@ -874,7 +875,7 @@ func TestNewPayloadOnInvalidTerminalBlock(t *testing.T) { n, ethservice := startEthService(t, genesis, preMergeBlocks) defer n.Close() - genesis.Config.TerminalTotalDifficulty = preMergeBlocks[0].Difficulty() //.Sub(genesis.Config.TerminalTotalDifficulty, preMergeBlocks[len(preMergeBlocks)-1].Difficulty()) + ethservice.BlockChain().Config().TerminalTotalDifficulty = preMergeBlocks[len(preMergeBlocks)-1].Difficulty() //.Sub(genesis.Config.TerminalTotalDifficulty, preMergeBlocks[len(preMergeBlocks)-1].Difficulty()) var ( api = NewConsensusAPI(ethservice) diff --git a/graphql/graphql.go b/graphql/graphql.go index 0c13cc80f555..74e71a203522 100644 --- a/graphql/graphql.go +++ b/graphql/graphql.go @@ -457,22 +457,26 @@ func (t *Transaction) Logs(ctx context.Context) (*[]*Log, error) { if t.block == nil { return nil, nil } - if _, ok := t.block.numberOrHash.Hash(); !ok { + hash, ok := t.block.numberOrHash.Hash() + if !ok { header, err := t.r.backend.HeaderByNumberOrHash(ctx, *t.block.numberOrHash) if err != nil { return nil, err } - hash := header.Hash() - t.block.numberOrHash.BlockHash = &hash + hash = header.Hash() + // TODO: (Marius) not setting the blockhash here + // prevents a race condition (since multiple calls of Logs can set the blockhash) + // It however also means that every subsequent call for logs of the same block hash to + // query the node again. Solution would be to mutex numberOrHash + // t.block.numberOrHash.BlockHash = &hash } - return t.getLogs(ctx) + return t.getLogs(ctx, hash) } // getLogs returns log objects for the given tx. // Assumes block hash is resolved. -func (t *Transaction) getLogs(ctx context.Context) (*[]*Log, error) { +func (t *Transaction) getLogs(ctx context.Context, hash common.Hash) (*[]*Log, error) { var ( - hash, _ = t.block.numberOrHash.Hash() filter = t.r.filterSystem.NewBlockFilter(hash, nil, nil) logs, err = filter.Logs(ctx) ) From e60f668fdd9f7978105508bb919815fc8f7a3a77 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Fri, 3 Mar 2023 16:01:05 +0330 Subject: [PATCH 2/7] hacky fix for blockhash data race in graphql --- graphql/graphql.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/graphql/graphql.go b/graphql/graphql.go index 74e71a203522..332c2eec38cf 100644 --- a/graphql/graphql.go +++ b/graphql/graphql.go @@ -457,8 +457,13 @@ func (t *Transaction) Logs(ctx context.Context) (*[]*Log, error) { if t.block == nil { return nil, nil } - hash, ok := t.block.numberOrHash.Hash() - if !ok { + hash, err := t.block.Hash(ctx) + if err != nil { + return nil, err + } + // This is a sanity check. Practically block hash + // should be filled already. + if (hash == common.Hash{}) { header, err := t.r.backend.HeaderByNumberOrHash(ctx, *t.block.numberOrHash) if err != nil { return nil, err @@ -474,7 +479,6 @@ func (t *Transaction) Logs(ctx context.Context) (*[]*Log, error) { } // getLogs returns log objects for the given tx. -// Assumes block hash is resolved. func (t *Transaction) getLogs(ctx context.Context, hash common.Hash) (*[]*Log, error) { var ( filter = t.r.filterSystem.NewBlockFilter(hash, nil, nil) @@ -591,10 +595,10 @@ func (b *Block) resolve(ctx context.Context) (*types.Block, error) { } var err error b.block, err = b.r.backend.BlockByNumberOrHash(ctx, *b.numberOrHash) - if b.block != nil && b.header == nil { - b.header = b.block.Header() - if hash, ok := b.numberOrHash.Hash(); ok { - b.hash = hash + if b.block != nil { + b.hash = b.block.Hash() + if b.header == nil { + b.header = b.block.Header() } } return b.block, err @@ -613,6 +617,7 @@ func (b *Block) resolveHeader(ctx context.Context) (*types.Header, error) { b.header, err = b.r.backend.HeaderByHash(ctx, b.hash) } else { b.header, err = b.r.backend.HeaderByNumberOrHash(ctx, *b.numberOrHash) + b.hash = b.header.Hash() } } return b.header, err From 4e7c39b771c2d25fb0cc6d12b46ea311228b37f2 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Fri, 3 Mar 2023 16:26:49 +0330 Subject: [PATCH 3/7] graphql: set hash in tx.resolve --- graphql/graphql.go | 1 + 1 file changed, 1 insertion(+) diff --git a/graphql/graphql.go b/graphql/graphql.go index 332c2eec38cf..4e3b71d19b58 100644 --- a/graphql/graphql.go +++ b/graphql/graphql.go @@ -202,6 +202,7 @@ func (t *Transaction) resolve(ctx context.Context) (*types.Transaction, error) { t.block = &Block{ r: t.r, numberOrHash: &blockNrOrHash, + hash: blockHash, } t.index = index return t.tx, nil From a70db38e388861052ed13786f644eac9069137ba Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Fri, 3 Mar 2023 13:39:54 +0100 Subject: [PATCH 4/7] graphql: get rid of unnecessary fix --- graphql/graphql.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/graphql/graphql.go b/graphql/graphql.go index 4e3b71d19b58..f7f05dba6e4a 100644 --- a/graphql/graphql.go +++ b/graphql/graphql.go @@ -462,20 +462,6 @@ func (t *Transaction) Logs(ctx context.Context) (*[]*Log, error) { if err != nil { return nil, err } - // This is a sanity check. Practically block hash - // should be filled already. - if (hash == common.Hash{}) { - header, err := t.r.backend.HeaderByNumberOrHash(ctx, *t.block.numberOrHash) - if err != nil { - return nil, err - } - hash = header.Hash() - // TODO: (Marius) not setting the blockhash here - // prevents a race condition (since multiple calls of Logs can set the blockhash) - // It however also means that every subsequent call for logs of the same block hash to - // query the node again. Solution would be to mutex numberOrHash - // t.block.numberOrHash.BlockHash = &hash - } return t.getLogs(ctx, hash) } From 0a364c15de4815fb938b3d63f5794005052545d7 Mon Sep 17 00:00:00 2001 From: Sina Mahmoodi Date: Fri, 3 Mar 2023 16:39:36 +0330 Subject: [PATCH 5/7] graphql: avoid writing to block --- graphql/graphql.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/graphql/graphql.go b/graphql/graphql.go index f7f05dba6e4a..6c03925180b2 100644 --- a/graphql/graphql.go +++ b/graphql/graphql.go @@ -458,9 +458,9 @@ func (t *Transaction) Logs(ctx context.Context) (*[]*Log, error) { if t.block == nil { return nil, nil } - hash, err := t.block.Hash(ctx) - if err != nil { - return nil, err + hash := t.block.readHash() + if hash == (common.Hash{}) { + return nil, errors.New("block hash not available") } return t.getLogs(ctx, hash) } @@ -651,6 +651,19 @@ func (b *Block) Hash(ctx context.Context) (common.Hash, error) { return b.hash, nil } +func (b *Block) readHash() common.Hash { + if b.hash != (common.Hash{}) { + return b.hash + } + if h, ok := b.numberOrHash.Hash(); ok { + return h + } + if b.header != nil { + return b.header.Hash() + } + return common.Hash{} +} + func (b *Block) GasLimit(ctx context.Context) (Long, error) { header, err := b.resolveHeader(ctx) if err != nil { From f138d2eadac7a4636b60cbd5888421e543828ebb Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Mon, 6 Mar 2023 10:23:36 +0100 Subject: [PATCH 6/7] graphql: split graphql fixes out --- graphql/graphql.go | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/graphql/graphql.go b/graphql/graphql.go index 6c03925180b2..0c13cc80f555 100644 --- a/graphql/graphql.go +++ b/graphql/graphql.go @@ -202,7 +202,6 @@ func (t *Transaction) resolve(ctx context.Context) (*types.Transaction, error) { t.block = &Block{ r: t.r, numberOrHash: &blockNrOrHash, - hash: blockHash, } t.index = index return t.tx, nil @@ -458,16 +457,22 @@ func (t *Transaction) Logs(ctx context.Context) (*[]*Log, error) { if t.block == nil { return nil, nil } - hash := t.block.readHash() - if hash == (common.Hash{}) { - return nil, errors.New("block hash not available") + if _, ok := t.block.numberOrHash.Hash(); !ok { + header, err := t.r.backend.HeaderByNumberOrHash(ctx, *t.block.numberOrHash) + if err != nil { + return nil, err + } + hash := header.Hash() + t.block.numberOrHash.BlockHash = &hash } - return t.getLogs(ctx, hash) + return t.getLogs(ctx) } // getLogs returns log objects for the given tx. -func (t *Transaction) getLogs(ctx context.Context, hash common.Hash) (*[]*Log, error) { +// Assumes block hash is resolved. +func (t *Transaction) getLogs(ctx context.Context) (*[]*Log, error) { var ( + hash, _ = t.block.numberOrHash.Hash() filter = t.r.filterSystem.NewBlockFilter(hash, nil, nil) logs, err = filter.Logs(ctx) ) @@ -582,10 +587,10 @@ func (b *Block) resolve(ctx context.Context) (*types.Block, error) { } var err error b.block, err = b.r.backend.BlockByNumberOrHash(ctx, *b.numberOrHash) - if b.block != nil { - b.hash = b.block.Hash() - if b.header == nil { - b.header = b.block.Header() + if b.block != nil && b.header == nil { + b.header = b.block.Header() + if hash, ok := b.numberOrHash.Hash(); ok { + b.hash = hash } } return b.block, err @@ -604,7 +609,6 @@ func (b *Block) resolveHeader(ctx context.Context) (*types.Header, error) { b.header, err = b.r.backend.HeaderByHash(ctx, b.hash) } else { b.header, err = b.r.backend.HeaderByNumberOrHash(ctx, *b.numberOrHash) - b.hash = b.header.Hash() } } return b.header, err @@ -651,19 +655,6 @@ func (b *Block) Hash(ctx context.Context) (common.Hash, error) { return b.hash, nil } -func (b *Block) readHash() common.Hash { - if b.hash != (common.Hash{}) { - return b.hash - } - if h, ok := b.numberOrHash.Hash(); ok { - return h - } - if b.header != nil { - return b.header.Hash() - } - return common.Hash{} -} - func (b *Block) GasLimit(ctx context.Context) (Long, error) { header, err := b.resolveHeader(ctx) if err != nil { From 7d805c2babb97e2b0456f08afbad7c003ce422ad Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Mon, 6 Mar 2023 15:32:04 +0100 Subject: [PATCH 7/7] eth/catalyst: reduce diff --- eth/catalyst/api_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/eth/catalyst/api_test.go b/eth/catalyst/api_test.go index 4a330bda9b71..09972691729e 100644 --- a/eth/catalyst/api_test.go +++ b/eth/catalyst/api_test.go @@ -443,15 +443,14 @@ func startEthService(t *testing.T, genesis *core.Genesis, blocks []*types.Block) if err != nil { t.Fatal("can't create eth service:", err) } + if err := n.Start(); err != nil { + t.Fatal("can't start node:", err) + } if _, err := ethservice.BlockChain().InsertChain(blocks); err != nil { n.Close() t.Fatal("can't import test blocks:", err) } - if err := n.Start(); err != nil { - t.Fatal("can't start node:", err) - } - ethservice.SetEtherbase(testAddr) ethservice.SetSynced() return n, ethservice @@ -875,7 +874,7 @@ func TestNewPayloadOnInvalidTerminalBlock(t *testing.T) { n, ethservice := startEthService(t, genesis, preMergeBlocks) defer n.Close() - ethservice.BlockChain().Config().TerminalTotalDifficulty = preMergeBlocks[len(preMergeBlocks)-1].Difficulty() //.Sub(genesis.Config.TerminalTotalDifficulty, preMergeBlocks[len(preMergeBlocks)-1].Difficulty()) + ethservice.BlockChain().Config().TerminalTotalDifficulty = preMergeBlocks[0].Difficulty() //.Sub(genesis.Config.TerminalTotalDifficulty, preMergeBlocks[len(preMergeBlocks)-1].Difficulty()) var ( api = NewConsensusAPI(ethservice)