rpc: Add getwalletstatus

[str4d]

Closes #316.
Closes COR-338.

[str4d]

I named it thusly because @oxarbitrage suggested walletstatus but get* fits better with existing naming. But is getwalletstatus both sufficiently clear vs getwalletinfo, and non-colliding with existing Bitcoin JSON-RPC names?

[oxarbitrage]

I think this is fine. There is no getwalletstatus in Bitcoin and i think the name is clear enough.

[str4d]

Currently what happens is:

• Accounts to recover are loaded into the wallet/
• Wallet scans the chain tip region.
  • fully_synced_height = None because the wallet birthday height has not been scanned.
  • This takes quite a while.
• Once the chain tip region is scanned, the next scan range is the birthday height.
  • fully_synced_height is now set.

Should instead fully_synced_height be non-optional and set to the last ignored height, or as a fallback the genesis block?

[oxarbitrage]

From an implementer’s perspective, optional fields are generally harder to work with, since the client needs to check for their presence before accessing them. They can also make responses more confusing in some cases.

In general, I tend to prefer having fields always present, but Zcash already uses this pattern in several places, so I think it’s fine as long as the behavior is clearly documented.

[str4d]

Having it be present-but-null vs not-present-if-null is a different question than the one I was asking.

My question was, should None be an allowable value here, or should we instead return something else?

[oxarbitrage]

Thanks for clarifying. I think returning the genesis block, the birthday block, or anything similar would also be confusing, so I prefer keeping None as you have it. Whatever we decide, I think it would be good to document this
behaviour in the field's description.

[str4d]

These are temporary because it's all I have access to currently. Given that Zallet is intended to track the backing node's chain tip as closely as possible, once #237 is done we will always know the size of the note commitment trees, and thus it should always be possible to determine how many leaves within them have not yet been appended (and thus are unscanned).

[oxarbitrage]

Hey, thanks for adding this, looking good.

I’ve been trying to integrate this into the Zebra QA framework so we can get rid of several anti-pattern sleeps. In particular, I was experimenting with using it in this test:

https://github.com/ZcashFoundation/zebra/blob/b46305a6437bea49d268aeb915b8e62ee4e63f0a/zebra-rpc/qa/rpc-tests/wallet.py

The test passes most of the time, but occasionally it gets stuck in scenarios like this:

{
  'node_tip': {
    'blockhash': '6058ceb2b07f544c586ad948b68a7ed49d6c3fdec8ac2f1b26779993f29f64fc',
    'height': 4
  },
  'wallet_tip': {
    'blockhash': 'd72879af1530f3c234c974e08bbf9f62c38e69245a7880b6947c10f299b2ab54',
    'height': 3
  },
  'fully_synced_height': 3
}

In this case, I’m waiting for the node tip and wallet tip to match before proceeding, but that never happens unless I mine a new block (which then triggers the Zallet scan to advance).

I’m not fully sure yet whether this is an issue in the test logic or something on the Zallet side. Any ideas or pointers would be very welcome. I’ll keep digging to find the root cause as well.

Happy to continue reviewing and approve once I can get this sorted out. Thanks!

[oxarbitrage]

Suggested change

	progress_numerator: u64,
	progress_denominator: u64,
	progress: (numerator: u64, denominator: u64),

Optional: These fields are closely related. Maybe consider grouping them into a tuple or a small type.

[linear]

COR-338 Add a `walletstatus` RPC method

[str4d]

The test passes most of the time, but occasionally it gets stuck in scenarios like this:

{
  'node_tip': {
    'blockhash': '6058ceb2b07f544c586ad948b68a7ed49d6c3fdec8ac2f1b26779993f29f64fc',
    'height': 4
  },
  'wallet_tip': {
    'blockhash': 'd72879af1530f3c234c974e08bbf9f62c38e69245a7880b6947c10f299b2ab54',
    'height': 3
  },
  'fully_synced_height': 3
}

In this case, I’m waiting for the node tip and wallet tip to match before proceeding, but that never happens unless I mine a new block (which then triggers the Zallet scan to advance).

My guess is this will be fixed as part of #336. So I think we should get zcash/integration-tests#1 merged, then test applying this PR to its current state, and then test #336 after merging this PR.