Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swap: exchange chain id on handshake #1913

Merged
merged 9 commits into from
Nov 12, 2019
8 changes: 8 additions & 0 deletions swap/protocol.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ var (
// ErrEmptyAddressInSignature is used when the empty address is used for the chequebook in the handshake
ErrEmptyAddressInSignature = errors.New("empty address in handshake")

// ErrDifferentChainId is used when the chain id exchanged during the handshake does not match
ErrDifferentChainId = errors.New("different chain id")
Copy link
Contributor

Choose a reason for hiding this comment

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

var ErrDifferentChainId should be ErrDifferentChainID -- go-lint

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that in the linter output when developing. Looks like I forgot to actually commit that. Interesting that the CI didn't catch this. Should be fixed now.


// ErrInvalidHandshakeMsg is used when the message received during handshake does not conform to the
// structure of the HandshakeMsg
ErrInvalidHandshakeMsg = errors.New("invalid handshake message")
Expand Down Expand Up @@ -98,6 +101,10 @@ func (s *Swap) verifyHandshake(msg interface{}) error {
return ErrEmptyAddressInSignature
}

if handshake.ChainID != s.chainID {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you update the comment to the function verifyHandshake? Right now it says that it is only verifying the chequebook address transmitted

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return ErrDifferentChainId
}

return s.chequebookFactory.VerifyContract(handshake.ContractAddress)
}

