update report::hybrid tests to use TestWorld::rng

[eriktaubeneck]

small refactor to use

let world = TestWorld::default();
let mut rng = world.rng();

instead of

let mut rng = thread_rng();

in report::hybrid.rs tests.

[codecov]

Codecov Report

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

Project coverage is 93.42%. Comparing base (7f46319) to head (2a7c247).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
ipa-core/src/report/hybrid.rs	99.48%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1428      +/-   ##
==========================================
+ Coverage   93.39%   93.42%   +0.03%     
==========================================
  Files         225      227       +2     
  Lines       38979    39162     +183     
==========================================
+ Hits        36403    36589     +186     
+ Misses       2576     2573       -3     

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

[andyleiserson]

I'm curious why this was required.

On the one hand, it makes sense that we might need this bound if we pass the test RNG to implementation routines that call for a CryptoRng.

On the other hand, seeding an RNG with a 64-bit seed (which is not happening in this function, but I think is happening for the root RNG) is not sound for cryptographic purposes, so it's arguably wrong to declare that this returns a CryptoRng.

It's probably at least worth a comment clarifying the significance of the bound.

[eriktaubeneck]

The encrypt method on HybridReport and KeyRegistry::<KeyPair>::random both require this bound. In the past with IPA, we just used thread_rng (which seems to work).

It seems like this is seeded from another rng, which is seeded from the fixed value. I'm not sure exactly how many bytes it pulls to seed this rng...

All that said, this is obviously not sound for cryptographic purposes because it has a fixed seed from a config file. I can add a comment, but I suppose the only real alternative would be to not use TestWorld::rng for crypto stuff, and that doesn't seem like the right outcome.

[andyleiserson]

Yeah, I agree with that assessment.

[andyleiserson]

it has a fixed seed from a config file

Not sure what you mean by that. TestWorld generates a seed at random each time it is constructed. The purpose of the seeding is to enable reproducing a failure after the fact by manually specifying the seed from the failure. Otherwise, the seed is different run-to-run, which gives us more coverage at the possible cost of some CI flakiness. If specific tests are problematic in this regard, they need to use TestWorld::with_seed instead of TestWorld::default to have the same seed on every run.

[akoshelev]

@eriktaubeneck Can you please also make the same comment in the function doc? I agree it is important to make it crystal clear that it is not a crypto-safe rng.

[eriktaubeneck]

Not sure what you mean by that. TestWorld generates a seed at random each time it is constructed.

Got it, my misunderstanding. I think we're still on the same page that the tests are "cryptographic" - the seed is known (and also all keys and everything else is all on one machine.) The intention isn't to be cryptographic, but to test the cryptography. The comment seems like the right approach.

[eriktaubeneck]

@akoshelev the comment I added is right below this? which other function doc are you referring to?

[akoshelev]

yea, I was referring to rustdoc comment to this function

[eriktaubeneck]

oh got it. let me move that up

[andyleiserson]

Is the in-memory-infra restriction temporary, or permanent?

[eriktaubeneck]

I would assume permanent. This is the errror without it; it seems that TestWorld is gated behind in-memory-infra, unless I'm misusing that.

[eriktaubeneck]

trying this out with run_random so we don't need this.

[andyleiserson]

There is a run_random helper that you could use for these tests instead of TestWorld (which does a bunch of other setup that isn't necessary if the test doesn't do MPC). Using run_random would avoid the in-memory restriction. Doesn't matter much, but maybe useful to know.