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.

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

cmd/devp2p/internal/ethtest: update eth test suite #21615

Merged
merged 13 commits into from
Oct 7, 2020
53 changes: 53 additions & 0 deletions cmd/devp2p/internal/ethtest/chain.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
// Copyright 2020 The go-ethereum Authors
// This file is part of the go-ethereum library.
//
// The go-ethereum library is free software: you can redistribute it and/or modify
// it under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// The go-ethereum library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

package ethtest

import (
Expand Down Expand Up @@ -68,6 +84,43 @@ func (c *Chain) Head() *types.Block {
return c.blocks[c.Len()-1]
}

func (c *Chain) GetHeaders(req GetBlockHeaders) (BlockHeaders, error) {
if req.Amount < 1 {
return nil, fmt.Errorf("no block headers requested")
}

headers := make(BlockHeaders, req.Amount)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to place an upper cap on Amount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just spoke with Felix about this and he thinks that it's okay to panic in a circumstance where the request amount is too large for the test to manually handle. We can put an explicit panic here too. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I @fjl thinks so, then ok

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's OK because panic here will just fail the test. If we try to handle it gracefully here, we won't notice if a client requests too much.

var blockNumber uint64

// range over blocks to check if our chain has the requested header
for _, block := range c.blocks {
if block.Hash() == req.Origin.Hash || block.Number().Uint64() == req.Origin.Number {
headers[0] = block.Header()
blockNumber = block.Number().Uint64()
}
}
if headers[0] == nil {
return nil, fmt.Errorf("no headers found for given origin number %v, hash %v", req.Origin.Number, req.Origin.Hash)
}

if req.Reverse {
for i := 1; i < int(req.Amount); i++ {
blockNumber -= (1 - req.Skip)
headers[i] = c.blocks[blockNumber].Header()
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably crash, if e..g someone reqeusts reverse from block 1, step 256

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same goes for this circumstance as the above reply -- let me know what you think.


}

return headers, nil
}

for i := 1; i < int(req.Amount); i++ {
blockNumber += (1 + req.Skip)
headers[i] = c.blocks[blockNumber].Header()
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we don't have it? That will be a nil deref, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes out of range. But I can't really see a circumstance in an isolated test (where my test is the only peer) that a node should request block headers from me that I have not announced / the node does not know about. We have a test (Broadcast) which does test that a block announcement will be adequately handled by the node in which case the test suite will update its own chain with the announced block (and would technically be able to provide the header if the node asks)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess it's fine -- if we notice it panicing we can fix it later

}

return headers, nil
}

// loadChain takes the given chain.rlp file, and decodes and returns
// the blocks from the file.
func loadChain(chainfile string, genesis string) (*Chain, error) {
Expand Down
150 changes: 150 additions & 0 deletions cmd/devp2p/internal/ethtest/chain_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
// Copyright 2020 The go-ethereum Authors
// This file is part of the go-ethereum library.
//
// The go-ethereum library is free software: you can redistribute it and/or modify
// it under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// The go-ethereum library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

package ethtest

import (
"path/filepath"
"strconv"
"testing"

"github.com/ethereum/go-ethereum/p2p"
"github.com/stretchr/testify/assert"
)

// TestEthProtocolNegotiation tests whether the test suite
// can negotiate the highest eth protocol in a status message exchange
func TestEthProtocolNegotiation(t *testing.T) {
var tests = []struct {
conn *Conn
caps []p2p.Cap
expected uint32
}{
{
conn: &Conn{},
caps: []p2p.Cap{
{Name: "eth", Version: 63},
{Name: "eth", Version: 64},
{Name: "eth", Version: 65},
},
expected: uint32(65),
},
{
conn: &Conn{},
caps: []p2p.Cap{
{Name: "eth", Version: 0},
{Name: "eth", Version: 89},
{Name: "eth", Version: 65},
},
expected: uint32(65),
},
{
conn: &Conn{},
caps: []p2p.Cap{
{Name: "eth", Version: 63},
{Name: "eth", Version: 64},
{Name: "wrongProto", Version: 65},
},
expected: uint32(64),
},
}

for i, tt := range tests {
t.Run(strconv.Itoa(i), func(t *testing.T) {
tt.conn.negotiateEthProtocol(tt.caps)
assert.Equal(t, tt.expected, uint32(tt.conn.ethProtocolVersion))
})
}
}

// TestChain_GetHeaders tests whether the test suite can correctly
// respond to a GetBlockHeaders request from a node.
func TestChain_GetHeaders(t *testing.T) {
chainFile, err := filepath.Abs("./testdata/chain.rlp.gz")
if err != nil {
t.Fatal(err)
}
genesisFile, err := filepath.Abs("./testdata/genesis.json")
if err != nil {
t.Fatal(err)
}

chain, err := loadChain(chainFile, genesisFile)
if err != nil {
t.Fatal(err)
}

var tests = []struct {
req GetBlockHeaders
expected BlockHeaders
}{
{
req: GetBlockHeaders{
Origin: hashOrNumber{
Number: uint64(2),
},
Amount: uint64(5),
Skip: 1,
Reverse: false,
},
expected: BlockHeaders{
chain.blocks[2].Header(),
chain.blocks[4].Header(),
chain.blocks[6].Header(),
chain.blocks[8].Header(),
chain.blocks[10].Header(),
},
},
{
req: GetBlockHeaders{
Origin: hashOrNumber{
Number: uint64(chain.Len() - 1),
},
Amount: uint64(3),
Skip: 0,
Reverse: true,
},
expected: BlockHeaders{
chain.blocks[chain.Len()-1].Header(),
chain.blocks[chain.Len()-2].Header(),
chain.blocks[chain.Len()-3].Header(),
},
},
{
req: GetBlockHeaders{
Origin: hashOrNumber{
Hash: chain.Head().Hash(),
},
Amount: uint64(1),
Skip: 0,
Reverse: false,
},
expected: BlockHeaders{
chain.Head().Header(),
},
},
}

for i, tt := range tests {
t.Run(strconv.Itoa(i), func(t *testing.T) {
headers, err := chain.GetHeaders(tt.req)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, headers, tt.expected)
})
}
}
Loading