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
invoices: expose custom tlv records from the payload #3742
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import ( | |
"time" | ||
|
||
"github.com/coreos/bbolt" | ||
"github.com/lightningnetwork/lnd/htlcswitch/hop" | ||
"github.com/lightningnetwork/lnd/lntypes" | ||
"github.com/lightningnetwork/lnd/lnwire" | ||
"github.com/lightningnetwork/lnd/tlv" | ||
|
@@ -308,6 +309,10 @@ type InvoiceHTLC struct { | |
// canceled htlc isn't just removed from the invoice htlcs map, because | ||
// we need AcceptHeight to properly cancel the htlc back. | ||
State HtlcState | ||
|
||
// CustomRecords contains the custom key/value pairs that accompanied | ||
// the htlc. | ||
CustomRecords hop.CustomRecordSet | ||
} | ||
|
||
// HtlcAcceptDesc describes the details of a newly accepted htlc. | ||
|
@@ -320,6 +325,10 @@ type HtlcAcceptDesc struct { | |
|
||
// Expiry is the expiry height of this htlc. | ||
Expiry uint32 | ||
|
||
// CustomRecords contains the custom key/value pairs that accompanied | ||
// the htlc. | ||
CustomRecords hop.CustomRecordSet | ||
} | ||
|
||
// InvoiceUpdateDesc describes the changes that should be applied to the | ||
|
@@ -1013,7 +1022,8 @@ func serializeHtlcs(w io.Writer, htlcs map[CircuitKey]*InvoiceHTLC) error { | |
resolveTime := uint64(htlc.ResolveTime.UnixNano()) | ||
state := uint8(htlc.State) | ||
|
||
tlvStream, err := tlv.NewStream( | ||
var records []tlv.Record | ||
records = append(records, | ||
tlv.MakePrimitiveRecord(chanIDType, &chanID), | ||
tlv.MakePrimitiveRecord(htlcIDType, &key.HtlcID), | ||
tlv.MakePrimitiveRecord(amtType, &amt), | ||
|
@@ -1025,6 +1035,16 @@ func serializeHtlcs(w io.Writer, htlcs map[CircuitKey]*InvoiceHTLC) error { | |
tlv.MakePrimitiveRecord(expiryHeightType, &htlc.Expiry), | ||
tlv.MakePrimitiveRecord(htlcStateType, &state), | ||
) | ||
|
||
// Convert the custom records to tlv.Record types that are ready | ||
// for serialization. | ||
customRecords := tlv.MapToRecords(htlc.CustomRecords) | ||
|
||
// Append the custom records. Their ids are in the experimental | ||
// range and sorted, so there is no need to sort again. | ||
records = append(records, customRecords...) | ||
|
||
tlvStream, err := tlv.NewStream(records...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question open as to whether we want to bump the db version, to prevent users running master from downgrading and loosing their custom records There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed this in the context of another PR. We don't give this guarantee to users running master generally. |
||
if err != nil { | ||
return err | ||
} | ||
|
@@ -1191,7 +1211,8 @@ func deserializeHtlcs(r io.Reader) (map[CircuitKey]*InvoiceHTLC, error) { | |
return nil, err | ||
} | ||
|
||
if err := tlvStream.Decode(htlcReader); err != nil { | ||
parsedTypes, err := tlvStream.DecodeWithParsedTypes(htlcReader) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
|
@@ -1201,6 +1222,10 @@ func deserializeHtlcs(r io.Reader) (map[CircuitKey]*InvoiceHTLC, error) { | |
htlc.State = HtlcState(state) | ||
htlc.Amt = lnwire.MilliSatoshi(amt) | ||
|
||
// Reconstruct the custom records fields from the parsed types | ||
// map return from the tlv parser. | ||
htlc.CustomRecords = hop.NewCustomRecords(parsedTypes) | ||
|
||
htlcs[key] = &htlc | ||
} | ||
|
||
|
@@ -1290,12 +1315,20 @@ func (d *DB) updateInvoice(hash lntypes.Hash, invoices, settleIndex *bbolt.Bucke | |
if _, exists := invoice.Htlcs[key]; exists { | ||
return nil, fmt.Errorf("duplicate add of htlc %v", key) | ||
} | ||
|
||
// Force caller to supply htlc without custom records in a | ||
// consistent way. | ||
if htlcUpdate.CustomRecords == nil { | ||
cfromknecht marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return nil, errors.New("nil custom records map") | ||
} | ||
|
||
htlc := &InvoiceHTLC{ | ||
Amt: htlcUpdate.Amt, | ||
Expiry: htlcUpdate.Expiry, | ||
AcceptHeight: uint32(htlcUpdate.AcceptHeight), | ||
AcceptTime: now, | ||
State: HtlcStateAccepted, | ||
Amt: htlcUpdate.Amt, | ||
Expiry: htlcUpdate.Expiry, | ||
AcceptHeight: uint32(htlcUpdate.AcceptHeight), | ||
AcceptTime: now, | ||
State: HtlcStateAccepted, | ||
CustomRecords: htlcUpdate.CustomRecords, | ||
} | ||
|
||
invoice.Htlcs[key] = htlc | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offline comment from @cfromknecht: not sure if it is a good idea to mix the custom records with the db types here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this further. It may be better to create a single tlv record for which the value is a tlv stream with just the custom records. As a simple insurance against accidental modification of standard fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. There seems to be more support for a flat structure, so leaving it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still somewhat concerned about mingling minimally validated user data with known records. I think it is possible to do it safely, but i think it forces us to be stricter in how we handle migrations.
Consider the scenario where we add a field to the invoice body but do not bump the db version. If a user downgrades, this will now appear in the custom record set even tho its type is beneath the cutoff. I think it's possible that data gets written back, but it's difficult imo to ascertain whether this can lead to unforeseen bugs.
We could prevent this by filter the custom records when reading as we do in the hop, then the data is actually dropped when writing back. This would maintain the current behavior in that scenario.