Add deterministic random bytes in override_random_bytes

[moisesPompilio]

Fixes #2827

To ensure deterministic override_random_bytes for each node, I first reverted to the state before commit a3b416a and used keys_manager.get_secure_random_bytes() to retrieve the required values. Then, I restored the current state of the code and assigned the collected values to their respective nodes.

This is my first PR for LDK, so I’d appreciate any feedback on my approach and possible improvements.

[ldk-reviews-bot]

👋 I see @valentinewallace was un-assigned.
If you'd like another reviewer assignemnt, please click here.

[codecov]

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (ebdbee0) to head (f8d1126).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/functional_test_utils.rs	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3647      +/-   ##
==========================================
- Coverage   89.25%   89.23%   -0.03%     
==========================================
  Files         155      155              
  Lines      119954   119952       -2     
  Branches   119954   119952       -2     
==========================================
- Hits       107064   107034      -30     
- Misses      10277    10292      +15     
- Partials     2613     2626      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

I think post-#3302 we should focus on making the test robust in that it doesn't rely on the output of the RNG, rather than trying to fix the RNG output (which #3302 largely already did).

[moisesPompilio]

Ok, thanks for the feedback! Quick question—would it make more sense to reduce the RNG dependency just enough so override_random_bytes doesn’t break the test so easily, or should I focus on keeping override_random_bytes and minimizing the test’s reliance on RNG? Sorry if I’m being redundant with my question, still getting familiar with the project.

[TheBlueMatt]

In an ideal world I think we can drop the override_random_bytes and also the set_counter lines above. I think that would be the goal to close the issue.

[moisesPompilio]

Got it! I'll work on these removals and see if I can make it work.

[moisesPompilio]

Hello @TheBlueMatt,

I updated the code as suggested. I removed the set_counter call from do_test_restored_packages_retry and changed serialized_monitor to match the node under test. To do this, I reverted to the code state before commit a3b416a, adjusted the override_random_bytes to reflect the current implementation, and used get_monitor to obtain the updated serialized_monitor value.

Could you please confirm if this reasoning is correct or if there's anything else needed?

[TheBlueMatt]

Doh! When I did #3302 and commented above I had not actually bothered to look into why the test needed specific RNG output. Having to have the RNG output match the hard-coded ChannelMonitor is really the annoying part here. Ideally we'd have a way to override the keys selected themselves so that we're not relying on the RNG at all. I think that would mean having a variant to create_chanmon_cfgs which allows us to specify the node's seed and then hooking TestKeysInterface::generate_channel_keys_id to let us specify the next keys-id that will be used so that it matches the one we want.

[moisesPompilio]

Thanks a lot! I saw your comment—I was trying to tackle the RNG issue in a different way, but I realized I was way off and getting stuck. I had already gone back and modified serialized_monitor, but I started questioning if I was just overcomplicating things. I'll start making the suggested changes now. Appreciate the guidance!

[moisesPompilio]

Hi @TheBlueMatt

I've implemented the requested changes. I removed set_account and introduced override_next_keys_id, allowing generate_channel_keys_id to return a predefined value if set. Otherwise, it follows the normal flow. The override resets to None after use to prevent duplicate IDs.

I also added create_chanmon_cfgs_with_keys, which works like create_chanmon_cfgs but allows predefined keys_id values to be set in chan_mon_cfgs, ensuring predictable key generation when needed. Additionally, I used set_counter to track generate_channel_keys_id outputs, preserving the original keys_id values and avoiding changes to serialized_monitor.

I believe the test now relies less on RNG manipulation.

[TheBlueMatt]

Hell yea, thanks! Just gonna land this since its test-only and straightforward.