Allows using aliases within the config file #103

sr-gi · 2023-09-15T20:29:11Z

The goal of this PR is to allow aliases to be used throughout the config file to identify nodes, so the file is more human-readable. This involves:

Allow aliases to be defined as node identifiers
Allow aliases to be used as source of activities
Allow aliases to be used as destination of activities as long as we control those nodes*
Include both aliases and short versions of public keys when logging activities

* We cannot use aliases as destination for nodes we don't control, given we have no way of mapping the aliases to the nodes' public keys. Notice aliases here do not have to match node aliases, they are just user-defined identifiers, and even if they did, node aliases don't have to be unique.

sr-gi · 2023-09-20T11:55:11Z

@carlaKC @okjodom I've added the changes to allow aliases to be used as ids in the config file (check 8b65670 for the code and rationale).

Looking for an Approach ACK/ tACK of this before moving along with the logging, given the data passed to the simulator to be able to handle logging both PKs and aliases is dependant on how we approach this.

sim-lib/src/lib.rs

okjodom · 2023-09-21T01:45:17Z

sim-lib/src/lnd.rs

+                )));
+                }
+            }
+            crate::NodeId::Alias(a) => alias = a,


Why do we assert that pubkey used as ID must match the node pubkey, but for the alias, we prefer the user provided value, ignoring the actual node alias

I had a conversation w/ @carlaKC regarding this. We argued that the "alias" provided by the user can be anything, and does not really have to match the node alias, but in case it is not defined, we can default to that (tho a warning would be good to have).

I'm happy to go any alternative route

I do like it like this, reasoning being:

Public key is very easily verifiable and unique - if you set this value, you've intentionally fetched it and it should be correct.

