Fix blockchain sync progress

[thomask7b]

I've merged roundup_progress in rounded_up_progress since was in utils and was no longer used by any other method.
I also added some tests to verify the precision's update but also for specific cases that we manage. By the way tests has been moved in the related file.

Solves #907

[jp1ac4]

Thanks very much for this PR. I tested the change is working as expected.

I think merging the two functions makes sense in some ways, but it could still be useful to keep them separate for testing the rounding logic by itself so I would keep them separate for now.

[thomask7b]

I can adapt if you want but I'm not sure to understand what you mean, because the rounding logic is still tested with even more coverage.
Also think it adds complexity to have the function in another file when it can be in SyncProgress.
So we could move both metohds in SyncProgress. And then why not merge them and test the final method.
I don't understand what you dislike.
Let me know what you think and I'll adapt.

[jp1ac4]

I meant to test the rounding logic just in terms of the floats without any regard to the number of blocks or headers. But I think your changes are good and either approach would be fine.

Given that there's already a separate utils function and in order to keep the changes small, I would keep this separate function as I don't think there's a clear advantage not to.

[thomask7b]

I took in account your remarks.
I kept specific tests for SyncProgress::rounded_up_progress to verify that we "don't return a 100% progress until we are actually done syncing"

[pythcoiner]

not related to this PR:

Suggested change

	if progress == 1.0 && self.blocks != self.headers {
	if progress == 1.0 && self.blocks < self.headers {

@jp1ac4 i don't think this is likely to happen as iiuc as of now it's only used w/ a bitcoind backend, but wdyt we should do if self.blocks > self.headers? we actually return 0.999 but maybe 1.0 should be more relevant? (i'm not sure from a security POV, if returning 0.999 i guess we will stuck in the syncing process while if returning 1.0 we will assumed we are synced)

[jp1ac4]

Hmm I'm not sure really but I guess if they're not equal in either direction then something still needs to happen in terms of syncing.

[pythcoiner]

maybe you can add this test cases?:

        // approximatively year 2198
        assert_eq!(
            SyncProgress::new(1.0, 9999999, 9999998).rounded_up_progress(),
            0.9999
        );
        //bug or corrupted bitcond (blocks > headers)
        assert_eq!(
            SyncProgress::new(1.0, 999998, 999999).rounded_up_progress(),
            0.9999
        );

[pythcoiner]

lgtm:

@thomask7b could you squash your commits into a single one please?

[pythcoiner]

tACK dc4feac

[jp1ac4]

Thanks for the updates.

In addition to the suggested test, could you also please update the docstrings of the two functions.

[jp1ac4]

It would be good to include an additional test to show that numbers below 0.9999 are truncated rather than rounded up, e.g.

diff --git a/src/bitcoin/d/utils.rs b/src/bitcoin/d/utils.rs
index c317172b..ddd6d8af 100644
--- a/src/bitcoin/d/utils.rs
+++ b/src/bitcoin/d/utils.rs
@@ -376,6 +376,7 @@ mod tests {
         assert_eq!(roundup_progress(0.998), 0.998);
         assert_eq!(roundup_progress(0.9476), 0.9476);
         assert_eq!(roundup_progress(0.94761), 0.9476);
+        assert_eq!(roundup_progress(0.94769), 0.9476);
         assert_eq!(roundup_progress(0.9998), 0.9998);
         assert_eq!(roundup_progress(0.99998), 1.0);
         assert_eq!(roundup_progress(0.99991), 1.0);

[jp1ac4]

You can squash your commits again before pushing. Thanks.

[pythcoiner]

ACK 439af75

[jp1ac4]

Tested ACK 439af75.

Thanks very much.