tapchannel: check for feature bits before opening chans #1041
Conversation
Roasbeef
force-pushed
the
funding-fast-fail
branch
from
8050d21 to
e2c25e4
Compare
| } | ||
|
|
||
| // If we get to this point, we weren't able to find the peer. | ||
| return false, tapchannel.ErrNoPeer |
There was a problem hiding this comment.
Seems weird to be returning an error from a package using this code... I know it's probably unavoidable with the current way the packages are depending on each other. But we should likely move all the lnd client related code into its own package.
There was a problem hiding this comment.
Yeah not a bad idea to move them.
Re returning the error from another package: I think it can be a useful pattern (assuming we use a new type for the errors) to restrict the type of errors that an implementation can return.
With the way the protocol works as is, we signal the optional feature bit, so connections can be made with older peers. Rather than timeout after they don't recognize our custom message, we'll instead use the feature bits to detect early if they don't understand the new protocol. Fixes #1038
Roasbeef
force-pushed
the
funding-fast-fail
branch
from
e2c25e4 to
cf34c65
Compare
|
|
||
| peerBytes := route.NewVertex(&peerPub) | ||
|
|
||
| peers, err := l.lnd.Client.ListPeers(ctx) |
There was a problem hiding this comment.
Would GetNodeInfo also work here? IIUC the info returned from that call should be as up-to-date and what's returned from ListPeers, for connected nodes. But AFAICT it doesn't show if we're actually connected to that node.
There was a problem hiding this comment.
So each time we connect to a peer, we'll get the fresh set of feature bits from the init message. Compared to waiting for the next node_announcement, this should be more up to date.
With the way the protocol works as is, we signal the optional feature bit, so connections can be made with older peers. Rather than timeout after they don't recognize our custom message, we'll instead use the feature bits to detect early if they don't understand the new protocol.
Fixes #1038