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

All modified and coverable lines are covered by tests ✅

Project coverage is 89.18%. Comparing base (7188d5b) to head (8080bc0).
Report is 64 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3647      +/-   ##
==========================================
- Coverage   89.19%   89.18%   -0.01%     
==========================================
  Files         155      155              
  Lines      118974   119324     +350     
  Branches   118974   119324     +350     
==========================================
+ Hits       106119   106423     +304     
- Misses      10265    10283      +18     
- Partials     2590     2618      +28     

☔ 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!