GH-552: Eliminate extra block increment

[masqrauder]

Follow-up to the original GH-552 with a new branch of the same name.

• With these changes the block number is no longer double-incremented.
• These changes also include additional tests to verify the block number handles the max_block_count size chunks and doesn't skip any blocks by incrementing one too many times.

[bertllll]

Would you mind if we rewrite these repetitive blocks of code as in iterator over a set of data?

I imagine it like:

let accountant_received_payment = accountant_recording_arc.lock().unwrap();
vec![
(received_payments_1, expected_transactions_1, new_start_bock_1),
(received_payments_2, expected_transactions_2, new_start_bock_2),
(received_payments_3, expected_transactions_3, new_start_bock_3),
(received_payments_4, expected_transactions_4, new_start_bock_4),
]
.enumerate()
.for_each(|(idx, (received_payments, expected_transactions, new_start_bock))|{
        let received_payment = accountant_received_payment.get_record::<ReceivedPayments>(idx);
        check_timestamp(before, received_payments1.timestamp, after);
        assert_eq!(
            received_payments,
            &ReceivedPayments {
                timestamp: received_payments.timestamp,
                payments: expected_transactions.transactions,
                new_start_block: new_start_block,
                response_skeleton_opt: Some(ResponseSkeleton {
                    client_id: 1234,
                    context_id: idx
                }),
            }
        );
})

It's a nice way of making the test less lengthy and easier to scan over.

[bertllll]

Similarly as I pointed out in the comment after this one, you could refactor some parts out like this:

The replacement passage starts by send_bind_message!().

let requests = 1..=4.map(|context_id|{
     RetrieveTransactions {
            recipient: earning_wallet.clone(),
            response_skeleton_opt: Some(ResponseSkeleton {
                client_id: 1234,
                context_id,
     })
}).collect_vec();
let before = SystemTime::now();

requests.into_iter().for_each(|request|{
     let _ = addr.try_send(request).unwrap();
})

System::current().stop();
system.run();
let after = SystemTime::now();

😉

[bertllll]

This, the use of this TestServer isn't an issue immediately. However, when Syther and Utkarsh completes their work on GH-744, there might not be this thing anymore in the codebase. I've acquired an impression that they go with its alternative only, the MockBlockchainClientServer.

This might surprise you unpleasantly, therefore I'm preparing you for that eventuality. At least after this you could possibly recall I've mentioned it once and it'll bring you there to what you need.

[bertllll]

This is not your fault. I don't even know if I should require this.

...but the traditional location of this expression is the very first line of tests.

[bertllll]

Here we can see again passages massively repeating the same steps. If you can, please create a parametrized function whose body will contain these code pieces. Something like:

fn test_retrieve_transactions(last_end_block: u64, newly_computed_end_block: BlockNumber, wallet: &Wallet)-> u64 {
    let result3 = subject
        .retrieve_transactions(last_end_block, newly_computed_end_block, wallet)
        .unwrap();

    if let BlockNumber::Number(value) = result3.new_start_block {
        value.as_u64()
    } else {
        panic!("Unexpected block number type")
    }
}

or maybe even better as a closure with only two args...

let test_retrieve_transactions = |last_end_block: u64, newly_computed_end_block: BlockNumber| -> u64 {
    let result3 = subject
        .retrieve_transactions(last_end_block, newly_computed_end_block, wallet)
        .unwrap();

    if let BlockNumber::Number(value) = result3.new_start_block {
        value.as_u64()
    } else {
        panic!("Unexpected block number type")
    }
};

[bertllll]

Just to be consistent with the way you represent the values in the following assertions, I'd prefer you writing

   assert_eq!(new_start_block_1, 1 + 1 + DEFAULT_MAX_BLOCK_COUNT);

[bertllll]

These log assertion don't need to be present at all. They probably are of any acute matter, rather debugging.
If you insist on them being in here, I'd like to ask you to write an iteration process over just two values in a tuple, which all in a vector together. The first value would signify the start_block parameter, the second would take place for the end_block parameter, both inserted into a single string with placeholders for these two values. The string would be this message in your log assertions...

You'd minimize the quantity of code one must read if it's needed that test should be checked and understood quickly. Because not always can you be sure that what looks similar can be safely considered having a pattern. That means, uncovering there isn't a pattern really, that it contains exceptions, requires careful reading.
However if we refactor it out, nobody will ever wonder again. It'll invoke the feeling that it repeats identically (especially after the repetitively invoked function is inspected closely for not containing any conditions...; not a problem here, sure...).