fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 209 forks source link

Concurrent integration tests #493

Closed justinmoon closed 1 year ago

justinmoon commented 1 year ago

Integration tests should be able to run concurrently.

This PR attempted a fix by using randomly generated ports, but I think it would be better to have an option to mock out network APIs so we don't need to worry about port conflicts at all.

Figured I'd make an issue because this is a recurring problem that needs to be fixed.

jkitman commented 1 year ago

With the mocks you should be able to run the Rust tests concurrently since everything included the network is mocked. With the real fixtures it will not be possible because you depend on shared state (in bitcoind and lightningd) and then you might have port collisions.

This code should avoid port collisions https://github.com/fedimint/fedimint/blob/master/integrationtests/tests/fixtures/mod.rs#L95-L111 However, it's possible that those ports are already bound somehow (but I'm not sure how), and we could use portpicker here instead. We probably don't need to introduce that dependency outside of the fedimint-tests crate though.

I'm not convinced this is the cause of the tests deadlocking, can anyone recreate it?

NicolaLS commented 1 year ago

@jkitman The hbbft and api will not collide because of the code but the gateway will. Should we just use base_port + num_peers + 1 for the gateway ? Also if that actually solves the issue do we still want to mock everything network related with channels to increase performance and have more control (for eg #287) ? I wanted to implement this soon

jkitman commented 1 year ago

@NicolaLS If this solves the issue let's just do the port change for now...although I'm not sure how you can test it solves it.

NicolaLS commented 1 year ago

@NicolaLS If this solves the issue let's just do the port change for now...although I'm not sure how you can test it solves it.

Ill just run cargo test -- --test-threads=8 (I think default is more than one but not full) a few times haha, normaly it would only succeed like 2 out of 10 times (think not even that much)

jkitman commented 1 year ago

Must be one of those strange environment things because I never have it happen locally.

NicolaLS commented 1 year ago
failures:
    drop_peers_who_dont_contribute_decryption_shares
    drop_peers_who_dont_contribute_peg_out_psbts
    minted_coins_can_be_exchanged_between_users

..only drop_peers_who_dont_contribute_decryption_shares uses the gateway so there are more/other problems

NicolaLS commented 1 year ago

ok but my PR dosn't fix it...

thread 'tokio-runtime-worker' panicked at 'error binding to 127.0.0.1:4064: error creating server listener: Address already in use (os error 98)', /home/sus/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.14.20/src/server/server.rs:77:13
jkitman commented 1 year ago

What command are you running + full log? And are mocks enabled?

NicolaLS commented 1 year ago

Yes mocks enabled I can run it again but I think it was actually just something else. Doing sudo netstat -tunlp | grep :4 just gave me udp 0 0 0.0.0.0:43238 0.0.0.0:* 2541/firefox so maybe it was just some other program

NicolaLS commented 1 year ago

To observe I run watch 'sudo netstat -tunlp | grep fedimint' but Ill grep :4 instead and see if something other than fedimint pops up

elsirion commented 1 year ago

Did #497 fix this?

NicolaLS commented 1 year ago

Did #497 fix this?

yes this and #512

NicolaLS commented 1 year ago

I would keep this issue open though because we might still want to mock the networking stuff and use a portpicker

NicolaLS commented 1 year ago

or maybe close this and open a new one for that I'm not sure what'd be best

justinmoon commented 1 year ago

Let's close this and open up a new issue for mocking out network https://github.com/fedimint/fedimint/issues/545