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

Support the max_htlc field from ChannelUpdates #2460

Merged
merged 23 commits into from Jan 23, 2019

Conversation

Projects
None yet
3 participants
@halseth
Copy link
Collaborator

halseth commented Jan 11, 2019

Continuation of #2296.

Replaces #2296 and #2435

@halseth halseth force-pushed the halseth:max-htlc-size-pickup branch from a7af14e to fca8955 Jan 11, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 12, 2019

Build failing with new travis failures.

@halseth halseth force-pushed the halseth:max-htlc-size-pickup branch 3 times, most recently from 28b01cc to 623ad8d Jan 12, 2019

@valentinewallace
Copy link
Contributor

valentinewallace left a comment

Very nice @halseth :) I like how you also added validation for the MaxValueInFlight. Also, I agree with the decision to use the opaque bytes for the max HTLC amount to avoid a migration, until we have TLV.

Show resolved Hide resolved lnwire/lnwire_test.go
Show resolved Hide resolved lnwire/channel_update.go
func validateOptionalFields(capacity btcutil.Amount,
msg *lnwire.ChannelUpdate) error {

if msg.MessageFlags&lnwire.ChanUpdateOptionMaxHtlc != 0 {

This comment has been minimized.

@valentinewallace

valentinewallace Jan 13, 2019

Contributor

When will lnwire.ChanUpdateOptionMaxHtlc be zero? Confused bc it's hardcoded here.

This comment has been minimized.

@Roasbeef

Roasbeef Jan 16, 2019

Member

It won't ever be zero, we're checking here to see if the max HTLC bit is set in the message flags variable. If it isn't set (all zeroes), then this won't execute. If it is, then the only thing left on the LHS there will be the max HTLC bit.

This comment has been minimized.

@Roasbeef

Roasbeef Jan 16, 2019

Member

In this case, I personally prefer to write this instead:

if msg.MessageFlags&lnwire.ChanUpdateOptionMaxHtlc == lnwire.ChanUpdateOptionMaxHtlc

Usually with a helper:

func hasFlag(flags byte, flag byte) bool {
    return flags&flag == flag 
}

This comment has been minimized.

@halseth

halseth Jan 16, 2019

Author Collaborator

I personally prefer the current flags&flag != 0 style because it is less verbose (C style), but agree that a helper method makes it more readable!

This comment has been minimized.

@halseth

halseth Jan 16, 2019

Author Collaborator

Added a helper here: 1ffebc8

lmk if that's what you envisioned!

func validateOptionalFields(capacity btcutil.Amount,
msg *lnwire.ChannelUpdate) error {

if msg.MessageFlags&lnwire.ChanUpdateOptionMaxHtlc != 0 {

This comment has been minimized.

@valentinewallace

valentinewallace Jan 13, 2019

Contributor

Should we invalidate if msg.MessageFlags is not set but msg.HtlcMaximumMsat is set?

This comment has been minimized.

@Roasbeef

Roasbeef Jan 16, 2019

Member

Agree we should reject that. However, this should be in the lnwire layer: if as we're reading, we see the max HTLC bit set, but then there's no opaque data, or not enough, we should reject the message.

This comment has been minimized.

@halseth

halseth Jan 16, 2019

Author Collaborator

If the message flag is set, but there's not enough data in the byte stream for the field to be there, we do reject it in the lnwire layer.

If the message flag is not set, we just interpret the data as opaque if it's there. (still talking about lnwire here, meaning that when the message is handed to discovery or routing the flag cannot be set and msg.HtlcMaximumMsat not set at the same time )

EDIT: added lnwire clarification

Show resolved Hide resolved channeldb/graph.go
Show resolved Hide resolved channeldb/graph.go
Show resolved Hide resolved channeldb/graph_test.go Outdated
Show resolved Hide resolved lnwire/channel_update.go
Show resolved Hide resolved lnwire/channel_update.go
func validateOptionalFields(capacity btcutil.Amount,
msg *lnwire.ChannelUpdate) error {

if msg.MessageFlags&lnwire.ChanUpdateOptionMaxHtlc != 0 {

This comment has been minimized.

@Roasbeef

Roasbeef Jan 16, 2019

Member

It won't ever be zero, we're checking here to see if the max HTLC bit is set in the message flags variable. If it isn't set (all zeroes), then this won't execute. If it is, then the only thing left on the LHS there will be the max HTLC bit.

func validateOptionalFields(capacity btcutil.Amount,
msg *lnwire.ChannelUpdate) error {

if msg.MessageFlags&lnwire.ChanUpdateOptionMaxHtlc != 0 {

This comment has been minimized.

@Roasbeef

Roasbeef Jan 16, 2019

Member

Agree we should reject that. However, this should be in the lnwire layer: if as we're reading, we see the max HTLC bit set, but then there's no opaque data, or not enough, we should reject the message.

Show resolved Hide resolved channeldb/graph.go
func validateOptionalFields(capacity btcutil.Amount,
msg *lnwire.ChannelUpdate) error {

if msg.MessageFlags&lnwire.ChanUpdateOptionMaxHtlc != 0 {

This comment has been minimized.

@Roasbeef

Roasbeef Jan 16, 2019

Member

In this case, I personally prefer to write this instead:

if msg.MessageFlags&lnwire.ChanUpdateOptionMaxHtlc == lnwire.ChanUpdateOptionMaxHtlc

Usually with a helper:

func hasFlag(flags byte, flag byte) bool {
    return flags&flag == flag 
}
oldEdgePolicy, err := deserializeChanEdgePolicy(
bytes.NewReader(edgeBytes), nodes,
)
if err != nil {
if err != nil && err != ErrEdgePolicyOptionalFieldNotFound {

This comment has been minimized.

@Roasbeef

Roasbeef Jan 16, 2019

Member

We may want to eventually even just do a migration (or call it a database sanity check) that finds and deletes any channel updates that are malformed.

This comment has been minimized.

@halseth

halseth Jan 16, 2019

Author Collaborator

Added a TODO.

Show resolved Hide resolved discovery/gossiper.go
Show resolved Hide resolved server.go
Show resolved Hide resolved fundingmanager.go

@halseth halseth force-pushed the halseth:max-htlc-size-pickup branch 2 times, most recently from 1ffebc8 to 02b83dd Jan 16, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Jan 16, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Jan 17, 2019

Fails lint:

lnwallet/interface_test.go:440:46:error: not enough arguments in call to aliceChanReservation.CommitConstraints (vet)
lnwallet/interface_test.go:872:46:error: not enough arguments in call to aliceChanReservation.CommitConstraints (vet)

@halseth halseth force-pushed the halseth:max-htlc-size-pickup branch from 02b83dd to 5089304 Jan 17, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Jan 17, 2019

Lint fixed in 5089304

@halseth halseth force-pushed the halseth:max-htlc-size-pickup branch from 5089304 to 580d534 Jan 17, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 🔭

Tested on a few existing nodes, and found no major issues. After upgrading we see all the channels re-transmitted to update peers with the new max_htlc fields:

2019-01-21 19:13:10.520 [INF] DISC: Retransmitting 73 outgoing channels

Sample log message once a peer requests the new updates:

2019-01-21 19:19:44.479 [TRC] PEER: writeMessage to 82.196.97.86:9735: (*lnwire.ChannelUpdate)(0xc0011e04d0)({
 Signature: (lnwire.Sig) (len=64 cap=64) {
  00000000  ec 54 48 2a 64 45 75 65  09 d8 42 e3 b3 11 6d f7  |.TH*dEue..B...m.|
  00000010  74 9f 8c 55 55 6a 3d f1  c6 19 5c e8 2b f7 d2 04  |t..UUj=...\.+...|
  00000020  67 c6 90 78 68 1c 31 01  60 b3 d4 34 9f e8 21 23  |g..xh.1.`..4..!#|
  00000030  b0 e8 43 67 90 ae 3c a8  19 d5 a6 c5 e3 89 68 cb  |..Cg..<.......h.|
 },
 ChainHash: (chainhash.Hash) (len=32 cap=32) 000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943,
 ShortChannelID: (lnwire.ShortChannelID) 1449209:8:1,
 Timestamp: (uint32) 1548116368,
 MessageFlags: (lnwire.ChanUpdateMsgFlags) 00000001,
 ChannelFlags: (lnwire.ChanUpdateChanFlags) 00000000,
 TimeLockDelta: (uint16) 71,
 HtlcMinimumMsat: (lnwire.MilliSatoshi) 1000 mSAT,
 BaseFee: (uint32) 2000,
 FeeRate: (uint32) 100,
 HtlcMaximumMsat: (lnwire.MilliSatoshi) 6044166000 mSAT,
 ExtraOpaqueData: ([]uint8) {
 }
})

Ready to merge after rebase.

return err
}

var scratch [8]byte
updateUnix := uint64(edge.LastUpdate.Unix())
byteOrder.PutUint64(scratch[:], updateUnix)
if _, err := b.Write(scratch[:]); err != nil {

This comment has been minimized.

@Roasbeef

Roasbeef Jan 22, 2019

Member

Instead of using this intermediate buffer (that we used to buffer the update time before writing), we can put the update time directly into the indexKey buffer below.

This comment has been minimized.

@halseth

halseth Jan 22, 2019

Author Collaborator

Done!

Show resolved Hide resolved channeldb/graph.go

valentinewallace and others added some commits Jan 12, 2019

multi: partition lnwire.ChanUpdateFlag into ChannelFlags and MessageF…
…lags

In this commit:

* we partition lnwire.ChanUpdateFlag into two (ChanUpdateChanFlags and
ChanUpdateMsgFlags), from a uint16 to a pair of uint8's

* we rename the ChannelUpdate.Flags to ChannelFlags and add an
additional MessageFlags field, which will be used to indicate the
presence of the optional field HtlcMaximumMsat within the ChannelUpdate.

* we partition ChannelEdgePolicy.Flags into message and channel flags.
This change corresponds to the partitioning of the ChannelUpdate's Flags
field into MessageFlags and ChannelFlags.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
discovery/gossiper_test: mock AddEdge: set capacity
In this commit, we modify the mockGraphSource's `AddEdge`
method to set the capacity of the edge it's adding to be a large
capacity.

This will enable us to test the validation of each ChannelUpdate's
max HTLC, since future validation checks will ensure the specified
max HTLC is less than total channel capacity.
lnwire: add HtlcMaximumMsat field to ChannelUpdate
In this commit, we add a field to the ChannelUpdate
denoting the maximum HTLC we support sending over
this channel, a field which was recently added to the
spec.

This field serves multiple purposes. In the short
term, it enables nodes to signal the largest HTLC
they're willing to carry, allows light clients who
don't verify channel existence to have some guidance
when routing HTLCs, and finally may allow nodes to
preserve a portion of bandwidth at all times.

In the long term, this field can be used by
implementations of AMP to guide payment splitting,
as it becomes apparent to a node the largest possible
HTLC one can route over a particular channel.

This PR was made possible by the merge of #1825,
which enables older nodes to properly retain and
verify signatures on updates that include new fields
(like this new max HTLC field) that they haven't yet
been updated to recognize.

In addition, the new ChannelUpdate fields are added to
the lnwire fuzzing tests.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
lnwire/channel_update: add String method for ChanUpdate[Chan|Msg]Flags
In this commit, we fix the problem where it's annoying to parse a
bitfield printed out in decimal by writing a String method for the
ChanUpdate[Chan|Msg]Flags bitfield.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
discovery/gossiper_test: set ChannelUpdate max htlc
In this commit, we alter the gossiper test's helper method
that creates channel updates to include the max htlc field
in the ChannelUpdates it creates.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
discovery+routing: validate msg flags and max htlc in ChannelUpdates
In this commit, we alter the ValidateChannelUpdateAnn function in
ann_validation to validate a remote ChannelUpdate's message flags
and max HTLC field. If the message flag is set but the max HTLC
field is not set or vice versa, the ChannelUpdate fails validation.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
channeldb/graph: add max HTLC to ChannelEdgePolicy
Adding this field will allow us to persist an edge's
max HTLC to disk, thus preserving it between restarts.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
channeldb/graph_test: assert MaxHTLC field correctness
Co-authored-by: Valentine Wallace <valentine.m.wallace@gmail.com>

halseth and others added some commits Jan 12, 2019

channeldb/channel: ignore ChannelEdgePolicy with missing max_htlc
If the max_htlc field is not found when fetching a ChannelEdgePolicy
from the DB, we treat this as an unknown policy.

This is done to ensure we won't propagate invalid data further. The data
will be overwritten with a valid one when we receive an update for this
channel.

It shouldn't be very common, but old data could be lingering in the DB
added before this field was validated.
discovery/chan_series+utils: include max htlc when syncing with peers
In this commit, we ensure that max HTLC is included when we're
synchronizing ChannelUpdates with remote peers.
discovery/gossiper: include max HTLC when rebroadcasting stale channe…
…l updates

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
server: set max htlc in ChannelUpdate createChannelUpdate
This method is called to convert an EdgePolicy to a ChannelUpdate. We
make sure to carry over the max_htlc value.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
routing/router: when applying onion failure channel update, set max htlc
In this commit, we ensure that when we update an edge
as a result of a ChannelUpdate being returned from an
onion failure, the max htlc portion of the channel update
is included in the edge update.
autopilot/graph: set max htlc in test edge policies
Co-authored-by: Johan T. Halseth <johanth@gmail.com>
fundingmanager: when funding new channels, set a default max htlc
In this commit, we set a default max HTLC value in ChannelUpdates
sent out for newly funded channels. As a result, we also default
to setting `MessageFlags` equal to 1 in each new ChannelUpdate, since
the max HTLC field is an optional field and MessageFlags indicates
the presence of optional fields within the ChannelUpdate.

For a default max HTLC, we choose the maximum msats worth of
HTLCs that can be pending (or in-flight) on our side of the channel.
The reason for this is because the spec specifies that the max
HTLC present in a ChannelUpdate must be less than or equal to
both total channel capacity and the maximum in-flight amount set
by the peer. Since this in-flight value will always be less than
or equal to channel capacity, it is a safe spec-compliant default.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>
fundingmanager_test: verify max HTLC in ChannelUpdates
In this commit, we verify that ChannelUpdates for newly
funded channels contain the max HTLC that we expect.
We expect the max HTLC value of each ChannelUpdate to
equal the maximum pending msats in HTLCs required by
the remote peer.

Co-authored-by: Johan T. Halseth <johanth@gmail.com>

@halseth halseth force-pushed the halseth:max-htlc-size-pickup branch from 580d534 to 7d34ce9 Jan 22, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Jan 22, 2019

Rebased.

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 👑

@Roasbeef Roasbeef merged commit e228573 into lightningnetwork:master Jan 23, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.008%) to 56.317%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment