Skip to content

Support group keys on SendPayment & AddInvoice #1423

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

Merged
merged 15 commits into from
Apr 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions asset/asset.go
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,23 @@ func NewSpecifierFromGroupKey(groupPubKey btcec.PublicKey) Specifier {
}
}

// NewExlusiveSpecifier creates a specifier that may only include one of asset
// ID or group key. If both are set then a specifier over the group key is
// created.
func NewExclusiveSpecifier(id *ID,
groupPubKey *btcec.PublicKey) (Specifier, error) {

switch {
case groupPubKey != nil:
return NewSpecifierFromGroupKey(*groupPubKey), nil

case id != nil:
return NewSpecifierFromId(*id), nil
}

return Specifier{}, fmt.Errorf("must set either asset ID or group key")
}

// String returns a human-readable description of the specifier.
func (s *Specifier) String() string {
// An unset asset ID is represented as an empty string.
Expand Down
22 changes: 20 additions & 2 deletions rfq/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ import (
"context"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
"sync"
"time"

"github.com/btcsuite/btcd/btcec/v2"
"github.com/btcsuite/btcd/btcec/v2/schnorr"
"github.com/lightninglabs/lndclient"
"github.com/lightninglabs/taproot-assets/address"
"github.com/lightninglabs/taproot-assets/asset"
"github.com/lightninglabs/taproot-assets/fn"
"github.com/lightninglabs/taproot-assets/rfqmsg"
Expand Down Expand Up @@ -232,6 +235,7 @@ func (m *Manager) startSubsystems(ctx context.Context) error {
HtlcInterceptor: m.cfg.HtlcInterceptor,
HtlcSubscriber: m.cfg.HtlcSubscriber,
AcceptHtlcEvents: m.acceptHtlcEvents,
SpecifierChecker: m.AssetMatchesSpecifier,
})
if err != nil {
return fmt.Errorf("error initializing RFQ order handler: %w",
Expand Down Expand Up @@ -948,6 +952,10 @@ func (m *Manager) getAssetGroupKey(ctx context.Context,
// Perform the DB query.
group, err := m.cfg.GroupLookup.QueryAssetGroup(ctx, id)
if err != nil {
if errors.Is(err, address.ErrAssetGroupUnknown) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this commit, given that we now check whether a given asset ID is a group key before querying the DB?
It just feels weird to return fn.None[btcec.PublicKey](). To me that means: the given asset ID is _not_ part of a group.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is still valid for cases where AssetMatchesSpecifier is called with an asset ID and a specifier which includes a group key

An actual group lookup would have to be performed for that set of inputs. If that asset doesn't belong to a group we would run into the above line.

Copy link
Member

Choose a reason for hiding this comment

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

An actual group lookup would have to be performed for that set of inputs. If that asset doesn't belong to a group we would run into the above line.

Seems related to the first comment I left in this round of review. Looks like we would do a DB look up to make sure that we didn't accidentally interpret an asset ID as a group key.

return fn.None[btcec.PublicKey](), nil
}

return fn.None[btcec.PublicKey](), err
}

Expand All @@ -971,6 +979,18 @@ func (m *Manager) AssetMatchesSpecifier(ctx context.Context,

switch {
case specifier.HasGroupPubKey():
specifierGK := specifier.UnwrapGroupKeyToPtr()

// Let's directly check if the ID is equal to the X coordinate
// of the group key. This is used by the sender to indicate that
// any asset that belongs to this group may be used.
groupKeyX := schnorr.SerializePubKey(specifierGK)
if asset.ID(groupKeyX) == id {
return true, nil
}

// Now let's make an actual query to find this assetID's group,
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, given that we don't call getAssetGroupKey with a "fake" asset ID anymore, I think we can potentially remove the previous commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

// if it exists.
group, err := m.getAssetGroupKey(ctx, id)
if err != nil {
return false, err
Expand All @@ -980,8 +1000,6 @@ func (m *Manager) AssetMatchesSpecifier(ctx context.Context,
return false, nil
}

specifierGK := specifier.UnwrapGroupKeyToPtr()

return group.UnwrapToPtr().IsEqual(specifierGK), nil

case specifier.HasId():
Expand Down
173 changes: 173 additions & 0 deletions rfq/marshal.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
package rfq

import (
"fmt"

"github.com/lightninglabs/taproot-assets/fn"
"github.com/lightninglabs/taproot-assets/rfqmath"
"github.com/lightninglabs/taproot-assets/rfqmsg"
"github.com/lightninglabs/taproot-assets/taprpc/rfqrpc"
)

// MarshalAcceptedSellQuoteEvent marshals a peer accepted sell quote event to
// its RPC representation.
func MarshalAcceptedSellQuoteEvent(
event *PeerAcceptedSellQuoteEvent) *rfqrpc.PeerAcceptedSellQuote {

return MarshalAcceptedSellQuote(event.SellAccept)
}

// MarshalAcceptedSellQuote marshals a peer accepted sell quote to its RPC
// representation.
func MarshalAcceptedSellQuote(
accept rfqmsg.SellAccept) *rfqrpc.PeerAcceptedSellQuote {

rpcAssetRate := &rfqrpc.FixedPoint{
Coefficient: accept.AssetRate.Rate.Coefficient.String(),
Scale: uint32(accept.AssetRate.Rate.Scale),
}

// Calculate the equivalent asset units for the given total BTC amount
// based on the asset-to-BTC conversion rate.
numAssetUnits := rfqmath.MilliSatoshiToUnits(
accept.Request.PaymentMaxAmt, accept.AssetRate.Rate,
)

minTransportableMSat := rfqmath.MinTransportableMSat(
rfqmath.DefaultOnChainHtlcMSat, accept.AssetRate.Rate,
)

return &rfqrpc.PeerAcceptedSellQuote{
Peer: accept.Peer.String(),
Id: accept.ID[:],
Scid: uint64(accept.ShortChannelId()),
BidAssetRate: rpcAssetRate,
Expiry: uint64(accept.AssetRate.Expiry.Unix()),
AssetAmount: numAssetUnits.ScaleTo(0).ToUint64(),
MinTransportableMsat: uint64(minTransportableMSat),
}
}

// MarshalAcceptedBuyQuoteEvent marshals a peer accepted buy quote event to
// its rpc representation.
func MarshalAcceptedBuyQuoteEvent(
event *PeerAcceptedBuyQuoteEvent) (*rfqrpc.PeerAcceptedBuyQuote,
error) {

// We now calculate the minimum amount of asset units that can be
// transported within a single HTLC for this asset at the given rate.
// This corresponds to the 354 satoshi minimum non-dust HTLC value.
minTransportableUnits := rfqmath.MinTransportableUnits(
rfqmath.DefaultOnChainHtlcMSat, event.AssetRate.Rate,
).ScaleTo(0).ToUint64()

return &rfqrpc.PeerAcceptedBuyQuote{
Peer: event.Peer.String(),
Id: event.ID[:],
Scid: uint64(event.ShortChannelId()),
AssetMaxAmount: event.Request.AssetMaxAmt,
AskAssetRate: &rfqrpc.FixedPoint{
Coefficient: event.AssetRate.Rate.Coefficient.String(),
Scale: uint32(event.AssetRate.Rate.Scale),
},
Expiry: uint64(event.AssetRate.Expiry.Unix()),
MinTransportableUnits: minTransportableUnits,
}, nil
}

// MarshalInvalidQuoteRespEvent marshals an invalid quote response event to
// its rpc representation.
func MarshalInvalidQuoteRespEvent(
event *InvalidQuoteRespEvent) *rfqrpc.InvalidQuoteResponse {

peer := event.QuoteResponse.MsgPeer()
id := event.QuoteResponse.MsgID()

return &rfqrpc.InvalidQuoteResponse{
Status: rfqrpc.QuoteRespStatus(event.Status),
Peer: peer.String(),
Id: id[:],
}
}

// MarshalIncomingRejectQuoteEvent marshals an incoming reject quote event to
// its RPC representation.
func MarshalIncomingRejectQuoteEvent(
event *IncomingRejectQuoteEvent) *rfqrpc.RejectedQuoteResponse {

return &rfqrpc.RejectedQuoteResponse{
Peer: event.Peer.String(),
Id: event.ID.Val[:],
ErrorMessage: event.Err.Val.Msg,
ErrorCode: uint32(event.Err.Val.Code),
}
}

// NewAddAssetBuyOrderResponse creates a new AddAssetBuyOrderResponse from
// the given RFQ event.
func NewAddAssetBuyOrderResponse(
event fn.Event) (*rfqrpc.AddAssetBuyOrderResponse, error) {

resp := &rfqrpc.AddAssetBuyOrderResponse{}

switch e := event.(type) {
case *PeerAcceptedBuyQuoteEvent:
acceptedQuote, err := MarshalAcceptedBuyQuoteEvent(e)
if err != nil {
return nil, err
}

resp.Response = &rfqrpc.AddAssetBuyOrderResponse_AcceptedQuote{
AcceptedQuote: acceptedQuote,
}
return resp, nil

case *InvalidQuoteRespEvent:
resp.Response = &rfqrpc.AddAssetBuyOrderResponse_InvalidQuote{
InvalidQuote: MarshalInvalidQuoteRespEvent(e),
}
return resp, nil

case *IncomingRejectQuoteEvent:
resp.Response = &rfqrpc.AddAssetBuyOrderResponse_RejectedQuote{
RejectedQuote: MarshalIncomingRejectQuoteEvent(e),
}
return resp, nil

default:
return nil, fmt.Errorf("unknown AddAssetBuyOrder event "+
"type: %T", e)
}
}

// NewAddAssetSellOrderResponse creates a new AddAssetSellOrderResponse from
// the given RFQ event.
func NewAddAssetSellOrderResponse(
event fn.Event) (*rfqrpc.AddAssetSellOrderResponse, error) {

resp := &rfqrpc.AddAssetSellOrderResponse{}

switch e := event.(type) {
case *PeerAcceptedSellQuoteEvent:
resp.Response = &rfqrpc.AddAssetSellOrderResponse_AcceptedQuote{
AcceptedQuote: MarshalAcceptedSellQuoteEvent(e),
}
return resp, nil

case *InvalidQuoteRespEvent:
resp.Response = &rfqrpc.AddAssetSellOrderResponse_InvalidQuote{
InvalidQuote: MarshalInvalidQuoteRespEvent(e),
}
return resp, nil

case *IncomingRejectQuoteEvent:
resp.Response = &rfqrpc.AddAssetSellOrderResponse_RejectedQuote{
RejectedQuote: MarshalIncomingRejectQuoteEvent(e),
}
return resp, nil

default:
return nil, fmt.Errorf("unknown AddAssetSellOrder event "+
"type: %T", e)
}
}
59 changes: 42 additions & 17 deletions rfq/order.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"sync"
"time"

"github.com/btcsuite/btcd/btcec/v2/schnorr"
"github.com/davecgh/go-spew/spew"
"github.com/lightninglabs/lndclient"
"github.com/lightninglabs/taproot-assets/asset"
Expand Down Expand Up @@ -58,7 +59,8 @@ type SerialisedScid = rfqmsg.SerialisedScid
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(ctx context.Context, htlc lndclient.InterceptedHtlc,
specifierChecker rfqmsg.SpecifierChecker) error

// Expiry returns the policy's expiry time as a unix timestamp.
Expiry() uint64
Expand Down Expand Up @@ -145,8 +147,8 @@ func NewAssetSalePolicy(quote rfqmsg.BuyAccept) *AssetSalePolicy {
// included as a hop hint within the invoice. The SCID is the only piece of
// 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 {
func (c *AssetSalePolicy) CheckHtlcCompliance(_ context.Context,
htlc lndclient.InterceptedHtlc, _ rfqmsg.SpecifierChecker) error {

// Since we will be reading CurrentAmountMsat value we acquire a read
// lock.
Expand Down Expand Up @@ -248,11 +250,23 @@ 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

// 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.
switch {
case c.AssetSpecifier.HasGroupPubKey():
groupKey := c.AssetSpecifier.UnwrapGroupKeyToPtr()
groupKeyX := schnorr.SerializePubKey(groupKey)

assetID = asset.ID(groupKeyX)
Copy link
Member

Choose a reason for hiding this comment

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

This is nice compared to the hash version, as this way we aren't actually losing any information.

However, it's possible that a normal asset ID, can actually be interpreted as a valid x coordinate. To avoid this confusion, when we decode on the other side, and conclude it might be a pubkey, we should check our local db to make sure it's actually a group key that we've validated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is the receiver's part that you're describing

// Let's directly check if the ID is equal to the hash of the
// group key. This is used by the sender to indicate that any
// asset that belongs to this group may be used.
groupKeyX := schnorr.SerializePubKey(specifierGK)
if asset.ID(groupKeyX) == id {
return true, nil
}

a receiver is always checking this "assetID" against a specifier. If that specifier contains a groupkey we'll immediately check if the X coordinates match before doing any DB lookups

we should check our local db to make sure it's actually a group key that we've validated.

That's an interesting point. Do we really care if the group key is validated?

If that group key made it into a channel or RFQ quote, then it can't be a fake or un-validated group key. If you have DoS vectors in mind, I haven't digged a lot into that.


case c.AssetSpecifier.HasId():
specifierID := *c.AssetSpecifier.UnwrapIdToPtr()
copy(assetID[:], specifierID[:])
}

// Compute the outgoing asset amount given the msat outgoing amount and
Expand Down Expand Up @@ -341,8 +355,9 @@ 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 {
func (c *AssetPurchasePolicy) CheckHtlcCompliance(ctx context.Context,
htlc lndclient.InterceptedHtlc,
specifierChecker rfqmsg.SpecifierChecker) error {

// Since we will be reading CurrentAmountMsat value we acquire a read
// lock.
Expand All @@ -368,7 +383,9 @@ func (c *AssetPurchasePolicy) CheckHtlcCompliance(
}

// Sum the asset balance in the HTLC record.
assetAmt, err := htlcRecord.SumAssetBalance(c.AssetSpecifier)
assetAmt, err := htlcRecord.SumAssetBalance(
ctx, c.AssetSpecifier, specifierChecker,
)
if err != nil {
return fmt.Errorf("error summing asset balance: %w", err)
}
Expand Down Expand Up @@ -523,15 +540,19 @@ 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 {
func (a *AssetForwardPolicy) CheckHtlcCompliance(ctx context.Context,
htlc lndclient.InterceptedHtlc, sChk rfqmsg.SpecifierChecker) error {

if err := a.incomingPolicy.CheckHtlcCompliance(htlc); err != nil {
if err := a.incomingPolicy.CheckHtlcCompliance(
ctx, 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(
ctx, htlc, sChk,
); err != nil {
return fmt.Errorf("error checking forward policy, outbound "+
"HTLC does not comply with policy: %w", err)
}
Expand Down Expand Up @@ -642,6 +663,10 @@ type OrderHandlerCfg struct {
// HtlcSubscriber is a subscriber that is used to retrieve live HTLC
// event updates.
HtlcSubscriber HtlcSubscriber

// SpecifierChecker is an interface that contains methods for
// checking certain properties related to asset specifiers.
SpecifierChecker rfqmsg.SpecifierChecker
}

// OrderHandler orchestrates management of accepted quote bundles. It monitors
Expand Down Expand Up @@ -684,7 +709,7 @@ func NewOrderHandler(cfg OrderHandlerCfg) (*OrderHandler, error) {
//
// NOTE: This function must be thread safe. It is used by an external
// interceptor service.
func (h *OrderHandler) handleIncomingHtlc(_ context.Context,
func (h *OrderHandler) handleIncomingHtlc(ctx context.Context,
htlc lndclient.InterceptedHtlc) (*lndclient.InterceptedHtlcResponse,
error) {

Expand Down Expand Up @@ -716,7 +741,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(ctx, htlc, h.cfg.SpecifierChecker)
if err != nil {
log.Warnf("HTLC does not comply with policy: %v "+
"(HTLC=%v, policy=%v)", err, htlc, policy)
Expand Down
Loading
Loading