change(rpc): Return from long polling immediately when the chain tip changes

[teor2345]

Motivation

Miners want to start mining on the new tip immediately, to avoid wasted hash power and chain forks.

Closes #5720

Depends-On: #5843

Complex Code or Requirements

This PR uses the block consensus rules - changing the previous block hash or block time makes previous work invalid.

It also changes the tokio watch channel API, and does some complex future implementations. (But mainly for testing.)

Solution

• Check for chain tip changes using a future, and return on error
• Ignore state changes before the most recent state fetch
• Add a submit old field to the getblocktemplate RPC during long polling

Related changes:

• Add a best_tip_changed() method to trait ChainTip
• Add a mark as changed method to ChainTip

Review

Anyone can review this PR, it's mostly changes to trait implementations and a select!{} loop.

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

Follow Up Work

I think we're done here, we're doing what miners have told us is their top priority.

[teor2345]

I need to write tests before we merge this PR.

[codecov]

Codecov Report

Merging #5862 (7ac98eb) into main (f7011a9) will decrease coverage by 0.15%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5862      +/-   ##
==========================================
- Coverage   78.47%   78.32%   -0.16%     
==========================================
  Files         308      308              
  Lines       38709    38757      +48     
==========================================
- Hits        30378    30355      -23     
- Misses       8331     8402      +71     

[teor2345]

The test failures are in the snapshots:

     38 │+  "submitold": null

https://github.com/ZcashFoundation/zebra/actions/runs/3691441568/jobs/6249438736#step:14:2473

submitold should be missing when it's None, rather than null. I'll fix this tomorrow.

I think I need to add #[serde(skip_serializing_if = "Option::is_none()")] and #[serde(default = "Option::None")]. And then check it doesn't serialize as Some(true):
https://serde.rs/field-attrs.html#skip_serializing_if
https://docs.rs/serde_with/latest/serde_with/attr.skip_serializing_none.html

[mpguerra]

Can we evaluate this change in terms of how it will affect the code to be audited?

We had talked about keeping the getblocktemplate work self contained as much as possible so it didn't affect the stable release candidate series. This change might be fine as it's just making additions and not changing existing functionality in zebra-state and zebra-chain but I would just like us to be mindful of this.

[arya2]

Good call adding submitold and the mark_as_seen method on ChainTip.

[arya2]

Can we evaluate this change in terms of how it will affect the code to be audited?

We had talked about keeping the getblocktemplate work self contained as much as possible so it didn't affect the stable release candidate series. This change might be fine as it's just making additions and not changing existing functionality in zebra-state and zebra-chain but I would just like us to be mindful of this.

We should mark all of the new code here with #[cfg(feature = "getblocktemplate-rpcs")] too.

[teor2345]

Can we evaluate this change in terms of how it will affect the code to be audited?
We had talked about keeping the getblocktemplate work self contained as much as possible so it didn't affect the stable release candidate series. This change might be fine as it's just making additions and not changing existing functionality in zebra-state and zebra-chain but I would just like us to be mindful of this.

We should mark all of the new code here with #[cfg(feature = "getblocktemplate-rpcs")] too.

There are only around 30 lines of added production code in this PR. The rest is comments, code that's already inside the feature, or tests.

I'm happy to put that code behind the feature after I write some tests. Just in case the API needs to change for the tests.

But I just wanted to let you know that it won't impact a significant amount of code. (A single hex serialisation implementation is larger, and we've been leaving them outside the feature.)

[teor2345]

The test failures are in the snapshots:

     38 │+  "submitold": null

https://github.com/ZcashFoundation/zebra/actions/runs/3691441568/jobs/6249438736#step:14:2473

submitold should be missing when it's None, rather than null. I'll fix this tomorrow.

I think I need to add #[serde(skip_serializing_if = "Option::is_none()")] and #[serde(default = "Option::None")]. And then check it doesn't serialize as Some(true): https://serde.rs/field-attrs.html#skip_serializing_if https://docs.rs/serde_with/latest/serde_with/attr.skip_serializing_none.html

In 9a34243 I fixed the field serialization, made a separate snapshot that includes a submit old field, and changed the deserialized coinbase transaction format to RON. (Using JSON was a bit confusing due to the nulls.)

[teor2345]

There are only around 30 lines of added production code in this PR.

Arya made a suggestion, and we're now down to around 20 lines of added production code. At this point, I don't think it matters if it is behind a feature. (But I'm happy to do that.)

[arya2]

At this point, I don't think it matters if it is behind a feature. (But I'm happy to do that.)

I don't think so either.

This looks good to me if it's ready for review.

[teor2345]

This looks good to me if it's ready for review.

I think it still needs more tests, even if they have to be manual tests, I want to do them.

[teor2345]

I am happy with this PR, I did some tests as part of PR #5867. We can continue to test long polling in ticket #5686.