refactor: Unify Dispatch of Random and Defined Activity #160

carlaKC · 2023-11-17T21:19:10Z

This PR unifies dispatch of random and defined activities. I think that this is worthwhile doing:

As a pathway to allowing users to specify both random and defined activity in future changes
To reduce code duplication (functionality is similar in produce_events and produce_random_events)
Possibly provide a different impl of things like sim-lib: Allows amount and interval ranges in manual activity definition #153 (though the existing one is quite simple so this is a weak motivation)

Opening up in draft for an early look (still needs some cleanup). This is my first foray into trait objects, so this PR only exists by the good graces of crust of rust / suggestions for more canonical rust ways of doing this are very welcome!

okjodom · 2023-11-22T08:03:44Z

Concept ack ✅ And yes the Rust looks crusty!
Will take a closer look and test

sr-gi · 2023-12-04T16:31:05Z

I'll be reviewing the PR this week, sorry for taking so long

sr-gi

Approack ACK. There are some simplifications that we can discuss once the code is cleaned up.

There is only one comment I have regarding the approach. Check inline.

sim-lib/src/lib.rs

This trait is used to pick destinations for sending payments, so we rename it to more accurately reflect this.

In preparation for different implementations of this trait, we rename sample_node_by_capacity to a more general choose_destination to accommodate different ways of picking destinations.

…ivity In preparation for other types of payment activity generators, rename random implementation to be more specific.

carlaKC · 2024-01-19T15:33:19Z

Ready for another review with all the proposed name changed, apologies for the long turnaround!

@enigbe this has quite a few renames / refactors in it, so will be worthwhile for you to take a pass while you're getting familiar with the codebase.

sim-lib/src/lib.rs

okjodom

tACK 3ac677c ✅

Left stylistic comments on trait declarations, otherwise the change looks good to me
Here are the suggested patches
dyn-traits-patch.txt

carlaKC · 2024-01-24T17:09:02Z

dyn-traits-patch.txt

Whoop, just saw this now. Added a commit that does the same for LightningNode + Send!

okjodom

looks good!

sr-gi · 2024-01-24T21:33:45Z

Just dropping by to acknowledge that I owe a review here, will do it ASAP (hopefully this week)

sr-gi

LGTM, I left some comments but mostly about rustlang idiomatics, the logic looks sound.

sr-gi · 2024-01-25T21:11:31Z

.gitignore

nit: remove empty trailing line

sr-gi · 2024-01-25T21:24:44Z

sim-lib/src/lib.rs

+pub trait DestinationGenerator: Send {
+    // choose_destination picks a destination node within the network, returning the node's information and its
+    // capacity (if available).
+    fn choose_destination(&self, source: PublicKey) -> (NodeInfo, Option<u64>);
 }

 #[derive(Debug, Error)]
 #[error("Payment generation error: {0}")]
 pub struct PaymentGenerationError(String);
