shardtree: Add the ability to avoid pruning specific checkpoints.

[nuttycom]

No description provided.

[str4d]

Reviewed 34b1a5f.

[str4d]

This API needs a bit more documentation, because it is unclear whether or not this data being stored in ShardTree means it needs to be serialized. In particular, we should document that this API needs to be called after constructing ShardTree but before calling any methods that take &mut self (or whatever other criterion gates potential pruning). An example would be ideal:

Suggested change

	/// Adds the provided checkpoint to the set of checkpoints to be retained
	/// across pruning operations.
	/// Adds the provided checkpoint to the set of checkpoints to be retained
	/// across pruning operations.
	///
	/// TODO: A bit more documentation.
	///
	/// # Examples
	///
	/// ```
	/// use incrementalmerkletree::Retention;
	/// use shardtree::ShardTree;
	///
	/// # fn load_shardtree_from_disk() -> ShardTree<SomeShardStore> {
	/// # todo!("Kris returns something useful for testing here.")
	/// # }
	/// # fn checkpoints_to_retain() -> Vec<SomeCheckpointId> {
	/// # vec![] // TODO: Return something useful for testing here.
	/// # }
	/// # fn get_next_leaf() -> SomeLeafType {
	/// # todo!("Kris returns something useful for testing here.")
	/// # }
	/// # fn main() -> Result<(), Box<dyn std::error::Error>> {
	/// let mut tree = load_shardtree_from_disk();
	///
	/// // The required checkpoints are stored separately from the tree.
	/// for checkpoint_id in checkpoints_to_retain() {
	/// tree.ensure_retained(checkpoint_id);
	/// }
	///
	/// // Now perform operations that might prune the tree.
	/// tree.append(get_next_leaf(), Retention::Ephemeral)?;
	/// # Ok(())
	/// # }
	/// ```

[nuttycom]

Hm, I guess maybe I should actually add this to the ShardStore interface, so that it's up to the implementer of that to decide whether or not the explicitly retained items are represented in the serialization. In the zcash_client_sqlite database, they'll actually be serialized; the retained checkpoint identifier will always be the block height prior to the earliest unscanned FoundNote or ChainTip range, and each time we go to perform a put_blocks operation, we'll check whether the operation has increased that height.

It gets a little complicated, because Historic and OpenAdjacent scanning can find notes at lower heights than the current "high water mark" - in the event that that happens, the value of those notes can't be added to the available balance until their entire subtrees are scanned. This leads to an uncomfortable situation where you might be able to increase the available balance if you were to choose a lower checkpoint height, for example if you have a small spendable note at the chain tip, but then discover a large spendable note at a lower height.

[str4d]

Add a test case where we request to retain a checkpoint that is within the range of checkpoints that would already be retained. This is to exercise the self.max_checkpoints + self.to_retain.len() behaviour I questioned above; we should have a case that checks whichever behaviour we go with.

[daira]

Suggested change

	/// The minumum number of checkpoints to retain when pruning.
	/// The minimum number of checkpoints to retain when pruning.

[daira]

This seems quite a significant semantic change for something that retains the same type signature. Is it the case that previous callers will be correct for the new semantics? In any case please document the parameter.

[daira]

utACK with comments.