From 904a3115a623ad0b7a30d87d3472339ae7d1b5dc Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 22 Mar 2018 11:27:10 +0100 Subject: [PATCH] require addresses to have an ID by default Refs #1228 --- CHANGELOG.md | 9 +++-- p2p/listener.go | 2 +- p2p/netaddress.go | 37 +++++++++++++-------- p2p/netaddress_test.go | 75 ++++++++++++++++++++++++++---------------- 4 files changed, 77 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf01bdbb1..68416cbeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,15 +25,18 @@ BUG FIXES: - Graceful handling/recovery for apps that have non-determinism or fail to halt - Graceful handling/recovery for violations of safety, or liveness -## 0.17.2 (TBD) +## 0.18.0 (TBD) -BUG FIXES: -- [rpc] fix subscribing using an abci.ResponseDeliverTx tag +BREAKING: +- [p2p] require all addresses come with an ID no matter what IMPROVEMENTS: - [rpc] `/tx` and `/tx_search` responses now include the transaction hash - [rpc] include validator power in `/status` +BUG FIXES: +- [rpc] fix subscribing using an abci.ResponseDeliverTx tag + ## 0.17.1 (March 27th, 2018) BUG FIXES: diff --git a/p2p/listener.go b/p2p/listener.go index 884c45ee8..e698765cd 100644 --- a/p2p/listener.go +++ b/p2p/listener.go @@ -72,7 +72,7 @@ func NewDefaultListener(protocol string, lAddr string, skipUPNP bool, logger log // Determine internal address... var intAddr *NetAddress - intAddr, err = NewNetAddressString(lAddr) + intAddr, err = NewNetAddressStringWithOptionalID(lAddr) if err != nil { panic(err) } diff --git a/p2p/netaddress.go b/p2p/netaddress.go index c7773a9f8..a77090a78 100644 --- a/p2p/netaddress.go +++ b/p2p/netaddress.go @@ -49,33 +49,45 @@ func NewNetAddress(id ID, addr net.Addr) *NetAddress { } ip := tcpAddr.IP port := uint16(tcpAddr.Port) - netAddr := NewNetAddressIPPort(ip, port) - netAddr.ID = id - return netAddr + na := NewNetAddressIPPort(ip, port) + na.ID = id + return na } -// NewNetAddressString returns a new NetAddress using the provided -// address in the form of "ID@IP:Port", where the ID is optional. +// NewNetAddressString returns a new NetAddress using the provided address in +// the form of "ID@IP:Port". // Also resolves the host if host is not an IP. func NewNetAddressString(addr string) (*NetAddress, error) { - addr = removeProtocolIfDefined(addr) + spl := strings.Split(addr, "@") + if len(spl) < 2 { + return nil, fmt.Errorf("Address (%s) does not contain ID", addr) + } + return NewNetAddressStringWithOptionalID(addr) +} + +// NewNetAddressStringWithOptionalID returns a new NetAddress using the +// provided address in the form of "ID@IP:Port", where the ID is optional. +// Also resolves the host if host is not an IP. +func NewNetAddressStringWithOptionalID(addr string) (*NetAddress, error) { + addrWithoutProtocol := removeProtocolIfDefined(addr) var id ID - spl := strings.Split(addr, "@") + spl := strings.Split(addrWithoutProtocol, "@") if len(spl) == 2 { idStr := spl[0] idBytes, err := hex.DecodeString(idStr) if err != nil { - return nil, errors.Wrap(err, fmt.Sprintf("Address (%s) contains invalid ID", addr)) + return nil, errors.Wrapf(err, "Address (%s) contains invalid ID", addrWithoutProtocol) } if len(idBytes) != IDByteLength { return nil, fmt.Errorf("Address (%s) contains ID of invalid length (%d). Should be %d hex-encoded bytes", - addr, len(idBytes), IDByteLength) + addrWithoutProtocol, len(idBytes), IDByteLength) } - id, addr = ID(idStr), spl[1] + + id, addrWithoutProtocol = ID(idStr), spl[1] } - host, portStr, err := net.SplitHostPort(addr) + host, portStr, err := net.SplitHostPort(addrWithoutProtocol) if err != nil { return nil, err } @@ -120,11 +132,10 @@ func NewNetAddressStrings(addrs []string) ([]*NetAddress, []error) { // NewNetAddressIPPort returns a new NetAddress using the provided IP // and port number. func NewNetAddressIPPort(ip net.IP, port uint16) *NetAddress { - na := &NetAddress{ + return &NetAddress{ IP: ip, Port: port, } - return na } // Equals reports whether na and other are the same addresses, diff --git a/p2p/netaddress_test.go b/p2p/netaddress_test.go index 6c1930a2f..653b436a6 100644 --- a/p2p/netaddress_test.go +++ b/p2p/netaddress_test.go @@ -9,20 +9,18 @@ import ( ) func TestNewNetAddress(t *testing.T) { - assert, require := assert.New(t), require.New(t) - tcpAddr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:8080") - require.Nil(err) + require.Nil(t, err) addr := NewNetAddress("", tcpAddr) - assert.Equal("127.0.0.1:8080", addr.String()) + assert.Equal(t, "127.0.0.1:8080", addr.String()) - assert.NotPanics(func() { + assert.NotPanics(t, func() { NewNetAddress("", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8000}) }, "Calling NewNetAddress with UDPAddr should not panic in testing") } -func TestNewNetAddressString(t *testing.T) { +func TestNewNetAddressStringWithOptionalID(t *testing.T) { testCases := []struct { addr string expected string @@ -57,6 +55,28 @@ func TestNewNetAddressString(t *testing.T) { {" @ ", "", false}, } + for _, tc := range testCases { + addr, err := NewNetAddressStringWithOptionalID(tc.addr) + if tc.correct { + if assert.Nil(t, err, tc.addr) { + assert.Equal(t, tc.expected, addr.String()) + } + } else { + assert.NotNil(t, err, tc.addr) + } + } +} + +func TestNewNetAddressString(t *testing.T) { + testCases := []struct { + addr string + expected string + correct bool + }{ + {"127.0.0.1:8080", "127.0.0.1:8080", false}, + {"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", true}, + } + for _, tc := range testCases { addr, err := NewNetAddressString(tc.addr) if tc.correct { @@ -70,23 +90,22 @@ func TestNewNetAddressString(t *testing.T) { } func TestNewNetAddressStrings(t *testing.T) { - addrs, errs := NewNetAddressStrings([]string{"127.0.0.1:8080", "127.0.0.2:8080"}) - assert.Len(t, errs, 0) + addrs, errs := NewNetAddressStrings([]string{ + "127.0.0.1:8080", + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef@127.0.0.1:8080", + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeed@127.0.0.2:8080"}) + assert.Len(t, errs, 1) assert.Equal(t, 2, len(addrs)) } func TestNewNetAddressIPPort(t *testing.T) { - assert := assert.New(t) addr := NewNetAddressIPPort(net.ParseIP("127.0.0.1"), 8080) - - assert.Equal("127.0.0.1:8080", addr.String()) + assert.Equal(t, "127.0.0.1:8080", addr.String()) } func TestNetAddressProperties(t *testing.T) { - assert, require := assert.New(t), require.New(t) - // TODO add more test cases - tests := []struct { + testCases := []struct { addr string valid bool local bool @@ -96,21 +115,19 @@ func TestNetAddressProperties(t *testing.T) { {"ya.ru:80", true, false, true}, } - for _, t := range tests { - addr, err := NewNetAddressString(t.addr) - require.Nil(err) + for _, tc := range testCases { + addr, err := NewNetAddressStringWithOptionalID(tc.addr) + require.Nil(t, err) - assert.Equal(t.valid, addr.Valid()) - assert.Equal(t.local, addr.Local()) - assert.Equal(t.routable, addr.Routable()) + assert.Equal(t, tc.valid, addr.Valid()) + assert.Equal(t, tc.local, addr.Local()) + assert.Equal(t, tc.routable, addr.Routable()) } } func TestNetAddressReachabilityTo(t *testing.T) { - assert, require := assert.New(t), require.New(t) - // TODO add more test cases - tests := []struct { + testCases := []struct { addr string other string reachability int @@ -119,13 +136,13 @@ func TestNetAddressReachabilityTo(t *testing.T) { {"ya.ru:80", "127.0.0.1:8080", 1}, } - for _, t := range tests { - addr, err := NewNetAddressString(t.addr) - require.Nil(err) + for _, tc := range testCases { + addr, err := NewNetAddressStringWithOptionalID(tc.addr) + require.Nil(t, err) - other, err := NewNetAddressString(t.other) - require.Nil(err) + other, err := NewNetAddressStringWithOptionalID(tc.other) + require.Nil(t, err) - assert.Equal(t.reachability, addr.ReachabilityTo(other)) + assert.Equal(t, tc.reachability, addr.ReachabilityTo(other)) } }