Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Measure scan progress from wallet birthday, not the fully scanned height #944

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

nuttycom
Copy link
Contributor

@nuttycom nuttycom commented Sep 6, 2023

No description provided.

@zookozcash
Copy link

Not to go into SDK 2.0 Release Candidate, at least pending my imminent phone call with Kris.

@nuttycom nuttycom force-pushed the wallet/progress_from_birthday branch from 10f1508 to 19299a1 Compare September 6, 2023 18:00
@str4d
Copy link
Contributor

str4d commented Sep 6, 2023

The motivation for this change is that the current approach looks exponential, which gives the impression to users during recovery from seed or after long periods offline, of very little progress. Here are some snapshots from my mainnet node:

    chain_tip_height: BlockHeight(
        2217761,
    ),
    fully_scanned_height: BlockHeight(
        1373035,
    ),
    scan_progress: Some(
        Ratio {
            numerator: 48807,
            denominator: 72279746,
        },
    ),
     Synced: 0.068%
[...]
    chain_tip_height: BlockHeight(
        2217766,
    ),
    fully_scanned_height: BlockHeight(
        1722754,
    ),
    scan_progress: Some(
        Ratio {
            numerator: 48818,
            denominator: 71762038,
        },
    ),
     Synced: 0.068%
[...]
    chain_tip_height: BlockHeight(
        2217771,
    ),
    fully_scanned_height: BlockHeight(
        1732754,
    ),
    scan_progress: Some(
        Ratio {
            numerator: 48831,
            denominator: 67203704,
        },
    ),
     Synced: 0.073%
[...]
    chain_tip_height: BlockHeight(
        2217785,
    ),
    fully_scanned_height: BlockHeight(
        1772754,
    ),
    scan_progress: Some(
        Ratio {
            numerator: 48867,
            denominator: 39559642,
        },
    ),
     Synced: 0.124%

@zookozcash
Copy link

Update: Per phone call with Kris and str4d’s notes above in this PR, current implementation does not satisfy Criterion 3, because under the (probably common and important use case) that the user hasn’t let their wallet sync in months or years, it would show no visible progress for a long time (like at least minutes, potentially hours). So I’m reversing the above decision to exclude this change to progress measure for 2.0.

@nuttycom nuttycom force-pushed the wallet/progress_from_birthday branch from 19299a1 to f56c5d8 Compare September 6, 2023 18:07
@str4d
Copy link
Contributor

str4d commented Sep 6, 2023

With this PR, I see the following progress state instead (from the same wallet sync state as the last snapshot above):

    chain_tip_height: BlockHeight(
        2217785,
    ),
    fully_scanned_height: BlockHeight(
        1772754,
    ),
    scan_progress: Some(
        Ratio {
            numerator: 33342966,
            denominator: 72853741,
        },
    ),
     Synced: 45.767%

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f56c5d8. Output appears to be more comprehensible for the catching-up-to-chain-tip use case.

"SELECT MAX(sapling_commitment_tree_size)
FROM blocks
WHERE height <= :fully_scanned_height",
named_params![":fully_scanned_height": u32::from(fully_scanned_height)],
WHERE height <= :start_height",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: now that start_height = birthday_height there's an off-by-one here, because the birthday height is one of the blocks that gets scanned, not the base block.

Non-blocking, the error in the output is minimal, and it isn't worth re-running CI.

@str4d
Copy link
Contributor

str4d commented Sep 6, 2023

Output 10k blocks later:

    chain_tip_height: BlockHeight(
        2217817,
    ),
    fully_scanned_height: BlockHeight(
        1782754,
    ),
    scan_progress: Some(
        Ratio {
            numerator: 38544695,
            denominator: 72853741,
        },
    ),
     Synced: 52.907%

@str4d str4d merged commit 3d0ec00 into zcash:main Sep 6, 2023
@nuttycom nuttycom deleted the wallet/progress_from_birthday branch September 6, 2023 18:42
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-hoc ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants