Use a channel in sync timing test

[oxarbitrage]

Motivation

We are trying to get rid of Atomics, including in test cases. #2268

Designs

[link to async coding rfc - atomics section]

Solution

Use a tokio::watch for this test.

Review

By now this is a draft for sharing with @jvff

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

[oxarbitrage]

Error:

oxarbitrage@oxarbitrage-Lenovo-ideapad-320S-14IKB:~/zebra/fix_request_genesis_test/zebra$ cargo test
   Compiling zebrad v1.0.0-alpha.10 (/home/oxarbitrage/zebra/fix_request_genesis_test/zebra/zebrad)
error[E0277]: the trait bound `tokio::sync::watch::Sender<u8>: Clone` is not satisfied in `[closure@zebrad/src/components/sync/tests/timing.rs:87:42: 98:6]`
   --> zebrad/src/components/sync/tests/timing.rs:122:26
    |
87  |       let peer_service = tower::service_fn(move |request| {
    |  __________________________________________-
88  | |         match request {
89  | |             zebra_network::Request::BlocksByHash(_) => {
90  | |                 // Track the call
...   |
97  | |         }
98  | |     });
    | |_____- within this `[closure@zebrad/src/components/sync/tests/timing.rs:87:42: 98:6]`
...
122 |       let mut chain_sync = ChainSync::new(
    |                            ^^^^^^^^^^^^^^ within `[closure@zebrad/src/components/sync/tests/timing.rs:87:42: 98:6]`, the trait `Clone` is not implemented for `tokio::sync::watch::Sender<u8>`
    | 
   ::: zebrad/src/components/sync.rs:218:5
    |
218 |       pub fn new(config: &ZebradConfig, peers: ZN, state: ZS, verifier: ZV) -> Self {
    |       ----------------------------------------------------------------------------- required by `components::sync::ChainSync::<ZN, ZS, ZV>::new`
    |
    = note: required because it appears within the type `[closure@zebrad/src/components/sync/tests/timing.rs:87:42: 98:6]`
    = note: required because of the requirements on the impl of `Clone` for `ServiceFn<[closure@zebrad/src/components/sync/tests/timing.rs:87:42: 98:6]>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `zebrad`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
oxarbitrage@oxarbitrage-Lenovo-ideapad-320S-14IKB:~/zebra/fix_request_genesis_test/zebra$ 

[jvff]

I think I found the cause and a fix :D

[jvff]

The error seems to be caused by ChainSync needing to clone the service to create multiple instances of it. To allow that, I think one way would be to simply wrap the senders with Arcs so that the watch::Sender is shared between all instances:

Suggested change

	let (peer_requests_sender, peer_requests_receiver) = watch::channel(peer_requests_counter);
	let (state_requests_sender, state_requests_receiver) = watch::channel(state_requests_counter);
	let (peer_requests_sender, peer_requests_receiver) = watch::channel(peer_requests_counter);
	let (state_requests_sender, state_requests_receiver) = watch::channel(state_requests_counter);
	let peer_requests_sender = Arc::new(peer_requests_sender);
	let state_requests_sender = Arc::new(state_requests_sender);

[jvff]

I realized that the counter values will actually be copied instead of shared, so they will lead to incorrect counts :/ Replacing the atomics might need another similar abstraction 🤔

[teor2345]

I realized that the counter values will actually be copied instead of shared, so they will lead to incorrect counts :/ Replacing the atomics might need another similar abstraction 🤔

Looks like this cleanup is blocked by #2200, which will give us the watch::Sender::borrow method.

Can you update ticket #2268 with what we've learned here?

(This cleanup is a low priority right now, because the tests are passing. So we can just close this PR.)

[oxarbitrage]

The cleanup will be done after #2200 by using something as the following inside the services:

let _ = peer_requests_sender.send(*peer_requests_sender.borrow() + 1);

This should avoid any incorrect counts and will allow us to use peer_requests_receiver in the asserts at the end of the test.

Closing the PR and leaving the branch up for future reference.