Retrieve outputs within a block height range

[yeastplume]

Related to mimblewimble/grin-wallet#246

The idea is to ensure there's an API to scan outputs committed within a specified range of blocks. This will be used during the wallet update process to scan the output set (similarly to how the current check_repair function works,) and and re-create any outputs that are missing due to forks or multiple wallets using the same seed.

This PR simply adds the function to the API, WIP until I've verified and tweaked against the current wallet PR.

[yeastplume]

Decided it was easier and more consistent to just add a new API function that returns the pmmr indices corresponding to a range of blocks, and then just passing that to the existing outputs_by_pmmr function. Also modified the outputs_by_pmmr functionality to work directly on pmmr indices rather than accepting insertion indices and converting them. This was done to make the functionality more consistent given that function also needs to work with a maximum index.

[antiochp]

Subjective comment but "heightstopmmr" is a confusing name... "height stop mmr"?

Also not convinced we want to leak implementation details like this?
Seems like (at least intuitively) that you'd want to get outputs based on height directly, rather than retrieving the intermediate mmr positions.

Once we start doing things based on positions we inevitably go down the path of "is this 1-indexed or 0-index" and "is this leaf insertion indices or actual positions in the MMR"? And it is potientially confusing here because we are looking for leaves, the outputs themselves but via mmr positions, and presumably 1-indexed.

I'd much rather expose this via the api based on block height and keep the MMR implementation hidden behind the api.

[yeastplume]

Going to merge based on discussions below. Note this change actually makes the PMMR scan a bit more consistent as it uses actual position exclusively as opposed to index. Can come back to this later on during a general cleanup on what the API exposes:

"heights to pmmr", it's a crap name but I didn't have anything better
So it sounds like you prefer the earlier approach, where the current 'outputs' function is changed to accept block heights instead?
antiochp
2:48 PM
I think so yeah (personal preference) - just hesitant to expose more of the MMR internals via the api, specifically as we are inconsistent in various usage in terms of 0-indexed and 1-indexed and leaf index vs position.
EDITED
exposing this via the api will potentially complicate reworking this to be more consistent (and will be a source of confusion for others)
yeastplume
2:48 PM
this was basically done this way to be as compatible as possible with the existing pmmr scan, which confusingly accepted a leaf index position but returned an actual position to continue the scan from on the next chunk of scan
antiochp
2:48 PM
that said - willing to be ignored convinced otherwise
EDITED
which confusingly accepted a leaf index position but returned an actual position to continue the scan from on the next chunk of scan
that's even worse than my examples...
EDITED
yeastplume
2:50 PM
heh.. right.. well that's because it was returning, say chunks of 1000 at a time, and it was more efficient to just return the last index and continue from there
antiochp
2:51 PM
maybe we don't overthink it right now and revisit it once we start getting more serious about consistent usage
yeastplume
2:51 PM
but I can get the point about exposing pmmr index details being confusing. I think just changing the output function to accept height instead should work just as well, it may be just slightly less efficient
antiochp
2:52 PM
I'm good with leaving it as you propose right now - and we can tackle the bigger picture later
(I suspect the ideal approach is to always use leaf indices for data itself, and only ever use positions for hashes)
EDITED
leaves by position is just an implementation detail
yeastplume
2:53 PM
yeah.. hmm would probably appreciate that as what I have is working as intended and there's a ton of other stuff to attend to. That function call is relatively well encapsulated anyhow so we can revisit at a later date on a pass that cleans up the API somewhat
👍