change(state): Write finalized blocks to the state in a separate thre…

…ad, to avoid network and RPC hangs (#5134)

* Add a new block commit task and channels, that don't do anything yet

* Add last_block_hash_sent to the state service, to avoid database accesses

* Update last_block_hash_sent regardless of commit errors

* Rename a field to StateService.max_queued_finalized_height

* Commit finalized blocks to the state in a separate task

* Check for panics in the block write task

* Wait for the block commit task in tests, and check for errors

* Always run a proptest that sleeps once

* Add extra debugging to state shutdowns

* Work around a RocksDB shutdown bug

* Close the finalized block channel when we're finished with it

* Only reset state queue once per error

* Update some TODOs

* Add a module doc comment

* Drop channels and check for closed channels in the block commit task

* Close state channels and tasks on drop

* Remove some duplicate fields across StateService and ReadStateService

* Try tweaking the shutdown steps

* Update and clarify some comments

* Clarify another comment

* Don't try to cancel RocksDB background work on drop

* Fix up some comments

* Remove some duplicate code

* Remove redundant workarounds for shutdown issues

* Remode a redundant channel close in the block commit task

* Remove a mistaken `!force` shutdown condition

* Remove duplicate force-shutdown code and explain it better

* Improve RPC error logging

* Wait for chain tip updates in the RPC tests

* Wait 2 seconds for chain tip updates before skipping them

* Remove an unnecessary block_in_place()

* Fix some test error messages that were changed by earlier fixes

* Expand some comments, fix typos

Co-authored-by: Marek <[email protected]>

* Actually drop children of failed blocks

* Explain why we drop descendants of failed blocks

* Clarify a comment

* Wait for chain tip updates in a failing test on macOS

* Clean duplicate finalized blocks when the non-finalized state activates

* Send an error when receiving a duplicate finalized block

* Update checkpoint block behaviour, document its consensus rule

* Wait for chain tip changes in inbound_block_height_lookahead_limit test

* Wait for the genesis block to commit in the fake peer set mempool tests

* Disable unreliable mempool verification check in the send transaction test

* Appease rustfmt

* Use clear_finalized_block_queue() everywhere that blocks are dropped

* Document how Finalized and NonFinalized clones are different

* Use the same check as commit_finalized() for finalized block heights

Co-authored-by: Marek <[email protected]>

Co-authored-by: Marek <[email protected]>

zebra-rpc/src/methods.rs

@@ -872,7 +872,7 @@ where
                     hashes
                         .iter()
                         .map(|(tx_loc, tx_id)| {
-                            // TODO: downgrade to debug, because there's nothing the user can do
+                            // Check that the returned transactions are in chain order.
                             assert!(
                                 *tx_loc > last_tx_location,
                                 "Transactions were not in chain order:\n\
@@ -931,7 +931,7 @@ where
                 let satoshis = u64::from(utxo_data.3.value);
 
                 let output_location = *utxo_data.2;
-                // TODO: downgrade to debug, because there's nothing the user can do
+                // Check that the returned UTXOs are in chain order.
                 assert!(
                     output_location > last_output_location,
                     "UTXOs were not in chain order:\n\
@@ -1272,17 +1272,19 @@ impl GetRawTransaction {
 /// Check if provided height range is valid for address indexes.
 fn check_height_range(start: Height, end: Height, chain_height: Height) -> Result<()> {
     if start == Height(0) || end == Height(0) {
-        return Err(Error::invalid_params(
-            "Start and end are expected to be greater than zero",
-        ));
+        return Err(Error::invalid_params(format!(
+            "start {start:?} and end {end:?} must both be greater than zero"
+        )));
     }
-    if end < start {
-        return Err(Error::invalid_params(
-            "End value is expected to be greater than or equal to start",
-        ));
+    if start > end {
+        return Err(Error::invalid_params(format!(
+            "start {start:?} must be less than or equal to end {end:?}"
+        )));
     }
     if start > chain_height || end > chain_height {
-        return Err(Error::invalid_params("Start or end is outside chain range"));
+        return Err(Error::invalid_params(format!(
+            "start {start:?} and end {end:?} must both be less than or equal to the chain tip {chain_height:?}"
+        )));
     }
 
     Ok(())

zebra-rpc/src/methods/tests/vectors.rs

@@ -395,7 +395,7 @@ async fn rpc_getaddresstxids_invalid_arguments() {
         .unwrap_err();
     assert_eq!(
         error.message,
-        "End value is expected to be greater than or equal to start".to_string()
+        "start Height(2) must be less than or equal to end Height(1)".to_string()
     );
 
     // call the method with start equal zero
@@ -411,7 +411,7 @@ async fn rpc_getaddresstxids_invalid_arguments() {
         .unwrap_err();
     assert_eq!(
         error.message,
-        "Start and end are expected to be greater than zero".to_string()
+        "start Height(0) and end Height(1) must both be greater than zero".to_string()
     );
 
     // call the method outside the chain tip height
@@ -427,7 +427,7 @@ async fn rpc_getaddresstxids_invalid_arguments() {
         .unwrap_err();
     assert_eq!(
         error.message,
-        "Start or end is outside chain range".to_string()
+        "start Height(1) and end Height(11) must both be less than or equal to the chain tip Height(10)".to_string()
     );
 
     mempool.expect_no_requests().await;

zebra-state/src/arbitrary.rs

@@ -17,6 +17,8 @@ use crate::{
 
 /// Mocks computation done during semantic validation
 pub trait Prepare {
+    /// Runs block semantic validation computation, and returns the result.
+    /// Test-only method.
     fn prepare(self) -> PreparedBlock;
 }
 

zebra-state/src/lib.rs

@@ -16,7 +16,8 @@
 extern crate tracing;
 
 #[cfg(any(test, feature = "proptest-impl"))]
-mod arbitrary;
+pub mod arbitrary;
+
 mod config;
 pub mod constants;
 mod error;
@@ -39,7 +40,7 @@ pub use service::{
 
 #[cfg(any(test, feature = "proptest-impl"))]
 pub use service::{
-    arbitrary::populated_state,
+    arbitrary::{populated_state, CHAIN_TIP_UPDATE_WAIT_LIMIT},
     chain_tip::{ChainTipBlock, ChainTipSender},
     init_test, init_test_services,
 };

zebra-state/src/request.rs

@@ -381,20 +381,44 @@ pub enum Request {
     /// documentation for details.
     CommitBlock(PreparedBlock),
 
-    /// Commit a finalized block to the state, skipping all validation.
+    /// Commit a checkpointed block to the state, skipping most block validation.
     ///
     /// This is exposed for use in checkpointing, which produces finalized
     /// blocks. It is the caller's responsibility to ensure that the block is
-    /// valid and final. This request can be made out-of-order; the state service
-    /// will queue it until its parent is ready.
+    /// semantically valid and final. This request can be made out-of-order;
+    /// the state service will queue it until its parent is ready.
     ///
     /// Returns [`Response::Committed`] with the hash of the newly committed
     /// block, or an error.
     ///
     /// This request cannot be cancelled once submitted; dropping the response
     /// future will have no effect on whether it is eventually processed.
-    /// Duplicate requests should not be made, because it is the caller's
-    /// responsibility to ensure that each block is valid and final.
+    /// Duplicate requests will replace the older duplicate, and return an error
+    /// in its response future.
+    ///
+    /// # Note
+    ///
+    /// Finalized and non-finalized blocks are an internal Zebra implementation detail.
+    /// There is no difference between these blocks on the network, or in Zebra's
+    /// network or syncer implementations.
+    ///
+    /// # Consensus
+    ///
+    /// Checkpointing is allowed under the Zcash "social consensus" rules.
+    /// Zebra checkpoints both settled network upgrades, and blocks past the rollback limit.
+    /// (By the time Zebra release is tagged, its final checkpoint is typically hours or days old.)
+    ///
+    /// > A network upgrade is settled on a given network when there is a social consensus
+    /// > that it has activated with a given activation block hash. A full validator that
+    /// > potentially risks Mainnet funds or displays Mainnet transaction information to a user
+    /// > MUST do so only for a block chain that includes the activation block of the most
+    /// > recent settled network upgrade, with the corresponding activation block hash.
+    /// > ...
+    /// > A full validator MAY impose a limit on the number of blocks it will “roll back”
+    /// > when switching from one best valid block chain to another that is not a descendent.
+    /// > For `zcashd` and `zebra` this limit is 100 blocks.
+    ///
+    /// <https://zips.z.cash/protocol/protocol.pdf#blockchain>
     ///
     /// # Correctness
     ///