-pub trait PaymentGenerator {
+
+pub trait PaymentGenerator: Display + Send {
    // Returns the number of seconds that a node should wait until firing its next payment.
    fn next_payment_wait(&self) -> time::Duration;

-    // Returns a payment amount based on the capacity of the sending and receiving node.
-    fn payment_amount(&self, destination_capacity: u64) -> Result<u64, PaymentGenerationError>;
+    // Returns a payment amount based, with a destination capacity optionally provided to inform the amount picked.
+    fn payment_amount(
+        &self,
+        destination_capacity: Option<u64>,
+    ) -> Result<u64, PaymentGenerationError>;


Aren't all these supposed to be docs? (///)

Codebase is a little all over the place, going to add a commit to fix all the // -> /// in one go.
70ca4ba

sim-lib/src/lib.rs

sim-lib/src/random_activity.rs

sim-lib/src/defined_activity.rs

sr-gi · 2024-01-26T16:18:59Z

sim-lib/src/defined_activity.rs

+impl DestinationGenerator for DefinedPaymentActivity {
+    fn choose_destination(
+        &self,
+        _source: bitcoin::secp256k1::PublicKey,


nit: this could be just _: bitcoin::secp256k1::PublicKey

Is just a _ idiomatic rust, out of interest?
(old golang habits die hard)

Ummm, not sure if I picked this up from rustlang or where if I'm honest

sim-lib/src/defined_activity.rs

sim-lib/src/lib.rs

In preparation for implementing our generation traits for defined activities, we loosen the tie to destination capacity in our trait definition (which is a characteristic of the random activity generation implementation).

sr-gi

ACK 75ac437

carlaKC requested review from sr-gi and okjodom November 21, 2023 14:03

sr-gi reviewed Dec 5, 2023

View reviewed changes

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

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

carlaKC force-pushed the refactor-dispatch branch from dc176bd to 3aaaf6b Compare January 19, 2024 15:10

carlaKC added 3 commits January 19, 2024 10:25

sim-lib/refactor: rename NetworkGenerator to DestinationGenerator

d7e7900

This trait is used to pick destinations for sending payments, so we rename it to more accurately reflect this.

sim-lib/refactor: generalize naming of sample_node_by_capacity

7f31747

In preparation for different implementations of this trait, we rename sample_node_by_capacity to a more general choose_destination to accommodate different ways of picking destinations.

sim-lib/refactor: rename PaymentActivityGenerator to RandomPaymentAct…

734b799

…ivity In preparation for other types of payment activity generators, rename random implementation to be more specific.

carlaKC force-pushed the refactor-dispatch branch 2 times, most recently from df00c4e to 3ac677c Compare January 19, 2024 15:28

carlaKC marked this pull request as ready for review January 19, 2024 15:29

carlaKC requested a review from sr-gi January 19, 2024 15:39

okjodom reviewed Jan 22, 2024

View reviewed changes

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

okjodom reviewed Jan 22, 2024

View reviewed changes

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

okjodom approved these changes Jan 22, 2024

View reviewed changes

enigbe approved these changes Jan 24, 2024

View reviewed changes

carlaKC force-pushed the refactor-dispatch branch 2 times, most recently from ceaeec0 to 9ff717b Compare January 24, 2024 17:08

okjodom approved these changes Jan 24, 2024

View reviewed changes

sr-gi reviewed Jan 26, 2024

View reviewed changes

carlaKC added 2 commits January 30, 2024 08:56

sim-lib: allow optional payment amount in DestinationGenerator

db8220e

In preparation for implementing our generation traits for defined activities, we loosen the tie to destination capacity in our trait definition (which is a characteristic of the random activity generation implementation).

sim-lib/refactor: move create nodes function into test utils

1565793

carlaKC force-pushed the refactor-dispatch branch 2 times, most recently from 9aab9e1 to 3e85c5a Compare January 30, 2024 14:34

carlaKC added 2 commits January 30, 2024 09:41

sim-lib: add generator implementations for defined activities

9eaf420

sim-lib: unify dispatch of events across random and defined activity

ac54d1c

carlaKC added 5 commits January 30, 2024 09:41

sim-lib/refactor: fix naming of consumer channels for clarity

7510bbc

sim-lib/refactor: use single dispatch for random and define activities

01c31ec

multi: include Send in LightningNode trait as supertrait

c4a6fa9

multi: once off documentation comment sweep

5944847

gitingore: ignore any json file to accommodate different sim file names

75ac437

carlaKC force-pushed the refactor-dispatch branch from 3e85c5a to 75ac437 Compare January 30, 2024 14:42

carlaKC requested a review from sr-gi January 30, 2024 14:47

sr-gi reviewed Feb 1, 2024

View reviewed changes

carlaKC merged commit 69813aa into bitcoin-dev-project:main Feb 2, 2024
2 checks passed

This was referenced Feb 2, 2024

refactor: use listen and trigger universally #164

Merged

feat: add simulation config #166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Unify Dispatch of Random and Defined Activity #160

refactor: Unify Dispatch of Random and Defined Activity #160

carlaKC commented Nov 17, 2023

okjodom commented Nov 22, 2023

sr-gi commented Dec 4, 2023 •

edited

Loading

sr-gi left a comment

carlaKC commented Jan 19, 2024

okjodom left a comment •

edited

Loading

carlaKC commented Jan 24, 2024

okjodom left a comment

sr-gi commented Jan 24, 2024

sr-gi left a comment

sr-gi Jan 25, 2024

sr-gi Jan 25, 2024

carlaKC Jan 30, 2024 •

edited

Loading

sr-gi Jan 26, 2024

carlaKC Jan 30, 2024

sr-gi Feb 1, 2024

sr-gi left a comment

refactor: Unify Dispatch of Random and Defined Activity #160

refactor: Unify Dispatch of Random and Defined Activity #160

Conversation

carlaKC commented Nov 17, 2023

okjodom commented Nov 22, 2023

sr-gi commented Dec 4, 2023 • edited Loading

sr-gi left a comment

Choose a reason for hiding this comment

carlaKC commented Jan 19, 2024

okjodom left a comment • edited Loading

Choose a reason for hiding this comment

carlaKC commented Jan 24, 2024

okjodom left a comment

Choose a reason for hiding this comment

sr-gi commented Jan 24, 2024

sr-gi left a comment

Choose a reason for hiding this comment

sr-gi Jan 25, 2024

Choose a reason for hiding this comment

sr-gi Jan 25, 2024

Choose a reason for hiding this comment

carlaKC Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

sr-gi Jan 26, 2024

Choose a reason for hiding this comment

carlaKC Jan 30, 2024

Choose a reason for hiding this comment

sr-gi Feb 1, 2024

Choose a reason for hiding this comment

sr-gi left a comment

Choose a reason for hiding this comment

sr-gi commented Dec 4, 2023 •

edited

Loading

okjodom left a comment •

edited

Loading

carlaKC Jan 30, 2024 •

edited

Loading