add get_confirmations() RPC and add to dashboard

[dan-da]

Addresses #6. (partial)

The major objective of this PR is adding the get_confirmations RPC. A secondary objective is displaying the confirmations field in overview screen of the Dashboard. Other changes are ancillary.

Server:

• implement get_confirmations RPC
• return confirmations field in get_dashboard_overview_data RPC
• add wallet_state::get_latest_balance_height()
• add block_height to tuple returned from wallet_state::get_balance_history

Dashboard:

• display confirmations in overview screens
• change confirmations field from usize to u64

Notes:

• The get_confirmations() RPC function has been simplified and made more efficient than the prototype here.
• get_latest_balance_height() will need to be modified to take into account unsynced/abandoned utxo. There is already Balance history does not agree with balance #28 for this, so I think it is best to do in a follow-up PR. Also, I'm not yet certain of the correct fix, as I did try to simply filter out by utxo.is_abandoned in get_balance_history() and that did not solve the balance/history sync issue. suggestions welcome.
• I considered returning additional fields from get_latest_balance_height() and just calling it get_latest_balance(). However the amount field requires an additional call+loop and since there is no present need, I decided to keep it as simple/fast as possible.
• As a first pass at optimizing I had added block_height field to values returned from wallet_state::get_balance_history(). That fn is no longer called from get_confirmations() however I left the block_height field in place because I think it would be useful to callers of the get_history() RPC. I think it would be useful to display the height in Dashboard history, but decided to leave that change for a follow-up PR. Anyway, this note explains that diff.
• I'm a bit uncertain yet about the correct type(s) for block heights (and counts). There is the BlockHeight type which the compiler treats as i128. But it doesn't really make sense for a height to be signed. Also, i128 and u128 both seem like overkill... how many blocks are we anticipating? Is u64 not enough? The dashboard was previously using usize for confirmations field, which seems a bit loose as it could be u32 on some platform, so I changed it to u64 and also my reasoning is less data over the wire vs u128. But I would rather just be consistent with a single type (based on u64?) used everywhere to represent block heights. Perhaps the RPC and Dashboard should all be using the BlockHeight for confirmations field rather than a primitive type. thoughts?

Testing:

• Restarted server and Dashboard, then verified that confirmations are displayed in the overview screen to the right of synced balance
• Verified that confirmations field is zero when a new block is mined to my wallet, and increases by 1 for each subsequent block.
• Compared the synced balance with top-most balance in History screen. The history screen balance is higher than synced balance. This is wrong but expected until Balance history does not agree with balance #28 is fixed.

I wanted to create a unit test for get_confirmations RPC however it is unclear to me how to do so. There are a few unit tests in rpc_server.rs, but afaict none of them setup a wallet with a non-zero balance which would be a requirement for my test(s). If someone could provide such a setup fn, I would happily make use of it.

[Sword-Smith]

I'm afraid Alan and I gave you some wrong information. The description in #6 says " The number of blocks (=confirmations) since the most recent balance update". If the last balance update is a outgoing transaction (UTXOs being spent), then any of the wallet's monitored UTXOs could have been spent, not just the last one. The monitored UTXOs are ordered by the blockheight at which they were received, and this ordering does not change. The get_balance_height does actually return the relevant information that get_latest_balance_height needs, as the returned list from get_balance_height can be ordered by BlockHeight and then the last element in that ordered list should contain the latest update. Sorry that we didn't catch that when you showed you suggestion in #6.

I would suggest:

1. Fix logic to return BlockHeight of latest balance update -- not latest incoming transaction as this PR's code does.
2. Write a test demonstrating correct behavior, or find an existing test that sets up the relevant states and add a call to get_latest_balance_height in that.
3. Verify that the block in question belongs to the canonical chain. You can use block_belongs_to_canonical_chain in archival_state.rs for this. If it does not, then find the next-most recent balance update etc. This point can also be addressed later, as you suggest, if you prefer that

u64 for blockheight should be more than enough. But why not just use the existing BlockHeight struct that is a wrapped BFieldElement which is a data structure that goes up to almost u64::MAX ($2^{64} - 2^{32} + 1$ to be precise.)

[Sword-Smith]

Please see previous comment

[dan-da]

Fix logic to return BlockHeight of latest balance update -- not latest incoming transaction as this PR's code does.

Done. I think. Please review. ;-)

duh, whoops! nice catch on that. Now that you point it out, of course the list must be traversed to find latest spent utxo. I do wonder though if it could be possible to store the latest spent in the wallet to make this lookup o(1). Anyway I still tried to make the fn as efficient as possible given that it is called by the Dashboard overview. Maybe premature optimization, I dunno.

Write a test demonstrating correct behavior, or find an existing test that sets up the relevant states and add a call to get_latest_balance_height in that.

I didn't see such a test in wallet_state.rs. Perhaps I need to look elsewhere, or maybe you can point me to one. Anyway, I've run out of time tonight, so will look at that next coding session. We can keep the PR open.

Verify that the block in question belongs to the canonical chain

haven't looked into this yet. I will take a look and if it seems simple/clear and it works I will do it, else leave for a followup PR.

why not just use the existing BlockHeight struct

yes! That was my initial instinct, but I was a bit confused because the Dashboard was initially written to accept a u64 and BlockHeight::sub() returns i128. And I wasn't sure how pedantic we are being about BlockHeight vs say BlockCount. ie, a strict interpretation could say that a Height is only to be used in context of a count from block 0. Anyway, with your blessing I changed it to use BlockHeight everywhere in latest commit.

extra: I added an impl Display for Sign that just prints "-" if sign is negative. This helped me debug print a list of utxo's with amount and I figure it may be worth keeping, but happy to remove if you don't like it.

[Sword-Smith]

With respect to the testing, you can shoehorn tests of the get_latest_balance_height into the existing test wallet_state_prune_abandoned_mutxos. It's a bit intricate because the test previously didn't update all parts of the state as it would under real execution, so I modified the test to verify what I consider correct behavior for this new function. If you want to do test-driven development of this new function, feel free to cherry-pick this commit :)

Note that this modified test also checks for the correct behavior of property "3" in my above comment.

Verify that the block in question belongs to the canonical chain.

Also note that this modified test requires you to move the method get_latest_balance_height from wallet_state.rs to the file containing GlobalState.

2bc4858

Sorry about our confusion of types for BlockHeight, block count etc. The data structures are not set in stone, and neither is what we store in the database. So if you want to add anything there, feel free to suggest such changes. But maybe for now, you just want to get this functionality working and then we can revisit e.g. the wallet DB when you have a better overview?

Tip: Inspect the MonitoredUtxo data model and see what methods it has available. You can use existing methods to solve property "3".

[dan-da]

thx! super helpful comments. I'm away today, but will continue on it over the weekend.

[dan-da]

I think this is ready for another review. See commit messages for summary of changes.

[Sword-Smith]

Looks very good.