-
Notifications
You must be signed in to change notification settings - Fork 132
Support group keys on HTLC Interceptor & Invoice HTLC Modifier #1416
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
Changes from all commits
234e644
a94e9e3
338a6aa
e3be2a0
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 |
---|---|---|
|
@@ -3,6 +3,7 @@ package rfq | |
import ( | ||
"bytes" | ||
"context" | ||
"crypto/sha256" | ||
"fmt" | ||
"sync" | ||
"time" | ||
|
@@ -53,12 +54,23 @@ func parseHtlcCustomRecords(customRecords map[uint64][]byte) (*rfqmsg.Htlc, | |
// SerialisedScid is a serialised short channel id (SCID). | ||
type SerialisedScid = rfqmsg.SerialisedScid | ||
|
||
// AssetSpecifierChecker is an interface that contains methods for checking | ||
// certain properties related to asset specifiers. | ||
type AssetSpecifierChecker interface { | ||
// AssetMatchesSpecifier checks whether the passed specifier and asset | ||
// ID match. If the specifier contains a group key, it will check | ||
// whether the asset belongs to that group. | ||
AssetMatchesSpecifier(ctx context.Context, | ||
specifier asset.Specifier, id asset.ID) (bool, error) | ||
} | ||
|
||
// Policy is an interface that abstracts the terms which determine whether an | ||
// asset sale/purchase channel HTLC is accepted or rejected. | ||
type Policy interface { | ||
// CheckHtlcCompliance returns an error if the given HTLC intercept | ||
// descriptor does not satisfy the subject policy. | ||
CheckHtlcCompliance(htlc lndclient.InterceptedHtlc) error | ||
CheckHtlcCompliance(htlc lndclient.InterceptedHtlc, | ||
specifierChecker AssetSpecifierChecker) error | ||
|
||
// Expiry returns the policy's expiry time as a unix timestamp. | ||
Expiry() uint64 | ||
|
@@ -146,7 +158,8 @@ func NewAssetSalePolicy(quote rfqmsg.BuyAccept) *AssetSalePolicy { | |
// information used to determine the policy applicable to the HTLC. As a result, | ||
// HTLC custom records are not expected to be present. | ||
func (c *AssetSalePolicy) CheckHtlcCompliance( | ||
htlc lndclient.InterceptedHtlc) error { | ||
htlc lndclient.InterceptedHtlc, | ||
specifierChecker AssetSpecifierChecker) error { | ||
|
||
// Since we will be reading CurrentAmountMsat value we acquire a read | ||
// lock. | ||
|
@@ -248,11 +261,22 @@ func (c *AssetSalePolicy) GenerateInterceptorResponse( | |
|
||
outgoingAmt := rfqmath.DefaultOnChainHtlcMSat | ||
|
||
// Unpack asset ID. | ||
assetID, err := c.AssetSpecifier.UnwrapIdOrErr() | ||
if err != nil { | ||
return nil, fmt.Errorf("asset sale policy has no asset ID: %w", | ||
err) | ||
var assetID asset.ID | ||
|
||
switch { | ||
case c.AssetSpecifier.HasGroupPubKey(): | ||
groupKey := c.AssetSpecifier.UnwrapGroupKeyToPtr() | ||
|
||
// We have performed checks for the asset IDs inside the HTLC | ||
// against the specifier's group key in a previous step. Here | ||
// we just need to provide a dummy value as the asset ID. The | ||
// real asset IDs will be carefully picked in a later step in | ||
// the process. What really matters now is the total amount. | ||
assetID = sha256.Sum256(groupKey.SerializeCompressed()) | ||
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. Why isn't the asset ID relevant here? It looks like it's used to pass along which custom records to use for the outgoing HTLC transformation. IIUC, we need to select the asset ID amongst the set of active channels the outgoing link has that may all be diff, but have the same asset group. 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. We don't know of specific asset IDs on this level, and we don't need do. What we want to do here is signal the amount that needs to be sent to the other side. Channel selection was already taken care of when the receiver negotiated & embedded a quote in the invoice. At this point in the code, we're only intercepting the HTLC which has a predetermined outgoing channel. |
||
|
||
case c.AssetSpecifier.HasId(): | ||
specifierID := *c.AssetSpecifier.UnwrapIdToPtr() | ||
copy(assetID[:], specifierID[:]) | ||
} | ||
|
||
// Compute the outgoing asset amount given the msat outgoing amount and | ||
|
@@ -342,7 +366,8 @@ func NewAssetPurchasePolicy(quote rfqmsg.SellAccept) *AssetPurchasePolicy { | |
// CheckHtlcCompliance returns an error if the given HTLC intercept descriptor | ||
// does not satisfy the subject policy. | ||
func (c *AssetPurchasePolicy) CheckHtlcCompliance( | ||
htlc lndclient.InterceptedHtlc) error { | ||
htlc lndclient.InterceptedHtlc, | ||
specifierChecker AssetSpecifierChecker) error { | ||
|
||
// Since we will be reading CurrentAmountMsat value we acquire a read | ||
// lock. | ||
|
@@ -368,7 +393,9 @@ func (c *AssetPurchasePolicy) CheckHtlcCompliance( | |
} | ||
|
||
// Sum the asset balance in the HTLC record. | ||
assetAmt, err := htlcRecord.SumAssetBalance(c.AssetSpecifier) | ||
assetAmt, err := htlcRecord.SumAssetBalance( | ||
c.AssetSpecifier, specifierChecker, | ||
) | ||
if err != nil { | ||
return fmt.Errorf("error summing asset balance: %w", err) | ||
} | ||
|
@@ -524,14 +551,15 @@ func NewAssetForwardPolicy(incoming, outgoing Policy) (*AssetForwardPolicy, | |
// CheckHtlcCompliance returns an error if the given HTLC intercept descriptor | ||
// does not satisfy the subject policy. | ||
func (a *AssetForwardPolicy) CheckHtlcCompliance( | ||
htlc lndclient.InterceptedHtlc) error { | ||
htlc lndclient.InterceptedHtlc, | ||
sChk AssetSpecifierChecker) error { | ||
|
||
if err := a.incomingPolicy.CheckHtlcCompliance(htlc); err != nil { | ||
if err := a.incomingPolicy.CheckHtlcCompliance(htlc, sChk); err != nil { | ||
return fmt.Errorf("error checking forward policy, inbound "+ | ||
"HTLC does not comply with policy: %w", err) | ||
} | ||
|
||
if err := a.outgoingPolicy.CheckHtlcCompliance(htlc); err != nil { | ||
if err := a.outgoingPolicy.CheckHtlcCompliance(htlc, sChk); err != nil { | ||
return fmt.Errorf("error checking forward policy, outbound "+ | ||
"HTLC does not comply with policy: %w", err) | ||
} | ||
|
@@ -642,6 +670,10 @@ type OrderHandlerCfg struct { | |
// HtlcSubscriber is a subscriber that is used to retrieve live HTLC | ||
// event updates. | ||
HtlcSubscriber HtlcSubscriber | ||
|
||
// AssetSpecifierChecker is an interface that contains methods for | ||
// checking certain properties related to asset specifiers. | ||
AssetSpecifierChecker AssetSpecifierChecker | ||
} | ||
|
||
// OrderHandler orchestrates management of accepted quote bundles. It monitors | ||
|
@@ -716,7 +748,7 @@ func (h *OrderHandler) handleIncomingHtlc(_ context.Context, | |
// At this point, we know that a policy exists and has not expired | ||
// whilst sitting in the local cache. We can now check that the HTLC | ||
// complies with the policy. | ||
err = policy.CheckHtlcCompliance(htlc) | ||
err = policy.CheckHtlcCompliance(htlc, h.cfg.AssetSpecifierChecker) | ||
if err != nil { | ||
log.Warnf("HTLC does not comply with policy: %v "+ | ||
"(HTLC=%v, policy=%v)", err, htlc, policy) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,12 @@ type RfqManager interface { | |
// node and have been requested by our peers. These quotes are | ||
// exclusively available to our node for the sale of assets. | ||
LocalAcceptedSellQuotes() rfq.SellAcceptMap | ||
|
||
// AssetMatchesSpecifier checks if the provided asset satisfies the | ||
// provided specifier. If the specifier includes a group key, we will | ||
// check if the asset belongs to that group. | ||
AssetMatchesSpecifier(ctx context.Context, specifier asset.Specifier, | ||
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. Seems the 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. I usually stick with the interface approach, which also serves as a hint w.r.t what parts of rfq manager we'll be using. |
||
id asset.ID) (bool, error) | ||
} | ||
|
||
// A compile time assertion to ensure that the rfq.Manager meets the expected | ||
|
@@ -128,7 +134,7 @@ func (s *AuxInvoiceManager) Start() error { | |
// handleInvoiceAccept is the handler that will be called for each invoice that | ||
// is accepted. It will intercept the HTLCs that attempt to settle the invoice | ||
// and modify them if necessary. | ||
func (s *AuxInvoiceManager) handleInvoiceAccept(_ context.Context, | ||
func (s *AuxInvoiceManager) handleInvoiceAccept(ctx context.Context, | ||
req lndclient.InvoiceHtlcModifyRequest) ( | ||
*lndclient.InvoiceHtlcModifyResponse, error) { | ||
|
||
|
@@ -200,7 +206,7 @@ func (s *AuxInvoiceManager) handleInvoiceAccept(_ context.Context, | |
} | ||
|
||
// We now run some validation checks on the asset HTLC. | ||
err = s.validateAssetHTLC(htlc) | ||
err = s.validateAssetHTLC(ctx, htlc) | ||
if err != nil { | ||
log.Errorf("Failed to validate asset HTLC: %v", err) | ||
|
||
|
@@ -274,22 +280,23 @@ func (s *AuxInvoiceManager) identifierFromQuote( | |
buyQuote, isBuy := acceptedBuyQuotes[rfqID.Scid()] | ||
sellQuote, isSell := acceptedSellQuotes[rfqID.Scid()] | ||
|
||
var specifier asset.Specifier | ||
|
||
switch { | ||
case isBuy: | ||
if buyQuote.Request.AssetSpecifier.HasId() { | ||
req := buyQuote.Request | ||
return req.AssetSpecifier, nil | ||
} | ||
specifier = buyQuote.Request.AssetSpecifier | ||
|
||
case isSell: | ||
if sellQuote.Request.AssetSpecifier.HasId() { | ||
req := sellQuote.Request | ||
return req.AssetSpecifier, nil | ||
} | ||
specifier = sellQuote.Request.AssetSpecifier | ||
} | ||
|
||
err := specifier.AssertNotEmpty() | ||
if err != nil { | ||
return specifier, fmt.Errorf("rfqID does not match any "+ | ||
"accepted buy or sell quote: %v", err) | ||
} | ||
|
||
return asset.Specifier{}, fmt.Errorf("rfqID does not match any " + | ||
"accepted buy or sell quote") | ||
return specifier, nil | ||
} | ||
|
||
// priceFromQuote retrieves the price from the accepted quote for the given RFQ | ||
|
@@ -382,7 +389,9 @@ func isAssetInvoice(invoice *lnrpc.Invoice, rfqLookup RfqLookup) bool { | |
} | ||
|
||
// validateAssetHTLC runs a couple of checks on the provided asset HTLC. | ||
func (s *AuxInvoiceManager) validateAssetHTLC(htlc *rfqmsg.Htlc) error { | ||
func (s *AuxInvoiceManager) validateAssetHTLC(ctx context.Context, | ||
htlc *rfqmsg.Htlc) error { | ||
|
||
rfqID := htlc.RfqID.ValOpt().UnsafeFromSome() | ||
|
||
// Retrieve the asset identifier from the RFQ quote. | ||
|
@@ -392,27 +401,21 @@ func (s *AuxInvoiceManager) validateAssetHTLC(htlc *rfqmsg.Htlc) error { | |
"quote: %v", err) | ||
} | ||
|
||
if !identifier.HasId() { | ||
return fmt.Errorf("asset specifier has empty assetID") | ||
} | ||
|
||
// Check for each of the asset balances of the HTLC that the identifier | ||
// matches that of the RFQ quote. | ||
for _, v := range htlc.Balances() { | ||
err := fn.MapOptionZ( | ||
identifier.ID(), func(id asset.ID) error { | ||
if v.AssetID.Val != id { | ||
return fmt.Errorf("mismatch between " + | ||
"htlc asset ID and rfq asset " + | ||
"ID") | ||
} | ||
|
||
return nil | ||
}, | ||
match, err := s.cfg.RfqManager.AssetMatchesSpecifier( | ||
ctx, identifier, v.AssetID.Val, | ||
) | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
if !match { | ||
return fmt.Errorf("asset ID %s does not match %s", | ||
v.AssetID.Val.String(), identifier.String()) | ||
} | ||
} | ||
|
||
return nil | ||
|
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 think we should remove the other version of this interface added (in rfqmsg) and just keep this one.
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.
importing this in rfqmsg causes import cycle