-
Notifications
You must be signed in to change notification settings - Fork 37
Implement Distinct RNG Sequences #259
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
base: main
Are you sure you want to change the base?
Conversation
61929d6
to
cfd394a
Compare
simln-lib/src/lib.rs
Outdated
pub fn salted(&self, pubkey: &PublicKey) -> Self { | ||
// Get the pubkey bytes | ||
let pubkey_bytes = pubkey.serialize(); | ||
|
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.
Would appreciate thoughts on this approach to salting
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 asked the ai gods and they suggested just using a straight-up hash:
let mut hasher = DefaultHasher::new();
source.pubkey.hash(&mut hasher);
let salt = hasher.finish();
Since we only care about determinism here (not about salt that provides good entropy), I don't think it matters much which approach we use provided they yield distinct values per pubkey.
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.
Ah okay, we don't care to salt the base seed, right? As in, it's okay for the rng to be something like: Self(Arc::new(StdMutex::new(ChaCha8Rng::seed_from_u64(salt))))
or do we want to do something like base_seed ^ salt
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 that we do want to include the base seed here just so that there's an option for folks to run with different, fixed seeds? If we just use pubkey, we'll only ever get one set of payments per pubkey, vs if we include the base seed we can change it (while keeping it deterministic)
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.
Took another stab at this one, in this case I just concatenated the seed with the pubkey and hashed that, which should still give us different but deterministic payments
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.
Great start on this!
simln-lib/src/lib.rs
Outdated
struct MutRng(MutRngType); | ||
struct MutRng { | ||
rng: MutRngType, | ||
seed: Option<u64>, |
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.
Storing this so that we can split up rng = new()
and rng.salted
isn't very appealing to me - if we only need a field to instantiate the object, maybe we're instantiating it wrong.
I think it would be okay to have:
new
: as is, used to createMutRng
get_salted_seed(u64, pubkey) => u64
: a helper to generate the seed
The caller is responsible for setting up their own saltyseed and then calling new
, but I think that's fine
simln-lib/src/lib.rs
Outdated
// Get the pubkey bytes | ||
let pubkey_bytes = pubkey.serialize(); |
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.
nit: unnecessary comment (the code is so simple it's self-documenting IMO)
simln-lib/src/lib.rs
Outdated
pub fn salted(&self, pubkey: &PublicKey) -> Self { | ||
// Get the pubkey bytes | ||
let pubkey_bytes = pubkey.serialize(); | ||
|
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 asked the ai gods and they suggested just using a straight-up hash:
let mut hasher = DefaultHasher::new();
source.pubkey.hash(&mut hasher);
let salt = hasher.finish();
Since we only care about determinism here (not about salt that provides good entropy), I don't think it matters much which approach we use provided they yield distinct values per pubkey.
simln-lib/src/lib.rs
Outdated
fn create_salted_mut_rng() { | ||
let base_rng = MutRng::new(Some(42)); | ||
|
||
let pk1 = PublicKey::from_slice(&[ |
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.
While it seems a bit deranged to use random values in a test about determinism, I think we can use get_random_keypair
here to get different pubkeys on each run.
It should never fail for valid pubkeys, so we're basically fuzzing this incredibly slowly.
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.
Ah thanks for this! I was looking for a function like this! I was going to use this but didn't want to unecessarily add to Cargo.toml.
simln-lib/src/lib.rs
Outdated
// Create a salted RNG for this node based on its pubkey | ||
let salted_rng = self.cfg.seeded_rng.salted(&node_info.pubkey); |
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.
Ah, we already have our RNG
in SimulationConfig
which is why we need salted
, got it!
Let's do a refactor commit before this one that provides the seed in SimulationConfig
and then we can set up our own RNGs however we like internally. The caller already just passes in the seed and lets us handle it so I think it's fine to change.
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.
Just to clarify we want to remove seeded_rng
from the SimulationCfg and replace it with the seed, then generate the rng inside random_acitivity_nodes
?
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.
Yeah that's what I was thinking - that way we've got the seed available to create our salted RNGs and don't need to store it in the RNG itself.
simln-lib/src/lib.rs
Outdated
@@ -946,6 +959,8 @@ impl Simulation { | |||
); | |||
|
|||
for (node_info, capacity) in active_nodes.values() { | |||
// Create a salted RNG for this node based on its pubkey | |||
let salted_rng = base_rng.salted(&node_info.pubkey, self.cfg.seed.unwrap_or(0)); |
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 don't think that this does what we want in the case where seed
is None
? In this case we want totally random behavior, and this will always seed with pubkey + 0
because of the unwrap.
I think that some good options here would be:
- Move all this abstraction into the
new
function ofMutRng
pub fn new(seed_opt: Option<(u64, Option<Pubkey>)>) -> Self
We'd invoke this differently based on use-case:
let base_seed = self.cfg.seed.map(|s| Some((s, None)));
base_rng = new(self.cfg.seed, None);
let seed_material = self.cfg.seed.map(|s| Some((s, Some(&node_info.pubkey))));
salted_rng = new(seed_material)
- Match on presence of seed
let salted_rng = if let Some(s) = self.cfg.seed {
MutRng::new(None)
} else {
MutRng::salted(&node_info.pubkey, s)
}
I think that I prefer (1), but open to other approaches.
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 gave this another go by consolidating the salted functionality into new.
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.
One last comment here - really close!
simln-lib/src/lib.rs
Outdated
pub fn new(seed_opt: Option<u64>, pubkey: Option<&PublicKey>) -> Self { | ||
let seed = if let Some(seed) = seed_opt { | ||
if let Some(pk) = pubkey { | ||
let mut hasher = DefaultHasher::new(); | ||
let mut combined = pk.serialize().to_vec(); | ||
combined.extend_from_slice(&seed.to_le_bytes()); | ||
combined.hash(&mut hasher); | ||
hasher.finish() | ||
} else { | ||
seed | ||
} |
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 would prefer to go with the nested options for seed_opt
/ pubkey
because there is no valid case where you provide a pubkey
but not a seed
.
The handling isn't so bad if we get fancy with matching:
pub fn new(seed_opt: Option<(u64, Option<&PublicKey>)>) -> Self {
let seed = match seed_opt {
Some(( seed, Some(pubkey) )) => {
let mut hasher = DefaultHasher::new();
let mut combined = pk.serialize().to_vec();
combined.extend_from_slice(&seed.to_le_bytes());
combined.hash(&mut hasher);
hasher.finish()
},
Some(( seed, None )) => seed,
None => rand::random(),
}
Self(Arc::new(StdMutex::new(ChaCha8Rng::seed_from_u64(seed))))
}
e56bd72
to
7b47f4f
Compare
Overview
This PR introduces a deterministic yet unique random number generation system for the Lightning Network simulation. Each
NetworkGraphView
now maintains a base seed, while individual nodes receive their own RNG instances derived from this base seed and salted with their public keys. This ensures consistent but varied behavior across the network while preserving reproducibility.Changes
MutRng
structrandom_activity_nodes
to use salted RNGs for each node