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 backup #8490

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
46 changes: 46 additions & 0 deletions peer/brontide.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,10 @@ type Brontide struct {

// log is a peer-specific logging instance.
log btclog.Logger

// backupData is an in-memory store for data that we store for our
// peers.
backupData lnwire.PeerStorageBlob
}

// A compile-time check to ensure that Brontide satisfies the lnpeer.Peer interface.
Expand Down Expand Up @@ -1824,6 +1828,12 @@ out:
discStream.AddMsg(msg)

case *lnwire.PeerStorage:
err = p.handlePeerStorageMessage(msg)
if err != nil {
p.storeError(err)
p.log.Errorf("%v", err)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we should also have a case here for the storage retrieval message. Since we do know about it but dont allow it. Then we are at least storing a more useful error (since currently it will store the "unknown message" error

case *lnwire.PeerStorageRetrieval:
err = p.handlePeerStorageRetrieval()
if err != nil {
Expand Down Expand Up @@ -4169,6 +4179,42 @@ func (p *Brontide) handleRemovePendingChannel(req *newChannelMsg) {
p.addedChannels.Delete(chanID)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I think a good pattern is to first implement DB things (ie, PeerDataStore) and then to add things that call that interface. Because as is, this commit will actually panic if we receive this message since PeerDataStore is still nil at this point.

Copy link
Collaborator

@ProofOfKeags ProofOfKeags Mar 13, 2024

Choose a reason for hiding this comment

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

In general, I think a good pattern is to first implement DB things (ie, PeerDataStore) and then to add things that call that interface.

Please no. Implementation should always be last. Interface design first. Use the interface operations in the surrounding code next to validate the ergonomics and structure of the interface design. Finally, implement the interface last.

If you go the other way around it causes you to compromise the interface design in favor of the implementation rather than the call-sites. This will cause the quality of the library code in the codebase to atrophy. Intermediate commits not being fully valid implementations is completely fine. If you absolutely do not want to tolerate invalid intermediate commits, you can squash those commits later.

Work backwards from the goal, not forwards from the implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i do understand & agree with the principle but then I think it is important to make sure that it is not possible to call the interface at runtime yet. Cause that will cause nil panics.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the issue with that in an intermediate commit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess my personal rule is "let's make sure we can revert back any commit without worrying that the commit we are reverting back to actually depends on a future commit to compile/run correctly


// handlePeerStorageMessage handles `PeerStorage` message, it stores the message
// and sends it back to the peer as an ack.
func (p *Brontide) handlePeerStorageMessage(msg *lnwire.PeerStorage) error {
ellemouton marked this conversation as resolved.
Show resolved Hide resolved
// Check if we have the feature to store peer backup enabled.
if !p.LocalFeatures().HasFeature(lnwire.ProvideStorageOptional) {
warning := "received peer storage message but not " +
"advertising required feature bit"
Comment on lines +4288 to +4289
Copy link
Collaborator

Choose a reason for hiding this comment

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

This leaks data to the peer. We should not give indication that we understand the feature here.


if err := p.SendMessage(false, &lnwire.Warning{
ChanID: lnwire.ConnectionWideID,
Data: []byte(warning),
}); err != nil {
return err
}
ellemouton marked this conversation as resolved.
Show resolved Hide resolved

p.Disconnect(errors.New(warning))

return nil
}

// If we have no active channels with this peer, we return quickly.
if !p.hasActiveChannels() {
p.log.Tracef("Received peerStorage message from "+
"peer(%v) with no active channels -- ignoring",
p.String())

return nil
}

p.log.Debugf("handling peerbackup for peer(%s)", p)

p.backupData = msg.Blob

return nil
}

// sendLinkUpdateMsg sends a message that updates the channel to the
// channel's message stream.
func (p *Brontide) sendLinkUpdateMsg(cid lnwire.ChannelID, msg lnwire.Message) {
Expand Down
127 changes: 127 additions & 0 deletions peer/brontide_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/lightningnetwork/lnd/fn"
"github.com/lightningnetwork/lnd/htlcswitch"
"github.com/lightningnetwork/lnd/lntest/wait"
"github.com/lightningnetwork/lnd/lnutils"
"github.com/lightningnetwork/lnd/lnwallet"
"github.com/lightningnetwork/lnd/lnwallet/chancloser"
"github.com/lightningnetwork/lnd/lnwire"
Expand Down Expand Up @@ -1442,3 +1443,129 @@ func TestHandlePeerStorageRetrieval(t *testing.T) {
// Test that we disconnect.
require.True(t, peer.IsDisconnected())
}

// TestHandlePeerStorage tests handling peer storage message.
func TestHandlePeerStorage(t *testing.T) {
t.Parallel()
rt := require.New(t)

// Create test data.
blob := []byte{0x9c, 0x40, 0x1, 0x2, 0x3}

testCases := []struct {
name string
msgTestFunc func(msg lnwire.Message)
setUpFunc func(peer *Brontide)
noSendMsg bool
disconnect bool
backupData lnwire.PeerStorageBlob
}{
{
name: "option peer storage disabled",
msgTestFunc: func(msg lnwire.Message) {
// In this case, we expect to send a warning to
// this peer.
rt.IsType(msg, &lnwire.Warning{})
},
disconnect: true,
},
{
name: "option peer storage enabled, active channels " +
"present",
setUpFunc: func(peer *Brontide) {
// Enable option_peer_storage.
peer.cfg.Features = lnwire.NewFeatureVector(
lnwire.NewRawFeatureVector(
lnwire.ProvideStorageOptional,
),
lnwire.Features,
)

// Add a dummy active channel.
chanID := lnwire.NewChanIDFromOutPoint(
wire.OutPoint{Index: 1},
)
peer.activeChannels.Store(
chanID, &lnwallet.LightningChannel{},
)
},
noSendMsg: true,
backupData: blob,
},
{
name: "option peer storage enabled, active channels " +
"absent",
setUpFunc: func(peer *Brontide) {
// Enable option_peer_storage.
peer.cfg.Features = lnwire.NewFeatureVector(
lnwire.NewRawFeatureVector(
lnwire.ProvideStorageOptional,
),
lnwire.Features,
)

peer.activeChannels = &lnutils.SyncMap[
lnwire.ChannelID,
*lnwallet.LightningChannel,
]{}
},
noSendMsg: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
params := createTestPeer(t)
peer := params.peer

// The backupData should be nil on initialization.
rt.Nil(peer.backupData)

// Send a signal to this channel to indicate readiness
// for potential peer disconnection.
close(peer.startReady)

if tc.setUpFunc != nil {
tc.setUpFunc(peer)
}

// Buffer outgoingQueue to prevent blocking.
peer.outgoingQueue = make(chan outgoingMsg, 1)

peerStorageMsg, err := lnwire.NewPeerStorageMsg(
blob,
)
rt.NoError(err)

// Call the method.
err = peer.handlePeerStorageMessage(peerStorageMsg)
rt.NoError(err)

// Test for the backup data stored in memory.
rt.Equal(tc.backupData, peer.backupData)

// Test for expected outgoing messages, if any.
select {
case receivedMsg := <-peer.outgoingQueue:
if !tc.noSendMsg {
tc.msgTestFunc(receivedMsg.msg)

return
}

t.Fatalf("received unexpected "+
"message %v",
receivedMsg.msg.MsgType())

case <-time.After(2 * time.Second):
if !tc.noSendMsg {
t.Fatalf("did not receive message " +
"as expected.")
}
}

// Check if the peer should be disconnected.
rt.Equal(tc.disconnect, peer.IsDisconnected())
})
}
}