Expand All @@ -107,6 +114,7 @@ func (s *Swap) run(p *p2p.Peer, rw p2p.MsgReadWriter) error {

handshake, err := protoPeer.Handshake(context.Background(), &HandshakeMsg{
ContractAddress: s.GetParams().ContractAddress,
ChainID: s.chainID,
}, s.verifyHandshake)
if err != nil {
return err
Expand Down
197 changes: 152 additions & 45 deletions swap/protocol_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package swap

import (
"context"
"errors"
"fmt"
"io/ioutil"
"os"
Expand All @@ -44,68 +45,174 @@ func init() {
log.Root().SetHandler(log.LvlFilterHandler(log.Lvl(*loglevel), log.StreamHandler(colorable.NewColorableStderr(), log.TerminalFormat(true))))
}

/*
TestHandshake creates two mock nodes and initiates an exchange;
it expects a handshake to take place between the two nodes
(the handshake would fail because we don't actually use real nodes here)
*/
func TestHandshake(t *testing.T) {
var err error
// protocol tester based on a swap instance
type swapTester struct {
*p2ptest.ProtocolTester
swap *Swap
}

// setup test swap object
// creates a new protocol tester for swap with a deployed chequebook
func newSwapTester(t *testing.T) (*swapTester, func(), error) {
swap, clean := newTestSwap(t, ownerKey, nil)
defer clean()

ctx := context.Background()
err = testDeploy(ctx, swap)
err := testDeploy(context.Background(), swap)
if err != nil {
t.Fatal(err)
return nil, nil, err
}

// setup the protocolTester, which will allow protocol testing by sending messages
protocolTester := p2ptest.NewProtocolTester(swap.owner.privateKey, 2, swap.run)
protocolTester := p2ptest.NewProtocolTester(swap.owner.privateKey, 1, swap.run)
return &swapTester{
ProtocolTester: protocolTester,
swap: swap,
}, clean, nil
}

// creates a test exchange for the handshakes
func HandshakeMsgExchange(lhs, rhs *HandshakeMsg, id enode.ID) []p2ptest.Exchange {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this function exported?

return []p2ptest.Exchange{
{
Expects: []p2ptest.Expect{
{
Code: 0,
Msg: lhs,
Peer: id,
},
},
},
{
Triggers: []p2ptest.Trigger{
{
Code: 0,
Msg: rhs,
Peer: id,
},
},
},
}
}

// shortcut to creditor node
debitor := protocolTester.Nodes[0]
creditor := protocolTester.Nodes[1]
// helper function for testing the handshake
// lhs is the HandshakeMsg we expect to be sent, rhs the one we receive
// disconnects is a list of disconnect events to be expected
func (s *swapTester) testHandshake(lhs, rhs *HandshakeMsg, disconnects ...*p2ptest.Disconnect) error {
if err := s.TestExchanges(HandshakeMsgExchange(lhs, rhs, s.Nodes[0].ID())...); err != nil {
return err
}

// set balance artifially
swap.saveBalance(creditor.ID(), -42)
if len(disconnects) > 0 {
return s.TestDisconnected(disconnects...)
}

// create the expected cheque to be received
cheque := newTestCheque()
// If we don't expect disconnect, ensure peers remain connected
err := s.TestDisconnected(&p2ptest.Disconnect{
Peer: s.Nodes[0].ID(),
Error: nil,
})

// sign the cheque
cheque.Signature, err = cheque.Sign(swap.owner.privateKey)
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Isn't a TestDisconnected returning an err if it does not find the expected disconnect? So wouldn't here err be nil if it actually found the provided Disconnect, which is saying stay connected, which is what you wanted to test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, on a disconnect err would be nil. Which is an error because because this is where we we want to make sure that there is no disconnect. (This function is copied from the network tests btw).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit counter-intuitive to pass a Disconnect object when not expecting a disconnect...Can't this just be called with an empty object, which would signal that we don't expect a disconnect?

Then err would return nil only if there would be no disconnect.

If that's not possible, I guess that's the way to do it and we keep it (I understand you copied it from elsewhere)

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaik there is no other way to check if the node is disconnected or not. (It also would not be possible, if we passed nil, then the function would not know which peer to check).

I would keep the function as it is, so that we remain consistent with the handshake test style from the other protocols.

return fmt.Errorf("Unexpected peer disconnect")
}

if err.Error() != "timed out waiting for peers to disconnect" {
return err
}

return nil
}

// creates a new HandshakeMsg
func newSwapHandshakeMsg(contractAddress common.Address, chainID uint64) *HandshakeMsg {
return &HandshakeMsg{
ContractAddress: contractAddress,
ChainID: chainID,
}
}

// creates the correct HandshakeMsg based on Swap instance
func correctSwapHandshakeMsg(swap *Swap) *HandshakeMsg {
return newSwapHandshakeMsg(swap.GetParams().ContractAddress, swap.chainID)
}

// TestHandshake tests the correct handshake scenario
func TestHandshake(t *testing.T) {
// setup the protocolTester, which will allow protocol testing by sending messages
protocolTester, clean, err := newSwapTester(t)
defer clean()
if err != nil {
t.Fatal(err)
}

// run the exchange:
// trigger a `EmitChequeMsg`
// expect HandshakeMsg on each node
err = protocolTester.TestExchanges(p2ptest.Exchange{
Label: "TestHandshake",
Triggers: []p2ptest.Trigger{
{
Code: 0,
Msg: &HandshakeMsg{
ContractAddress: swap.GetParams().ContractAddress,
},
Peer: creditor.ID(),
},
err = protocolTester.testHandshake(
correctSwapHandshakeMsg(protocolTester.swap),
correctSwapHandshakeMsg(protocolTester.swap),
)
if err != nil {
t.Fatal(err)
}
}

// TestHandshakeInvalidChainID tests that a handshake with the wrong chain id is rejected
func TestHandshakeInvalidChainID(t *testing.T) {
// setup the protocolTester, which will allow protocol testing by sending messages
protocolTester, clean, err := newSwapTester(t)
defer clean()
if err != nil {
t.Fatal(err)
}

err = protocolTester.testHandshake(
correctSwapHandshakeMsg(protocolTester.swap),
newSwapHandshakeMsg(protocolTester.swap.GetParams().ContractAddress, 1234),
&p2ptest.Disconnect{
Peer: protocolTester.Nodes[0].ID(),
Error: errors.New("Handshake error: Message handler error: (msg code 0): different chain id"),
},
Expects: []p2ptest.Expect{
{
Code: 0,
Msg: &HandshakeMsg{
ContractAddress: swap.GetParams().ContractAddress,
},
Peer: debitor.ID(),
},
)
if err != nil {
t.Fatal(err)
}
}

// TestHandshakeEmptyContract tests that a handshake with an empty contract address is rejected
func TestHandshakeEmptyContract(t *testing.T) {
// setup the protocolTester, which will allow protocol testing by sending messages
protocolTester, clean, err := newSwapTester(t)
defer clean()
if err != nil {
t.Fatal(err)
}

err = protocolTester.testHandshake(
correctSwapHandshakeMsg(protocolTester.swap),
newSwapHandshakeMsg(common.Address{}, 1234),
&p2ptest.Disconnect{
Peer: protocolTester.Nodes[0].ID(),
Error: errors.New("Handshake error: Message handler error: (msg code 0): empty address in handshake"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you somehow create this error by including the (exported) error messages from the different protocols? Such that when if the content of the error message is changed in one protocol, it does not make our test fail?

For example, the expected error message here is a concatenation of:
protocols.ErrHandshake + protocols.ErrHandler + swap.ErrDifferentChainID.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

},
})
)
if err != nil {
t.Fatal(err)
}
}

// there should be no error at this point
// TestHandshakeInvalidContract tests that a handshake with an address that's not a valid chequebook
func TestHandshakeInvalidContract(t *testing.T) {
// setup the protocolTester, which will allow protocol testing by sending messages
protocolTester, clean, err := newSwapTester(t)
defer clean()
if err != nil {
t.Fatal(err)
}

err = protocolTester.testHandshake(
correctSwapHandshakeMsg(protocolTester.swap),
newSwapHandshakeMsg(ownerAddress, protocolTester.swap.chainID),
&p2ptest.Disconnect{
Peer: protocolTester.Nodes[0].ID(),
Error: errors.New("Handshake error: Message handler error: (msg code 0): not deployed by factory"),
},
)
if err != nil {
t.Fatal(err)
}
Expand Down
2 changes: 1 addition & 1 deletion swap/simulations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ func newSharedBackendSwaps(t *testing.T, nodeCount int) (*swapSimulationParams,
if err != nil {
t.Fatal(err)
}
params.swaps[i] = newSwapInstance(stores[i], owner, testBackend, defParams, factory)
params.swaps[i] = newSwapInstance(stores[i], owner, testBackend, 10, defParams, factory)
}

params.backend = testBackend
Expand Down
5 changes: 4 additions & 1 deletion swap/swap.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type Swap struct {
contract contract.Contract // reference to the smart contract
chequebookFactory contract.SimpleSwapFactory // the chequebook factory used
honeyPriceOracle HoneyOracle // oracle which resolves the price of honey (in Wei)
chainID uint64 // id of the chain the backend is connected to
}

// Owner encapsulates information related to accessing the contract
Expand Down Expand Up @@ -130,7 +131,7 @@ func swapRotatingFileHandler(logdir string) (log.Handler, error) {
}

// newSwapInstance is a swap constructor function without integrity checks
func newSwapInstance(stateStore state.Store, owner *Owner, backend contract.Backend, params *Params, chequebookFactory contract.SimpleSwapFactory) *Swap {
func newSwapInstance(stateStore state.Store, owner *Owner, backend contract.Backend, chainID uint64, params *Params, chequebookFactory contract.SimpleSwapFactory) *Swap {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to have the order of function arguments the same as the order of the functions in the swap struct

Copy link
Member Author

Choose a reason for hiding this comment

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

This already wasn't the case prior to the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

That being said I want the backend and chain id next to each other since they kind of belong together. So it should be changed in the struct.

return &Swap{
store: stateStore,
peers: make(map[enode.ID]*Peer),
Expand All @@ -139,6 +140,7 @@ func newSwapInstance(stateStore state.Store, owner *Owner, backend contract.Back
params: params,
chequebookFactory: chequebookFactory,
honeyPriceOracle: NewHoneyPriceOracle(),
chainID: chainID,
}
}

Expand Down Expand Up @@ -193,6 +195,7 @@ func New(dbPath string, prvkey *ecdsa.PrivateKey, backendURL string, params *Par
stateStore,
owner,
backend,
chainID.Uint64(),
params,
factory,
)
Expand Down
2 changes: 1 addition & 1 deletion swap/swap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1371,7 +1371,7 @@ func newBaseTestSwapWithParams(t *testing.T, key *ecdsa.PrivateKey, params *Para
if err != nil {
t.Fatal(err)
}
swap := newSwapInstance(stateStore, owner, backend, params, factory)
swap := newSwapInstance(stateStore, owner, backend, 10, params, factory)
return swap, dir
}

Expand Down
1 change: 1 addition & 0 deletions swap/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type Cheque struct {

// HandshakeMsg is exchanged on peer handshake
type HandshakeMsg struct {
ChainID uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave a comment here, specifying that this is the chainID of the Ethereum network

Copy link
Member Author

Choose a reason for hiding this comment

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

done

ContractAddress common.Address
}

Expand Down