Cleanup and finish relay transaction work.

- Coalesce the new bytes into the max message size constant to stay
  consistent
- Correct optional relay tx field handling
- Rename the relay transactions field to DisableRelayTx so the zero value
  of false has the correct default behavior
- Add tests for new bool fast paths in read/writeElement
- Stay consistent with version order in tests
- Add a single entry to TestVersionWire to test the new functionality
  instead of adding a whole new TextVersionRelayTx function.
- Use BIP0037 in tests instead of hard coding 70001
- Nuke XXX that 70001 is different since this is handled now
- Fix and cleanup some comments
- Update test coverage report

common_test.go

@@ -52,6 +52,14 @@ func TestElementWire(t *testing.T) {
 			uint64(4294967296),
 			[]byte{0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00},
 		},
+		{
+			true,
+			[]byte{0x01},
+		},
+		{
+			false,
+			[]byte{0x00},
+		},
 		{
 			[4]byte{0x01, 0x02, 0x03, 0x04},
 			[]byte{0x01, 0x02, 0x03, 0x04},
@@ -159,6 +167,7 @@ func TestElementWireErrors(t *testing.T) {
 		{int32(1), 0, io.ErrShortWrite, io.EOF},
 		{uint32(256), 0, io.ErrShortWrite, io.EOF},
 		{int64(65536), 0, io.ErrShortWrite, io.EOF},
+		{true, 0, io.ErrShortWrite, io.EOF},
 		{[4]byte{0x01, 0x02, 0x03, 0x04}, 0, io.ErrShortWrite, io.EOF},
 		{
 			[btcwire.CommandSize]byte{

msgversion.go

@@ -50,8 +50,8 @@ type MsgVersion struct {
 	// Last block seen by the generator of the version message.
 	LastBlock int32
 
-	// Announce transactions to peer.
+	// Don't announce transactions to peer.
-	RelayTx bool
+	DisableRelayTx bool
 }
 
 // HasService returns whether the specified service is supported by the peer
@@ -132,12 +132,18 @@ func (msg *MsgVersion) BtcDecode(r io.Reader, pver uint32) error {
 		}
 	}
 
-	if pver >= BIP0037Version {
+	// There was no relay transactions field before BIP0037Version, but
-		err = readElement(r, &msg.RelayTx)
+	// the default behavior prior to the addition of the field was to always
-		if err != nil {
+	// relay transactions.
-			// Optional
+	if buf.Len() > 0 {
-			msg.RelayTx = true
+		// It's safe to ignore the error here since the buffer has at
-		}
+		// least one byte and that byte will result in a boolean value
+		// regardless of its value.  Also, the wire encoding for the
+		// field is true when transactions should be relayed, so reverse
+		// it for the DisableRelayTx field.
+		var relayTx bool
+		readElement(r, &relayTx)
+		msg.DisableRelayTx = !relayTx
 	}
 
 	return nil
@@ -183,8 +189,11 @@ func (msg *MsgVersion) BtcEncode(w io.Writer, pver uint32) error {
 		return err
 	}
 
+	// There was no relay transactions field before BIP0037Version.  Also,
+	// the wire encoding for the field is true when transactions should be
+	// relayed, so reverse it from the DisableRelayTx field.
 	if pver >= BIP0037Version {
-		err = writeElement(w, msg.RelayTx)
+		err = writeElement(w, !msg.DisableRelayTx)
 		if err != nil {
 			return err
 		}
@@ -202,12 +211,13 @@ func (msg *MsgVersion) Command() string {
 // receiver.  This is part of the Message interface implementation.
 func (msg *MsgVersion) MaxPayloadLength(pver uint32) uint32 {
 	// XXX: <= 106 different
-	// XXX: >= 70001 different
 
-	// Protocol version 4 bytes + services 8 bytes + timestamp 8 bytes + remote
+	// Protocol version 4 bytes + services 8 bytes + timestamp 8 bytes +
-	// and local net addresses + nonce 8 bytes + length of user agent (varInt) +
+	// remote and local net addresses + nonce 8 bytes + length of user
-	// max allowed useragent length + last block 4 bytes + relay tx 1 byte.
+	// agent (varInt) + max allowed useragent length + last block 4 bytes +
-	return 32 + (maxNetAddressPayload(pver) * 2) + MaxVarIntPayload + MaxUserAgentLen + 1
+	// relay transactions flag 1 byte.
+	return 33 + (maxNetAddressPayload(pver) * 2) + MaxVarIntPayload +
+		MaxUserAgentLen
 }
 
 // NewMsgVersion returns a new bitcoin version message that conforms to the
@@ -227,7 +237,7 @@ func NewMsgVersion(me *NetAddress, you *NetAddress, nonce uint64,
 		Nonce:           nonce,
 		UserAgent:       userAgent,
 		LastBlock:       lastBlock,
-		RelayTx:         true,
+		DisableRelayTx:  false,
 	}
 }
 

msgversion_test.go

@@ -64,9 +64,9 @@ func TestVersion(t *testing.T) {
 		t.Errorf("NewMsgVersion: wrong last block - got %v, want %v",
 			msg.LastBlock, lastBlock)
 	}
-	if msg.RelayTx != true {
+	if msg.DisableRelayTx != false {
-		t.Errorf("NewMsgVersion: relaytx is not true by default: - got %v, want %v",
+		t.Errorf("NewMsgVersion: disable relay tx is not false by "+
-			msg.RelayTx, true)
+			"default - got %v, want %v", msg.DisableRelayTx, false)
 	}
 
 	// Version message should not have any services set by default.
@@ -89,7 +89,8 @@ func TestVersion(t *testing.T) {
 	// Ensure max payload is expected value.
 	// Protocol version 4 bytes + services 8 bytes + timestamp 8 bytes +
 	// remote and local net addresses + nonce 8 bytes + length of user agent
-	// (varInt) + max allowed user agent length + last block 4 bytes + relay tx 1 byte.
+	// (varInt) + max allowed user agent length + last block 4 bytes +
+	// relay transactions flag 1 byte.
 	wantPayload := uint32(2102)
 	maxPayload := msg.MaxPayloadLength(pver)
 	if maxPayload != wantPayload {
@@ -150,9 +151,18 @@ func TestVersion(t *testing.T) {
 	return
 }
 
-// TestAlertWire tests the MsgAlert wire encode and decode for various protocol
+// TestVersionWire tests the MsgVersion wire encode and decode for various
-// versions.
+// protocol versions.
 func TestVersionWire(t *testing.T) {
+	// verRelayTxFalse and verRelayTxFalseEncoded is a version message as of
+	// BIP0037Version with the transaction relay disabled.
+	baseVersionBIP0037Copy := *baseVersionBIP0037
+	verRelayTxFalse := &baseVersionBIP0037Copy
+	verRelayTxFalse.DisableRelayTx = true
+	verRelayTxFalseEncoded := make([]byte, len(baseVersionBIP0037Encoded))
+	copy(verRelayTxFalseEncoded, baseVersionBIP0037Encoded)
+	verRelayTxFalseEncoded[len(verRelayTxFalseEncoded)-1] = 0
+
 	tests := []struct {
 		in   *btcwire.MsgVersion // Message to encode
 		out  *btcwire.MsgVersion // Expected decoded message
@@ -161,12 +171,30 @@ func TestVersionWire(t *testing.T) {
 	}{
 		// Latest protocol version.
 		{
-			baseVersion70001,
+			baseVersionBIP0037,
-			baseVersion70001,
+			baseVersionBIP0037,
-			baseVersion70001Encoded,
+			baseVersionBIP0037Encoded,
 			btcwire.ProtocolVersion,
 		},
 
+		// Protocol version BIP0037Version with relay transactions field
+		// true.
+		{
+			baseVersionBIP0037,
+			baseVersionBIP0037,
+			baseVersionBIP0037Encoded,
+			btcwire.BIP0037Version,
+		},
+
+		// Protocol version BIP0037Version with relay transactions field
+		// false.
+		{
+			verRelayTxFalse,
+			verRelayTxFalse,
+			verRelayTxFalseEncoded,
+			btcwire.BIP0037Version,
+		},
+
 		// Protocol version BIP0035Version.
 		{
 			baseVersion,
@@ -183,14 +211,6 @@ func TestVersionWire(t *testing.T) {
 			btcwire.BIP0031Version,
 		},
 
-		// Protocol version BIP0037Version.
-		{
-			baseVersion70001,
-			baseVersion70001,
-			baseVersion70001Encoded,
-			btcwire.BIP0037Version,
-		},
-
 		// Protocol version NetAddressTimeVersion.
 		{
 			baseVersion,
@@ -305,9 +325,13 @@ func TestVersionWireErrors(t *testing.T) {
 		// Force error in user agent.
 		{baseVersion, baseVersionEncoded, pver, 82, io.ErrShortWrite, io.ErrUnexpectedEOF},
 		// Force error in last block.
-		{baseVersion, baseVersionEncoded, pver, 97, io.ErrShortWrite, io.EOF},
+		{baseVersion, baseVersionEncoded, pver, 98, io.ErrShortWrite, io.ErrUnexpectedEOF},
-		// Force error in relay tx.
+		// Force error in relay tx - no read error should happen since
-		{baseVersion70001, baseVersion70001Encoded, uint32(70001), 101, io.ErrShortWrite, io.EOF},
+		// it's optional.
+		{
+			baseVersionBIP0037, baseVersionBIP0037Encoded,
+			btcwire.BIP0037Version, 101, io.ErrShortWrite, nil,
+		},
 		// Force error due to user agent too big
 		{exceedUAVer, exceedUAVerEncoded, pver, newLen, btcwireErr, btcwireErr},
 	}
@@ -456,77 +480,6 @@ func TestVersionOptionalFields(t *testing.T) {
 	}
 }
 
-// TestVersionRelayTx tests the MsgVersion RelayTx API
-func TestVersionRelayTx(t *testing.T) {
-	// Create version message data.
-	userAgent := "/btcdtest:0.0.1/"
-	lastBlock := int32(234234)
-	tcpAddrMe := &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8333}
-	me, err := btcwire.NewNetAddress(tcpAddrMe, btcwire.SFNodeNetwork)
-	if err != nil {
-		t.Errorf("NewNetAddress: %v", err)
-	}
-	tcpAddrYou := &net.TCPAddr{IP: net.ParseIP("192.168.0.1"), Port: 8333}
-	you, err := btcwire.NewNetAddress(tcpAddrYou, btcwire.SFNodeNetwork)
-	if err != nil {
-		t.Errorf("NewNetAddress: %v", err)
-	}
-	nonce, err := btcwire.RandomUint64()
-	if err != nil {
-		t.Errorf("RandomUint64: error generating nonce: %v", err)
-	}
-
-	// Ensure we get the correct data back out.
-	msg := btcwire.NewMsgVersion(me, you, nonce, userAgent, lastBlock)
-
-	// Explictly set RelayTx to false since true by default.
-	msg.RelayTx = false
-
-	// Encode the message to wire format.
-	var buf bytes.Buffer
-	err = msg.BtcEncode(&buf, btcwire.BIP0037Version)
-	if err != nil {
-		t.Errorf("BtcEncode error %v", err)
-	}
-	b := buf.Bytes()
-	if len(b) != 102 || b[101] != 0x00 {
-		t.Errorf("Relay Tx is not false")
-	}
-
-	wantBuf := []byte{
-		0x71, 0x11, 0x01, 0x00, // Protocol version 70001
-		0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // SFNodeNetwork
-		0x29, 0xab, 0x5f, 0x49, 0x00, 0x00, 0x00, 0x00, // 64-bit Timestamp
-		// AddrYou -- No timestamp for NetAddress in version message
-		0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // SFNodeNetwork
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0xff, 0xff, 0xc0, 0xa8, 0x00, 0x01, // IP 192.168.0.1
-		0x20, 0x8d, // Port 8333 in big-endian
-		// AddrMe -- No timestamp for NetAddress in version message
-		0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // SFNodeNetwork
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0xff, 0xff, 0x7f, 0x00, 0x00, 0x01, // IP 127.0.0.1
-		0x20, 0x8d, // Port 8333 in big-endian
-		0xf3, 0xe0, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, // Nonce
-		0x10, // Varint for user agent length
-		0x2f, 0x62, 0x74, 0x63, 0x64, 0x74, 0x65, 0x73,
-		0x74, 0x3a, 0x30, 0x2e, 0x30, 0x2e, 0x31, 0x2f, // User agent
-		0xfa, 0x92, 0x03, 0x00, // Last block
-		0x00, // Relay tx (false)
-	}
-
-	// Decode the message from wire format.
-	msg = &btcwire.MsgVersion{}
-	rbuf := bytes.NewBuffer(wantBuf)
-	err = msg.BtcDecode(rbuf, btcwire.BIP0037Version)
-	if err != nil {
-		t.Errorf("BtcDecode error %v", err)
-	}
-	if msg.RelayTx != false {
-		t.Errorf("Relay Tx is not false")
-	}
-}
-
 // baseVersion is used in the various tests as a baseline MsgVersion.
 var baseVersion = &btcwire.MsgVersion{
 	ProtocolVersion: 60002,
@@ -572,8 +525,9 @@ var baseVersionEncoded = []byte{
 	0xfa, 0x92, 0x03, 0x00, // Last block
 }
 
-// baseVersion70001 is used in the various tests as a baseline MsgVersion.
+// baseVersionBIP0037 is used in the various tests as a baseline MsgVersion for
-var baseVersion70001 = &btcwire.MsgVersion{
+// BIP0037.
+var baseVersionBIP0037 = &btcwire.MsgVersion{
 	ProtocolVersion: 70001,
 	Services:        btcwire.SFNodeNetwork,
 	Timestamp:       time.Unix(0x495fab29, 0), // 2009-01-03 12:15:05 -0600 CST)
@@ -592,12 +546,11 @@ var baseVersion70001 = &btcwire.MsgVersion{
 	Nonce:     123123, // 0x1e0f3
 	UserAgent: "/btcdtest:0.0.1/",
 	LastBlock: 234234, // 0x392fa
-	RelayTx:   true,
 }
 
-// baseVersion70001Encoded is the wire encoded bytes for baseVersion using protocol
+// baseVersionBIP0037Encoded is the wire encoded bytes for baseVersionBIP0037
-// version 70001 and is used in the various tests.
+// using protocol version BIP0037Version and is used in the various tests.
-var baseVersion70001Encoded = []byte{
+var baseVersionBIP0037Encoded = []byte{
 	0x71, 0x11, 0x01, 0x00, // Protocol version 70001
 	0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // SFNodeNetwork
 	0x29, 0xab, 0x5f, 0x49, 0x00, 0x00, 0x00, 0x00, // 64-bit Timestamp

test_coverage.txt

@@ -1,16 +1,16 @@
 
-github.com/conformal/btcwire/common.go		 readElement				 100.00% (61/61)
+github.com/conformal/btcwire/common.go		 readElement				 100.00% (69/69)
-github.com/conformal/btcwire/common.go		 writeElement				 100.00% (61/61)
+github.com/conformal/btcwire/common.go		 writeElement				 100.00% (69/69)
 github.com/conformal/btcwire/message.go		 ReadMessageN				 100.00% (42/42)
 github.com/conformal/btcwire/message.go		 WriteMessageN				 100.00% (38/38)
 github.com/conformal/btcwire/msgtx.go		 MsgTx.BtcDecode			 100.00% (36/36)
-github.com/conformal/btcwire/msgversion.go	 MsgVersion.BtcDecode			 100.00% (32/32)
+github.com/conformal/btcwire/msgversion.go	 MsgVersion.BtcDecode			 100.00% (36/36)
 github.com/conformal/btcwire/msgtx.go		 MsgTx.BtcEncode			 100.00% (26/26)
+github.com/conformal/btcwire/msgversion.go	 MsgVersion.BtcEncode			 100.00% (26/26)
 github.com/conformal/btcwire/msgtx.go		 MsgTx.Copy				 100.00% (24/24)
 github.com/conformal/btcwire/msgtx.go		 readTxIn				 100.00% (22/22)
-github.com/conformal/btcwire/msgversion.go	 MsgVersion.BtcEncode			 100.00% (22/22)
-github.com/conformal/btcwire/msgblock.go	 MsgBlock.DeserializeTxLoc		 100.00% (21/21)
 github.com/conformal/btcwire/common.go		 readVarInt				 100.00% (21/21)
+github.com/conformal/btcwire/msgblock.go	 MsgBlock.DeserializeTxLoc		 100.00% (21/21)
 github.com/conformal/btcwire/netaddress.go	 readNetAddress				 100.00% (20/20)
 github.com/conformal/btcwire/msggetheaders.go	 MsgGetHeaders.BtcDecode		 100.00% (20/20)
 github.com/conformal/btcwire/msggetblocks.go	 MsgGetBlocks.BtcDecode			 100.00% (20/20)
@@ -165,5 +165,5 @@ github.com/conformal/btcwire/msggetdata.go	 NewMsgGetData				 100.00% (1/1)
 github.com/conformal/btcwire/msgmempool.go	 MsgMemPool.Command			 100.00% (1/1)
 github.com/conformal/btcwire/msgmempool.go	 MsgMemPool.MaxPayloadLength		 100.00% (1/1)
 github.com/conformal/btcwire/msgmempool.go	 NewMsgMemPool				 100.00% (1/1)
-github.com/conformal/btcwire			 ---------------------------------	 100.00% (1160/1160)
+github.com/conformal/btcwire			 ---------------------------------	 100.00% (1184/1184)