Add a ChainTipChange type to await chain tip changes

[teor2345]

Motivation

Some tasks need to wait for the state's best chain tip to change.

Solution

• crate a basic ChainTipChange implementation
  • expand existing tests to cover the new type
• rename some types and variables to avoid confusion

This PR is part of ticket #2639, but it doesn't close that ticket.

Review

@jvff can review this PR.

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

Implement TipAction::Reset in ChainTipChange - #2639

[jvff]

Overall it looks good. The only design related thing that could be discussed is if it would make sense to have only one type. I.e., moving the methods from LatestChainTip into ChainTipChange. It has the advantage of being less code and possibly requiring less mental context, but it does have the disadvantage of having two purposes for the same type, which is okay but could lead to increasing the mental context depending on what scenarios it is used. I'm fine either way, I just wanted to bring it up in case it helps.

[jvff]

Question: I'm wondering if separating the binding from the construction is a matter of style or if there's some advantage I missed. Like, is this used to be able to capture when new fields are added to PreparedBlock in the future? Just for comparison, my default approach would be something like:

Suggested change

	let PreparedBlock {
	block: _,
	hash,
	height,
	new_outputs: _,
	transaction_hashes,
	} = prepared;
	Self {
	hash,
	height,
	transaction_hashes,
	}
	Self {
	hash: prepared.hash,
	height: prepared.height,
	transaction_hashes: prepared.transaction_hashes,
	}

I think it's a matter of style, so there's no need to change it. I'm just curious if there's a reason or if there are certain situations where it's preferable to write the destructuring separately 🤔

[teor2345]

It's used to avoid cloning the Arcs, which is slightly more expensive due to the atomic reference count.

I've also found it useful to explicitly show all the fields, and show which fields we're dropping.

[jvff]

One more design thing. The ChainTipChange type could impl Stream, which would allow using the StreamExt methods. But I'm not sure if we'd ever need it 😅

Edit: This might need to wrap the watch::Receiver in a tokio_stream::wrappers::WatchStream, so I'm not sure if it's worth it 🤔

[teor2345]

One more design thing. The ChainTipChange type could impl Stream, which would allow using the StreamExt methods. But I'm not sure if we'd ever need it 😅

Edit: This might need to wrap the watch::Receiver in a tokio_stream::wrappers::WatchStream, so I'm not sure if it's worth it 🤔

I deliberately named next after the existing stream and iterator methods.

We can turn it into a stream, if we decide that would be useful in a later PR.

Edit: actually, I don't think that would be helpful, because it's conceptually different.

[teor2345]

The only design related thing that could be discussed is if it would make sense to have only one type. I.e., moving the methods from LatestChainTip into ChainTipChange. It has the advantage of being less code and possibly requiring less mental context, but it does have the disadvantage of having two purposes for the same type, which is okay but could lead to increasing the mental context depending on what scenarios it is used.

They're actually quite different types, I'll see if I can update the docs and names to make that clearer.

[teor2345]

They're actually quite different types, I'll see if I can update the docs and names to make that clearer.

@jvff what do you think of 09f451e?
Does it help clarify the differences?

[teor2345]

The only design related thing that could be discussed is if it would make sense to have only one type. I.e., moving the methods from LatestChainTip into ChainTipChange. It has the advantage of being less code and possibly requiring less mental context, but it does have the disadvantage of having two purposes for the same type, which is okay but could lead to increasing the mental context depending on what scenarios it is used.

Hopefully the code at https://github.com/ZcashFoundation/zebra/pull/2721/files#r700043308 also clarifies this question.

[jvff]

@jvff what do you think of 09f451e?
Does it help clarify the differences?

Yep, thanks!