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

peer: more interfaces, refactor unit tests #5067

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion htlcswitch/link.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@ type ChannelLinkConfig struct {
// TODO(conner): remove after refactoring htlcswitch testing framework.
Switch *Switch

// BestHeight returns the best known height.
BestHeight func() uint32

// ForwardPackets attempts to forward the batch of htlcs through the
// switch. The function returns and error in case it fails to send one or
// more packets. The link's quit signal should be provided to allow
Expand Down Expand Up @@ -2674,7 +2677,7 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg,
continue
}

heightNow := l.cfg.Switch.BestHeight()
heightNow := l.cfg.BestHeight()

pld, err := chanIterator.HopPayload()
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions htlcswitch/link_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1951,6 +1951,7 @@ func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) (
FwrdingPolicy: globalPolicy,
Peer: alicePeer,
Switch: aliceSwitch,
BestHeight: aliceSwitch.BestHeight,
Circuits: aliceSwitch.CircuitModifier(),
ForwardPackets: aliceSwitch.ForwardPackets,
DecodeHopIterators: decoder.DecodeHopIterators,
Expand Down Expand Up @@ -4454,6 +4455,7 @@ func (h *persistentLinkHarness) restartLink(
FwrdingPolicy: globalPolicy,
Peer: alicePeer,
Switch: aliceSwitch,
BestHeight: aliceSwitch.BestHeight,
Circuits: aliceSwitch.CircuitModifier(),
ForwardPackets: aliceSwitch.ForwardPackets,
DecodeHopIterators: decoder.DecodeHopIterators,
Expand Down
1 change: 1 addition & 0 deletions htlcswitch/test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,7 @@ func (h *hopNetwork) createChannelLink(server, peer *mockServer,
link := NewChannelLink(
ChannelLinkConfig{
Switch: server.htlcSwitch,
BestHeight: server.htlcSwitch.BestHeight,
FwrdingPolicy: h.globalPolicy,
Peer: peer,
Circuits: server.htlcSwitch.CircuitModifier(),
Expand Down
78 changes: 58 additions & 20 deletions peer/brontide.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,56 @@ type TimestampedError struct {
Timestamp time.Time
}

// ChannelSwitch is an implementation of the MessageSwitch interface that wraps
// htlcswitch.Switch.
type ChannelSwitch struct {
*htlcswitch.Switch
}

// NewChannelSwitch initializes a ChannelSwitch given a raw *htlcswitch.Switch
// pointer.
func NewChannelSwitch(innerSwitch *htlcswitch.Switch) *ChannelSwitch {
return &ChannelSwitch{innerSwitch}
}

// BestHeight returns innerSwitch's BestHeight.
func (s *ChannelSwitch) BestHeight() uint32 {
return s.Switch.BestHeight()
}

// CircuitModifier returns the innerSwitch's CircuitModifier.
func (s *ChannelSwitch) CircuitModifier() htlcswitch.CircuitModifier {
return s.Switch.CircuitModifier()
}

// GetLink retrieves a MessageLink given a ChannelID from the inner Switch.
func (s *ChannelSwitch) GetLink(cid lnwire.ChannelID) (MessageLink, error) {
return s.Switch.GetLink(cid)
}

// InitLink initializes a ChannelLink in the Switch.
Copy link
Member

Choose a reason for hiding this comment

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

MessageLink or ChannelLink?

Copy link
Collaborator Author

@Crypt-iQ Crypt-iQ Jul 8, 2021

Choose a reason for hiding this comment

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

This is using the underlying htlcswitch method AddLink and NewChannelLink, so I'd say a ChannelLink

func (s *ChannelSwitch) InitLink(linkCfg htlcswitch.ChannelLinkConfig,
lnChan *lnwallet.LightningChannel) error {

link := htlcswitch.NewChannelLink(linkCfg, lnChan)

// Before adding our new link, purge the Switch of any pending or live
// links going by the same channel id. If one is found, we'll shut it
// down to ensure that the mailboxes are only ever under the control of
// one link.
s.RemoveLink(link.ChanID())

// With the ChannelLink created, we'll now notify the Switch so this
// channel can be used to dispatch local payments and also passively
// forward payments.
return s.AddLink(link)
}

// RemoveLink removes a ChannelLink from the Switch.
func (s *ChannelSwitch) RemoveLink(cid lnwire.ChannelID) {
s.Switch.RemoveLink(cid)
}

// Config defines configuration fields that are necessary for a peer object
// to function.
type Config struct {
Expand Down Expand Up @@ -175,9 +225,9 @@ type Config struct {
// ReadPool is the task pool that manages reuse of read buffers.
ReadPool *pool.Read

// Switch is a pointer to the htlcswitch. It is used to setup, get, and
// tear-down ChannelLinks.
Switch *htlcswitch.Switch
// Switch is an implementation of MessageSwitch. It is used to setup,
// get, and tear-down MessageLinks.
Switch MessageSwitch
Crypt-iQ marked this conversation as resolved.
Show resolved Hide resolved

// InterceptSwitch is a pointer to the InterceptableSwitch, a wrapper around
// the regular Switch. We only export it here to pass ForwardPackets to the
Expand Down Expand Up @@ -813,7 +863,7 @@ func (p *Brontide) addLink(chanPoint *wire.OutPoint,
FetchLastChannelUpdate: p.cfg.FetchLastChanUpdate,
HodlMask: p.cfg.Hodl.Mask(),
Registry: p.cfg.Invoices,
Switch: p.cfg.Switch,
BestHeight: p.cfg.Switch.BestHeight,
Crypt-iQ marked this conversation as resolved.
Show resolved Hide resolved
Circuits: p.cfg.Switch.CircuitModifier(),
ForwardPackets: p.cfg.InterceptSwitch.ForwardPackets,
FwrdingPolicy: *forwardingPolicy,
Expand Down Expand Up @@ -841,18 +891,7 @@ func (p *Brontide) addLink(chanPoint *wire.OutPoint,
HtlcNotifier: p.cfg.HtlcNotifier,
}

link := htlcswitch.NewChannelLink(linkCfg, lnChan)

// Before adding our new link, purge the switch of any pending or live
// links going by the same channel id. If one is found, we'll shut it
// down to ensure that the mailboxes are only ever under the control of
// one link.
p.cfg.Switch.RemoveLink(link.ChanID())

// With the channel link created, we'll now notify the htlc switch so
// this channel can be used to dispatch local payments and also
// passively forward payments.
return p.cfg.Switch.AddLink(link)
return p.cfg.Switch.InitLink(linkCfg, lnChan)
}

// maybeSendNodeAnn sends our node announcement to the remote peer if at least
Expand Down Expand Up @@ -1139,10 +1178,9 @@ func (ms *msgStream) AddMsg(msg lnwire.Message) {
}

// waitUntilLinkActive waits until the target link is active and returns a
// ChannelLink to pass messages to. It accomplishes this by subscribing to
// MessageLink to pass messages to. It accomplishes this by subscribing to
// an ActiveLinkEvent which is emitted by the link when it first starts up.
func waitUntilLinkActive(p *Brontide,
cid lnwire.ChannelID) htlcswitch.ChannelLink {
func waitUntilLinkActive(p *Brontide, cid lnwire.ChannelID) MessageLink {

// Subscribe to receive channel events.
//
Expand Down Expand Up @@ -1211,7 +1249,7 @@ func waitUntilLinkActive(p *Brontide,
// lookups.
func newChanMsgStream(p *Brontide, cid lnwire.ChannelID) *msgStream {

var chanLink htlcswitch.ChannelLink
var chanLink MessageLink

apply := func(msg lnwire.Message) {
// This check is fine because if the link no longer exists, it will
Expand Down
34 changes: 34 additions & 0 deletions peer/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"net"
"time"

"github.com/lightningnetwork/lnd/htlcswitch"
"github.com/lightningnetwork/lnd/lnwallet"
"github.com/lightningnetwork/lnd/lnwire"
)

Expand Down Expand Up @@ -54,3 +56,35 @@ type MessageConn interface {
// ReadNextBody reads the next body.
ReadNextBody([]byte) ([]byte, error)
}

// MessageLink is an interface that contains some functionality from a
// htlcswitch.ChannelLink.
type MessageLink interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be more idiomatic Go, maybe we could name this interface ChannelUpdateHandler. Since it's handling channel updates. wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Prefer to keep it as MessageLink

// ChanID returns the ChannelID of the MessageLink.
ChanID() lnwire.ChannelID

// HandleChannelUpdate passes lnwire.Message to the MessageLink.
HandleChannelUpdate(lnwire.Message)
}

// MessageSwitch is an interface that manages setup, retrieval, and shutdown of
// MessageLink implementations.
type MessageSwitch interface {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like htlcswitch.Switch already satisfied most of the functions here. I wonder about the necessity to create the channelSwitch here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The motivation is to make peer not rely on htlcswitch.Switch, so instead use a ChannelSwitch that can be replaced with a mock. Otherwise you need to provide a *htlcswitch.Switch when testing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

channelSwitch needs to wrap GetLink to return MessageLink as *htlcswitch.Switch returns ChannelLink

Copy link
Member

@yyforyongyu yyforyongyu Jul 13, 2021

Choose a reason for hiding this comment

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

I don't think you could get over *htlcswitch.Switch since channelSwitch needs it. This design pattern is weird. You have a concrete type defined in another package, and you have an interface defined in this package, then you create a concrete struct that implements the interface, which in the end is the concrete type living in another package.

Copy link
Collaborator Author

@Crypt-iQ Crypt-iQ Jul 13, 2021

Choose a reason for hiding this comment

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

If we want to be able to replace *htlcswitch.Switch in peer tests (which IMO is worth it, as it does not belong in the peer unit tests):

  • Need an interface to pass into the peer config instead of the raw Switch pointer.
  • The interface should also live in the peer package.
    • So, the Switch now has to comply with this peer-pkg interface.
  • Peer calls the interface's GetLink which returns ChannelLink, error
    • To implement this interface in unit tests, and have a non-nil ChannelLink** returned on GetLink, we must implement this interface in the peer.
    • ChannelLink interface has this method HandleSwitchPacket(*htlcPacket) error which uses a private *htlcPacket argument.
  • So there are two options:
    • Change the interface GetLink to not return ChannelLink, error
    • Change ChannelLink methods to not have *htlcPacket

So IMO this is a workaround until we can cleanup the htlcswitch package. One thing would be to remove *htlcPacket in public methods. And another would be to have GetLink return a concrete type so that it can fit into any interface because we don't have interfaces narrowing ChannelLink -> MessageLink. This last change would allow us to remove the channelSwitch type.

** I have a test-case that requires a non-nil link to be returned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a blocker for your ACK? I can figure out the best step to avoid this "type confusion"

Copy link
Member

Choose a reason for hiding this comment

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

If we want to be able to replace *htlcswitch.Switch in peer tests (which IMO is worth it, as it does not belong in the peer unit tests):

I agree we should do this. And what confused me here is that, the change made in current commits do not acheive this goal? You have a MessageSwitch interface defined, which is good. But the concrete implementation still requires a *htlcswitch.Switch. The test below uses NewChannelSwitch, which takes a *htlcswitch.Switch.

The interface should also live in the peer package.
So, the Switch now has to comply with this peer-pkg interface.

This is probably my main blocker because, imo, packages should be independent in the sense of actual implementations of interfaces. peer should not care about the actual implementation of an interface defined by switch, and vice versa. peer should use the interfaces like its an API, only cares about what arguments to use and what values are returned. This way we could provide a proper abstraction for each package.

Peer calls the interface's GetLink which returns ChannelLink, error
To implement this interface in unit tests, and have a non-nil ChannelLink** returned on GetLink, we must implement this interface in the peer.
ChannelLink interface has this method HandleSwitchPacket(*htlcPacket) error which uses a private *htlcPacket argument.

You could also make *htlcPacket an interface so we don't need the concrete type? Just like you've mentioned,

One thing would be to remove *htlcPacket in public methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tests do not use NewChannelSwitch, they use the mockMessageSwitch? The peer interface will always be a subset of functionality provided by the Switch and only cares about the functions used in the peer package so I think this is achieved? I think why I didn't change *htlcPacket was because it was a pretty large change that touched the unit tests of the htlcswitch package

// BestHeight returns the best height known to the MessageSwitch.
BestHeight() uint32

// CircuitModifier returns a reference to a CircuitModifier.
CircuitModifier() htlcswitch.CircuitModifier

// GetLink retrieves a MessageLink given a ChannelID.
GetLink(lnwire.ChannelID) (MessageLink, error)

// InitLink creates a link given a ChannelLinkConfig and
// LightningChannel.
InitLink(htlcswitch.ChannelLinkConfig,
*lnwallet.LightningChannel) error

// RemoveLink removes a MessageLink from the MessageSwitch given a
// ChannelID.
RemoveLink(lnwire.ChannelID)
}
2 changes: 1 addition & 1 deletion peer/test_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func createTestPeer(notifier chainntnfs.ChainNotifier,
PubKeyBytes: pubKey,
ErrorBuffer: errBuffer,
ChainIO: chainIO,
Switch: htlcSwitch,
Switch: NewChannelSwitch(htlcSwitch),

ChanActiveTimeout: chanActiveTimeout,
InterceptSwitch: htlcswitch.NewInterceptableSwitch(htlcSwitch),
Expand Down
2 changes: 1 addition & 1 deletion server.go
Original file line number Diff line number Diff line change
Expand Up @@ -3237,7 +3237,7 @@ func (s *server) peerConnected(conn net.Conn, connReq *connmgr.ConnReq,
ErrorBuffer: errBuffer,
WritePool: s.writePool,
ReadPool: s.readPool,
Switch: s.htlcSwitch,
Switch: peer.NewChannelSwitch(s.htlcSwitch),
InterceptSwitch: s.interceptableSwitch,
ChannelDB: s.remoteChanDB,
ChannelGraph: s.localChanDB.ChannelGraph(),
Expand Down