zcash_client_sqlite: BlockSource::with_blocks impls behave unintuitively with from_height set to an unknown block

[str4d]

BlockSource::with_blocks is documented thusly:

librustzcash/zcash_client_backend/src/data_api/chain.rs

Lines 198 to 200 in 81d1928

	/// Scan the specified `limit` number of blocks from the blockchain, starting at
	/// `from_height`, applying the provided callback to each block. If `from_height`
	/// is `None` then scanning will begin at the first available block.

The documentation is clear that from_height = Some(h) means "scan from height h", while from_height = None means "scan from the first available block". However, it is not clear what should happen if from_height = Some(h) but h is unknown to the cache.

The implementations of this trait in zcash_client_sqlite behave similarly to the None case: up to limit blocks are made available starting from the first block with height >= h that the cache knows about. This is due to the following SQL query being used:

librustzcash/zcash_client_sqlite/src/chain.rs

Lines 43 to 54 in 81d1928

	let mut stmt_blocks = block_source
	.0
	.prepare(
	"SELECT height, data FROM compactblocks
	WHERE height >= ?
	ORDER BY height ASC LIMIT ?",
	)
	.map_err(to_chain_error)?;
	let mut rows = stmt_blocks
	.query(params![
	from_height.map_or(0u32, u32::from),

(and equivalently for FsBlockDb).

This behaviour crept in across several refactors:

• Originally, the SQL query against the cache DB was always relative to the latest block known to the wallet DB, because scan_cached_blocks queried it inline.
• The with_blocks method was factored out in 06c1772#diff-e9ee9345e22f7472adbe04051828e130308ab243f6ae047321ce4f70ee9e4abbR115 (as part of Data Access API (Low-level wallet persistence API) #307), but still passing in the inline-queried from_height inside scan_cached_blocks.
• The inline-queried from_height was replaced with a scan_cached_blocks argument in 3e358bc (as part of Migrations & data storage for pre-DAG-sync #831).
• from_height needed to be passed in by the mobile SDKs (e.g. Electric-Coin-Company/zcash-swift-wallet-sdk@6eb3956), causing bugs therein to be exposed to the Rust layer.

I found this issue because in some tests, the iOS SDK incorrectly set from_height = 0 as a side-effect of earlier logic, but due to the unintuitive behaviour of the FsBlockDb::with_blocks impl, it "self-corrected" to the intended first block in the database, hiding the bug (until it was exposed due to the change to move continuity testing into scan_cached_blocks).

We need to decide what the intended behaviour of BlockSource::with_blocks is, and then ensure that the documentation and implementations are consistent with it.

[str4d]

Possible behaviours:

• Keep it as-is: start scanning from whichever cache block is known above that height.
• Scan zero blocks (because there are none to scan that are known to the cache starting from that height).
• Scan as many blocks as are known that fall in the range from_height..(from_height + limit).
• Return an error (i.e. require the caller to only set from_height = Some(h) if h exists in the cache).

[nuttycom]

I think that if the specified block height isn't available in the cache, we should return an error; having the scan silently skip over part of the range is likely to cause subtle misbehavior that is difficult to debug, whereas handling the error is easy - just go get the block.