Filter order matches by node id #350

positiveblue · 2022-03-28T04:58:07Z

This PR includes the client changes for including node id constrains to pool orders. The users will be able to add a list of allowed/not allowed node ids for their order.

Thinks to take into account:

Every order has its own constrains. I may not want to match with someone in order X but not filter him out in order Y.
The filters are included during the order creation. This is important for sidecars because the provider may need to get the information from the recipient before creating the sidecar.

guggero

Very nice and clean PR!
Only a few minor comments about the TLV encoding/decoding, everything else looks pretty good 🎉

guggero · 2022-03-29T11:34:09Z

poolrpc/trader.proto

+    // List of nodes that will be allowed to match with our order.
+    repeated bytes allowed_node_ids = 14;
+
+    // List of nodes that won't be allowed to match with our order.


nit: add a comment to both that the lists are mutually exclusive.

guggero · 2022-03-29T11:35:48Z

cmd/pool/order.go

@@ -116,6 +116,18 @@ var sharedFlags = []cli.Flag{
 			channelTypePeerDependent,
 			channelTypeScriptEnforced),
 	},
+	cli.StringSliceFlag{
+		Name: "allowed_node_ids",


I assume this is specified multiple times to add multiple IDs? Perhaps the flag name should be singular then and the description should say "can be specified multiple times".

cmd/pool/order.go

guggero · 2022-03-29T11:37:57Z

order/rpc_parse.go

+	if len(details.AllowedNodeIds) > 0 &&
+		len(details.NotAllowedNodeIds) > 0 {
+
+		return nil, errors.New("my story")


order/rpc_parse.go

guggero · 2022-03-29T11:45:09Z

clientdb/order.go

@@ -689,6 +698,29 @@ func deserializeOrderTlvData(r io.Reader, o order.Order) error {
 		o.Details().ChannelType = order.ChannelType(channelType)
 	}

+	if allowedNodeIDsBytes, ok := parsedTypes[allowedNodeIDsType]; ok {


This isn't really correct. You only get the allowedNodeIDsBytes != nil when the TLV library considers this to be an "unknown record type". So instead you should add a primitive record to the top that just expects a byte slice. Then here you can just check t, ok := parsedTypes[allowedNodeIDSType]; ok && t == nil and expect that byte slice to be filled with the data you want.

guggero · 2022-03-29T11:50:20Z

clientdb/order.go

+	if allowedNodeIDsBytes, ok := parsedTypes[allowedNodeIDsType]; ok {
+		numberOfItems := len(allowedNodeIDsBytes) / 33
+		o.Details().AllowedNodeIDs = make([][33]byte, numberOfItems)
+		if err := binary.Read(


This falls back to some reflection magic. Since we know exactly that we're serializing and de-serializing a slice of arrays, should we add two helper functions that then do the explicit 33 byte calculation? Then we can just use copy() and don't need binary.Read() or a buffer for the reverse.

Do you mean flattering the [][33]byte to a []byte and then use the default tlv types serialize/deserialize? then add a function to transform from []byte to [][33]byte?

Yes, exactly. Feels more robust and performant than relying on some reflection code of binary.Read().

guggero · 2022-03-29T11:50:48Z

clientdb/order_test.go

+	// It is not possible for a oder to have AllowedNodeIDs and
+	// NotAllowedNodeIDs at the same time but we want to test
+	// serialization/deserialization here.
+	o.Details().AllowedNodeIDs = [][33]byte{{1, 2, 3}}


we should add at least two IDs to make sure multiple entries can be stored and retrieved correctly.

oh I run it manually to actually test the maximum number, 1000 is ok, 10000 is to much for tlv encoding. I will add few more for the automatic test ✔️

auctioneerrpc/auctioneer.proto

order/manager.go

guggero

Very nice, only nits remaining. Going to do a full test run while reviewing the subasta PR.

guggero · 2022-04-04T16:23:33Z

cmd/pool/order.go

+		Name: "allowed_node_id",
+		Usage: "the list of nodes this order is able to match with; " +
+			"if empty, the order will be able to match with any " +
+			"node unless not_allowed_node_id is set. Cann be " +


nit: typo in "Cann"

guggero · 2022-04-04T16:24:44Z

cmd/pool/order.go

+	},
+	cli.StringSliceFlag{
+		Name: "not_allowed_node_id",
+		Usage: "the list of nodes this order is not able to match " +


nit: s/not able to match with/not allowed to match with? "not able" implies a problem, but this is just a user preference.

guggero · 2022-04-04T16:24:44Z

cmd/pool/order.go

+	},
+	cli.StringSliceFlag{
+		Name: "not_allowed_node_id",
+		Usage: "the list of nodes this order is not able to match " +


nit: s/not able to match with/not allowed to match with? "not able" implies a problem, but this is just a user preference.

guggero · 2022-04-04T16:25:44Z

order/interfaces.go

@@ -340,6 +340,14 @@ type Kit struct {
 	// ChannelType denotes the channel type that must be used for the
 	// resulting matched channels.
 	ChannelType ChannelType
+
+	// AllowedNodeIDs is the list of node ids this order is allowed to
+	// macth with.


nit: typo in "match", here and comment below.

guggero · 2022-04-04T16:25:44Z

order/interfaces.go

@@ -340,6 +340,14 @@ type Kit struct {
 	// ChannelType denotes the channel type that must be used for the
 	// resulting matched channels.
 	ChannelType ChannelType
+
+	// AllowedNodeIDs is the list of node ids this order is allowed to
+	// macth with.


nit: type in "match".

guggero · 2022-04-04T16:28:46Z

clientdb/order.go

+	flatten := make([]byte, len(pubKeys)*33)
+
+	for i, key := range pubKeys {
+		for j, v := range key {


nit: could also use copy() here to avoid a nested loop.

guggero · 2022-04-04T16:29:19Z

clientdb/order.go

+
+	for i := 0; i < numOfPubKeys; i++ {
+		var pubKey [33]byte
+		for j := 0; j < 33; j++ {


Same here, can just use copy with the correct slice indices.

guggero · 2022-04-04T16:29:19Z

clientdb/order.go

+
+	for i := 0; i < numOfPubKeys; i++ {
+		var pubKey [33]byte
+		for j := 0; j < 33; j++ {


Same here, can just use copy with the correct slice indices.

guggero · 2022-04-04T16:30:57Z

order/manager.go

+		return true
+	}
+
+	// If we get here is because both lists were empty


nit: missing full stop.

guggero · 2022-04-04T16:30:57Z

order/manager.go

+		return true
+	}
+
+	// If we get here is because both lists were empty


nit: missing full stop.

guggero · 2022-04-04T16:32:18Z

Needs a rebase as well.

bhandras

Great PR @positiveblue just a few nits, otherwise LGTM 🎉

bhandras · 2022-04-06T09:31:03Z

cmd/pool/order.go

+			"Can be specified multiple times",
+	},
+	cli.StringSliceFlag{
+		Name: "not_allowed_node_id",


nit: maybe just for the cli we could consider renaming this to prohibited_node_id. Potentially even for the protocol if you think it's better and it's still possible.

Not sure about this one. I could change it only in the CLI, but it could leak from some rpc_server errors and potentially confuse the user. For example, parsing the order could return invalid not_allowed_node_id.

Is it because for native speakers prohibited_node_id sounds better than not_allowed_node_id? If so I can change it everywhere, just not sure if it is just a matter of taste.

Yeah, I'm not sure either given I'm not native but just thought it's simpler as "prohibited" is just one word. Regardless I'm fine with the current choice too.

Prohibited sounds good to me. Or just disallowed. "not allowed" does sound a bit bumpy.

But this probably also requires quite a big refactor/renaming so we can also just leave it the way it is.

bhandras · 2022-04-06T10:26:28Z

cmd/pool/order.go

+	for _, hexNodeID := range hexNodeIDs {
+		nodeID, err := hex.DecodeString(hexNodeID)
+		if err != nil {
+			return nil, fmt.Errorf("invalid node ID %v: %v", nodeID,


nit: I think nodeID might be empty here since DecodeString failed so it's better to print hexNodeID.

Much better 👍

clientdb/order.go

bhandras · 2022-04-06T14:16:16Z

auctioneer/client.go

+		[][]byte, len(o.Details().AllowedNodeIDs),
+	)
+	for idx, nodeID := range o.Details().AllowedNodeIDs {
+		details.AllowedNodeIds[idx] = nodeID[:]


Not sure if this is safe? Since nodeID is restricted to the scope of the for loop.

Good intuition. In Go, the runtime is the one that decides what goes in the stack/heap (the specification does not define it). So, even for pointers, the obj would survive the scope.

I actually ran a little experiment, because I wasn't quite sure myself. PTAL: https://go.dev/play/p/geeXf0C5E6m

ohhh, you are right. The var will out live the loop but all the elements in the slice of []byte will be equal to the last one!

fix: https://go.dev/play/p/hFON1QWCoOY

🙏 Thank you for catching this @bhandras

It's tricky and I think the compiler/linter should be more vocal about this as it's easy to miss.

Oh wow, good catch! I wouldn't have caught that one 😮

guggero

Code looks good to me now!

Though I think we should wait with merging this until we have the server version deployed! Otherwise users will still get matched according to the old rules and be unsure why. Perhaps we should even go ahead and add a new order version for this?

guggero · 2022-04-04T16:23:33Z

cmd/pool/order.go

+		Name: "allowed_node_id",
+		Usage: "the list of nodes this order is able to match with; " +
+			"if empty, the order will be able to match with any " +
+			"node unless not_allowed_node_id is set. Cann be " +


nit: typo in "Cann"

Did you already have the tab for the review open? It looks like it was already fixed

Oh yeah. Old comment, sorry. Please ignore 😅

guggero · 2022-04-12T13:18:52Z

cmd/pool/order.go

+			"Can be specified multiple times",
+	},
+	cli.StringSliceFlag{
+		Name: "not_allowed_node_id",


Prohibited sounds good to me. Or just disallowed. "not allowed" does sound a bit bumpy.

guggero · 2022-04-12T13:19:37Z

cmd/pool/order.go

+			"Can be specified multiple times",
+	},
+	cli.StringSliceFlag{
+		Name: "not_allowed_node_id",


But this probably also requires quite a big refactor/renaming so we can also just leave it the way it is.

lightninglabs-deploy · 2022-04-19T20:17:08Z

@positiveblue, remember to re-request review from reviewers when ready

…_fix subastadb: make etcd prefix queries paginated to avoid timeouts

guggero reviewed Mar 29, 2022

View reviewed changes

positiveblue force-pushed the nodefilters branch 6 times, most recently from 40651e1 to 986929a Compare March 31, 2022 05:53

positiveblue requested a review from guggero March 31, 2022 06:08

guggero approved these changes Apr 4, 2022

View reviewed changes

positiveblue force-pushed the nodefilters branch 2 times, most recently from 0aa913a to 86427e0 Compare April 6, 2022 00:26

positiveblue requested a review from bhandras April 6, 2022 00:42

bhandras approved these changes Apr 6, 2022

View reviewed changes

positiveblue force-pushed the nodefilters branch 2 times, most recently from aba4666 to 53b437d Compare April 7, 2022 15:37

guggero reviewed Apr 12, 2022

View reviewed changes

positiveblue added 7 commits April 22, 2022 00:25

poolrpc: add list of allowed node ids to Order

c85f217

cmd: parse allowed/not allowed nodeids order field

be0e66f

order: populate allowed/not allowed fields

ae5df70

clientdb: add node filter lists to order tlv data

355be15

auctioneerrpc: add node id filters to rpc requests

7361215

auctioneer: populate node id filter lists

1b48f7c

order: check node id filters when validating matches

0c06275

positiveblue force-pushed the nodefilters branch from 53b437d to 0c06275 Compare April 22, 2022 07:27

positiveblue merged commit 6601e0e into master Apr 26, 2022

guggero deleted the nodefilters branch May 5, 2022 15:25

positiveblue pushed a commit to positiveblue/pool that referenced this pull request Oct 11, 2022

Merge pull request lightninglabs#350 from bhandras/sql_mirror_timeout…

2d77c2e

…_fix subastadb: make etcd prefix queries paginated to avoid timeouts

Filter order matches by node id #350

Filter order matches by node id #350

Uh oh!

Conversation

positiveblue commented Mar 28, 2022

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guggero commented Apr 4, 2022

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

positiveblue Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

positiveblue Apr 7, 2022 •

edited

Loading

positiveblue Apr 7, 2022 •

edited

Loading