Alias is inherently less strictly defined (it's not guaranteed to be unique in our network), so we can take advantage of this looser mapping to have an easier UX (ie, just call it Alice or cln1). When I test things, I often name nodes in my head without setting an alias.

tho a warning would be good to have

this clears up any ambiguity for the users IMO (if they did want it to match).

Also think that this the type of decision where we just do something and then see how it goes in user testing since there's no clear technical "right way".

I've rebased ce036fa and added the warning

okjodom · 2023-09-21T02:15:26Z

sim-lib/src/lib.rs

+            let src_id = payment_flow
+                .source
+                .get_pk()
+                .map_err(LightningError::ValidationError)?;


just realized these should be SimulationError::ValidationError

Agreed, tho that does not exist, we may want to move it.

okjodom · 2023-09-21T02:19:42Z

sim-lib/src/lib.rs

+        if let NodeId::PublicKey(pk) = self {
+            Ok(pk)
+        } else {
+            panic!("NodeId is not a PublicKey. This should never happen, please report")


maybe we could just return a SimulationError. No need to panic :)

We can message this as "NodeId is not a PublicKey. Please prefer to use actual node pubkey for the node aliased as {}".

Any dependent on sim-lib who passes in activity definition with aliases knows what to change, instead of reporting (?)

Ups, I forgot to change that to an error.

Regarding encouraging one or the other, I don't think we should. I'm happy with getting rid of the reporting stuff if we think of this as something that can be used by a third party, but we actually don't care whether they use one variant or the other, we cast everything to PK internally

okjodom · 2023-09-21T02:21:51Z

sim-lib/src/lib.rs

@@ -383,8 +418,8 @@ impl Simulation {
        for (id, node) in self.nodes.iter().filter(|(pk, _)| {
            self.activity
                .iter()
-                .map(|a| a.source)
-                .collect::<HashSet<PublicKey>>()
+                .map(|a| a.source.get_pk().unwrap())


okay but can we avoid the assumption of having called validate_activity before this point?
Same sentiment for the other get_pk.unwrap() points

Can we? Sure, but I don't think we should. validate_activity is our sanity checker, we could modify all these calls to account for an alias being here, but that's exactly what is not supposed to happen. If any NodeId contains an alias at this point, it means the code is wrong (hence why there was a "report message").

I could patch the code so it does not unwrap and it fails more "elegantly", but all in all we want it to fail if that happens

Alternatively, we could have two different classes to account for node definition and activity definition, one that interfaces with the config file, and where we allow aliases and pks (data is parsed as NodeId), and an internal one that gets constructed from the former, and only accepts pks. In that way, we can be sure at compile time that everything is identified by pk internally.

where we allow aliases and pks (data is parsed as NodeId), and an internal one that gets constructed from the former, and only accepts pks.

I think that this has nice layering separation - the simulation library only really cares about public keys, and allowing user-friendly parsing of pubkey/alias is a CLI responsibility/feature.

I've split this into two sets of data structures in 75795ca. The Parsers that interact directly with the config file and allow NodeIds to be loaded, and the regular ones which roll back to use PublicKeys and can be built from the former.

This commit can be squashed with the previous one. Also, I'm happy to replace Parser with any other better term we can come up with (Loader, Reader, ...)

okjodom · 2023-09-21T02:41:23Z

cACK on e2d5fb3 allowing aliases as source or destination.

we internally identify activities using PK, so we do an additional mapping
step once the data is loaded to go from NodeId{PK, Alias} to NodeId(PK), hence, once data
is passed to the simulator it is always identified by NodeId(PK)

Sounds to me like a specific feature offering of sim-cli, or any other UI we build on top of sim-lib

after Simulation::validate_activity it is safe to call NodeId::get_pk::unwrap(),
given no NodeId::Alias should have passed validation.

I have strong reactions against this. Had suggestions to better communicate the error case and avoid the panic, but that doesn't seem sufficient.

carlaKC

Concept ACK, with a slight preference to shift some handling to the cli as mentioned here.

carlaKC · 2023-09-22T14:49:37Z

sim-lib/src/cln.rs

+            crate::NodeId::PublicKey(pk) => {
+                if pk != node_id {
+                    return Err(LightningError::ValidationError(format!(
+                    "The provided node id does not match the one returned by the backend ({} != {})",


nit: full stop / lower case for errors?

carlaKC · 2023-09-22T14:53:00Z

sim-lib/src/lnd.rs

+                )));
+                }
+            }
+            crate::NodeId::Alias(a) => alias = a,


I do like it like this, reasoning being:

Public key is very easily verifiable and unique - if you set this value, you've intentionally fetched it and it should be correct.

Alias is inherently less strictly defined (it's not guaranteed to be unique in our network), so we can take advantage of this looser mapping to have an easier UX (ie, just call it Alice or cln1). When I test things, I often name nodes in my head without setting an alias.

tho a warning would be good to have

this clears up any ambiguity for the users IMO (if they did want it to match).

Also think that this the type of decision where we just do something and then see how it goes in user testing since there's no clear technical "right way".

sim-cli/src/main.rs

carlaKC · 2023-09-22T14:59:43Z

sim-lib/src/lib.rs

@@ -383,8 +418,8 @@ impl Simulation {
        for (id, node) in self.nodes.iter().filter(|(pk, _)| {
            self.activity
                .iter()
-                .map(|a| a.source)
-                .collect::<HashSet<PublicKey>>()
+                .map(|a| a.source.get_pk().unwrap())


where we allow aliases and pks (data is parsed as NodeId), and an internal one that gets constructed from the former, and only accepts pks.

I think that this has nice layering separation - the simulation library only really cares about public keys, and allowing user-friendly parsing of pubkey/alias is a CLI responsibility/feature.

carlaKC · 2023-09-22T15:01:09Z

And sorry or the rebase conflicts :')

carlaKC

Just done a high level read through of the new approach, cACK!

sr-gi · 2023-09-27T14:32:51Z

I've added the last bit here. Notice how the last commit is pretty opinionated. I'm happy to re-visit the approach or even changing it as long as it does not end up being super messy (which has been my experience when trying to go for some of the alternatives).

sr-gi · 2023-10-02T20:48:51Z

Addressed the latest comments from @carlaKC plus the pending ones from the last review.

The issue with the repeated source/destination in an activity should be fixed now

okjodom

change read through, lgtm

sr-gi · 2023-10-03T15:02:30Z

Only major comment is allowing multiple activity descriptions for the same source (last commit). Really happy with the way everything else turned out.

Tested with the following config to sanity check that we do actually fail with repeated sources:

{
    "nodes": [
        {
            "LND": {
                "id": "alice",
                "address": "https://localhost:10011",
                "macaroon": "~/alice/admin.macaroon",
                "cert": "~/alice/tls.cert"
            }
        },
		{
            "LND": {
                "id": "dave",
                "address": "https://localhost:10014",
                "macaroon": "~/dave/admin.macaroon",
                "cert": "~/dave/tls.cert"
            }
        }
],
	"activity": [
		{
			"source": "alice",
			"destination": "03fd1854561cfd7e3b58714f4f8c597bba29428100352b207c0d2baef832bdf390",
			"interval_secs": 2,
			"amount_msat": 100
		},
		{
			"source": "alice",
			"destination": "0374f63bfb1a0e9607b05ac6a7fb5393216e6ffcdf80700ec6da7131815249881c",
			"interval_secs": 5,
			"amount_msat": 1000
		},
		{
			"source": "dave",
			"destination": "alice",
			"interval_secs": 2,
			"amount_msat": 100
		}

	]
}

-> Error: Config validation failed: unknown activity source: alice.

tested with a similar setup to this one, can confirm it does not crash anymore

sr-gi · 2023-10-03T19:11:12Z

FYI #120 needs to go in before this, given otherwise we cannot rebase main without setting the network field for nodes we don't control to something meaningful.

carlaKC · 2023-10-03T20:05:19Z

tACK f17662d pending rebase on #120.

Tested out various scenarios and everything is looking great. When I test this PR the easy config makes me so happy 🎆

Duplicate alias: Error: Config validation failed: duplicated node: alice.
Alias mismatch warning: WARN [sim_lib] The provided alias does not match the one returned by the backend (cln != BLUESQUIRREL-v23.05-175-g910630c)
Bad pubkey: Error: Config validation failed: the provided node id does not match the one returned by the backend (0255b34764dbe7a9d5ad4e1fd7fb069bb7992128cd55467e4db50796b318a9bc9e != 037359ab1279636979218125289e5abf832c1809cb80bbb33d8982e95e597d8127).
Unknown destination alias: Error: Config validation failed: unknown activity destination: loser

Also tested out positive paths of:

Alias/Pubkey -> Alias/Pubkey.
Multiple activities for a single source.

Node creation logic was mostly duplicated, creates a node based on LightningNode instead of on its implementations to reduce the code replication. I'd be nice the be able to do this with a different container than Arc<Mutex<T>>, since that will save us from having to acquire the mutex after creating `node`. However, I don't know of any other container that is is send and which size is known at compile time. We may need to ask this to some elder rust wizards

…e file

Crates a new enum, `NodeId`, which is used to parse the node identifier from the config file to either a `PublicKey` or an `Alias`. Internally, we will identify nodes as `PublicKey`s, so we add do further checks when creating {Cln, Lnd}Connection. Moreover, we make sure that the provided variant matches the one from the backend. In case of a `PublicKey`, if the assertion fails the code will return a `ValidationError`. In the case of an `Alias`, it will simply log a warning. The rationale for not failing if the provided alias does not match the one from the backend is that we want to allow user to identify nodes using whatever name they rather (alice/bob/carol/..., n1, n2, n3, ...), so there is not strong requirement for aliases to match (but it feels right to let the user know, just in case). Finally, if an identifier is used more than once, the simulation will also fail with a `ValidationError`, independently of the identifier variant.

sr-gi · 2023-10-05T18:40:07Z

Sorry, rebasing this took me longer than I wanted to. I was trying to move the validated_activities creation inside the Simulator so the validation happens internally but it has proven to be a pretty nasty change that involves passing along multiple maps and making validate_activity pretty messy.

I think keeping the Simulator interface simple is one of the design goals, and the way we personally parse information from our config in the CLI should not interfere with it, so I decided not to go for the change, at least for now.

I've squeezed a fix for #126 in 5c5497d since being able to check that depended on having NodeInfo around.

carlaKC

Only comment is on the final commit adding validation.

Would be happy to drop that off of this and open it in a separate PR so that we can hit the button on this one.

sim-lib/src/lib.rs

carlaKC · 2023-10-05T19:33:34Z

sim-lib/src/lib.rs

@@ -815,32 +815,37 @@ async fn produce_random_events<N: NetworkGenerator, A: PaymentGenerator + Displa
            // Wait until our time to next payment has elapsed then execute a random amount payment to a random
            // destination.
            _ = time::sleep(wait) => {
-                let destination = network_generator.lock().await.sample_node_by_capacity(source.pubkey);
+                let (destination, capacity) = network_generator.lock().await.sample_node_by_capacity(source.pubkey);


I would have thought we want to do this validation much higher up?

Like nodes that don't have enough capacity, if a node doesn't support keysend when we start running it never will so we should just exclude it from the start (and log a warning so the user knows why).

If we do this validation here, we could also theoretically end up effectively running with one node (in the case of two nodes and one doesn't have keysend) and then just run without ever sending a payment.

We can run this much higher up, as long as we are ok with compromising with --accept-keysend being enabled for both source and destination nodes. Otherwise, we cannot know if keysend receiving is really going to be necessary until a node is picked as a payment destination, and that happens on the fly.

compromising with --accept-keysend being enabled for both source and destination nodes.

In the case of random activity I think that's okay because every sending node can reasonably be expected to also receive?

Fair enough. 279ad56 should have fixed that

The approach followed for this is pretty straight-forward: instead of loading `PublicKey`s directly to our `ActivityDefinition` data structure, we create an intermediary class `ActivityParser` that accepts `NodeId`. However, we internally identify activities using `PublicKey`s, so we do an additional mapping step once the data is loaded to go from `ActivityParser` to `ActivityDefinition`, hence, once data is passed to the simulator it is always identified by `PublicKey`. For activity destinations, we also need to make sure that if an alias has been provided we do control that node, otherwise the Alias->PublicKey mapping cannot be done, in which case we fail with a `ValidationError`.

This commit redefines what data about a node is passed along to the simulator on activities. Previously, we used to pass only the src/dst `PublicKey`. However, by doing this we had no way of logging (nor accessing) any other information about them. This is mainly because all this mapping is only available on `sim-cli::main.rs`, and also because the data is mostly logged by the producers/consumers functions, which are not even part or the `Simulator` class. Therefore, the alternative we are left with are passing the information to the simulator. This can be done in several ways, I've gone with the one that has a better balance between codediff and usefulness, which is passing `NodeInfo` along. This, however, means having to obtain the destination node features on `main` instead of on `validation`.

carlaKC

🐑 it !

sr-gi · 2023-10-05T20:23:55Z

Let's go 👶 !

sr-gi force-pushed the 2023-09-allow-aliases-config branch 4 times, most recently from 47d831d to e2d5fb3 Compare September 20, 2023 12:49

carlaKC self-requested a review September 20, 2023 20:39

okjodom reviewed Sep 21, 2023

View reviewed changes

sim-lib/src/lib.rs Outdated Show resolved Hide resolved

okjodom reviewed Sep 21, 2023

View reviewed changes

sr-gi force-pushed the 2023-09-allow-aliases-config branch 2 times, most recently from 0cc7e8d to a85ab1e Compare September 21, 2023 10:58

carlaKC reviewed Sep 22, 2023

View reviewed changes

sr-gi force-pushed the 2023-09-allow-aliases-config branch 4 times, most recently from 32e66a7 to 75795ca Compare September 23, 2023 15:38

sr-gi requested review from okjodom and carlaKC September 25, 2023 14:31

carlaKC reviewed Sep 25, 2023

View reviewed changes

sr-gi force-pushed the 2023-09-allow-aliases-config branch 2 times, most recently from 05a0056 to 3cc59e2 Compare September 26, 2023 15:06

sr-gi marked this pull request as ready for review September 27, 2023 14:30

sr-gi force-pushed the 2023-09-allow-aliases-config branch 3 times, most recently from 0ab5b75 to ab7ddf8 Compare September 27, 2023 15:01

okjodom reviewed Oct 3, 2023

View reviewed changes

This was referenced Oct 3, 2023

chore: validate node network and disallow mainnet #112

Merged

sim-lib: Adds LightningNode::get_network #120

Merged

sr-gi added 2 commits October 4, 2023 14:43

sim-lib: refactors lib to place connection info to its respective nod…

204772a

…e file

carlaKC mentioned this pull request Oct 4, 2023

Add Random Activity to README and CLI Options #125

Merged

sr-gi mentioned this pull request Oct 5, 2023

Validate random activity peers support keysend #126

Closed

sr-gi force-pushed the 2023-09-allow-aliases-config branch 2 times, most recently from 3710351 to fe1690a Compare October 5, 2023 15:15

sr-gi force-pushed the 2023-09-allow-aliases-config branch 4 times, most recently from ee0dd4e to 5c5497d Compare October 5, 2023 18:33

sr-gi requested review from okjodom and carlaKC October 5, 2023 18:40

carlaKC reviewed Oct 5, 2023

View reviewed changes

sr-gi added 2 commits October 5, 2023 15:52

sr-gi force-pushed the 2023-09-allow-aliases-config branch from 5c5497d to 98c15d0 Compare October 5, 2023 19:55

sim-lib: ensures destination supports keysend for random payments

279ad56

sr-gi force-pushed the 2023-09-allow-aliases-config branch from 98c15d0 to 279ad56 Compare October 5, 2023 20:13

carlaKC approved these changes Oct 5, 2023

View reviewed changes

sr-gi merged commit 55d41ff into bitcoin-dev-project:main Oct 5, 2023
2 checks passed

Allows using aliases within the config file #103

Allows using aliases within the config file #103

Conversation

sr-gi commented Sep 15, 2023 • edited Loading

sr-gi commented Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

sr-gi Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

okjodom commented Sep 21, 2023

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Sep 22, 2023

carlaKC left a comment

Choose a reason for hiding this comment

sr-gi commented Sep 27, 2023 • edited Loading

sr-gi commented Oct 2, 2023

okjodom left a comment

Choose a reason for hiding this comment

sr-gi commented Oct 3, 2023

sr-gi commented Oct 3, 2023

carlaKC commented Oct 3, 2023

sr-gi commented Oct 5, 2023 • edited Loading

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr-gi Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

sr-gi commented Oct 5, 2023

sr-gi commented Sep 15, 2023 •

edited

Loading

sr-gi commented Sep 20, 2023 •

edited

Loading

sr-gi Sep 21, 2023 •

edited

Loading

sr-gi Sep 23, 2023 •

edited

Loading

sr-gi Sep 21, 2023 •

edited

Loading

sr-gi Sep 23, 2023 •

edited

Loading

sr-gi commented Sep 27, 2023 •

edited

Loading

sr-gi commented Oct 5, 2023 •

edited

Loading

sr-gi Oct 5, 2023 •

edited

Loading