fix(tests): Checking if transactions get into the mempool fails in lightwalletd tests (#7644)

* copy zcash/lightwalletd service.proto

* uses correct assert & minor cleanup

* adds comment about checking service.proto file when

* adds workaround in send_transaction_test

* Update docs

Co-authored-by: Arya <aryasolhi@gmail.com>

---------

Co-authored-by: teor <teor@riseup.net>

.github/workflows/zcash-lightwalletd.yml

@@ -58,6 +58,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4.1.0
         with:
+          # Note: check service.proto when modifying lightwalletd repo
           repository: zcash/lightwalletd
           ref: 'master'
           persist-credentials: false

zebrad/tests/common/lightwalletd/proto/service.proto

@@ -36,7 +36,7 @@ message TxFilter {
 // by GetMempoolStream(), the latest block height.
 message RawTransaction {
     bytes data = 1;     // exact data returned by Zcash 'getrawtransaction'
-    int64 height = 2;  // height that the transaction was mined (or -1)
+    uint64 height = 2;  // height that the transaction was mined (or -1)
 }
 
 // A SendResponse encodes an error code and a string. It is currently used
@@ -110,11 +110,12 @@ message Exclude {
 
 // The TreeState is derived from the Zcash z_gettreestate rpc.
 message TreeState {
-    string network = 1; // "main" or "test"
+    string network = 1;     // "main" or "test"
-    uint64 height = 2;
+    uint64 height = 2;      // block height
-    string hash = 3;    // block id
+    string hash = 3;        // block id
-    uint32 time = 4;    // Unix epoch time when the block was mined
+    uint32 time = 4;        // Unix epoch time when the block was mined
-    string tree = 5;    // sapling commitment tree state
+    string saplingTree = 5; // sapling commitment tree state
+    string orchardTree = 6; // orchard commitment tree state
 }
 
 enum ShieldedProtocol {
@@ -122,16 +123,11 @@ enum ShieldedProtocol {
     orchard = 1;
 }
 
-// Request type for `GetSubtreeRoots`
 message GetSubtreeRootsArg {
-    uint32 startIndex = 1;                  // Index identifying where to start returning subtree roots.
+    uint32 startIndex = 1;                  // Index identifying where to start returning subtree roots
-    ShieldedProtocol shieldedProtocol = 2;  // Shielded protocol to return subtree roots for.
+    ShieldedProtocol shieldedProtocol = 2;  // Shielded protocol to return subtree roots for
     uint32 maxEntries = 3;                  // Maximum number of entries to return, or 0 for all entries.
 }
-
-
-// Response type for `GetSubtreeRoots`.
-// The actual response contains a stream of `SubtreeRoot`s.
 message SubtreeRoot {
     bytes rootHash = 2;                     // The 32-byte Merkle root of the subtree.
     bytes completingBlockHash = 3;          // The hash of the block that completed this subtree.
@@ -162,8 +158,12 @@ service CompactTxStreamer {
     rpc GetLatestBlock(ChainSpec) returns (BlockID) {}
     // Return the compact block corresponding to the given block identifier
     rpc GetBlock(BlockID) returns (CompactBlock) {}
+    // Same as GetBlock except actions contain only nullifiers
+    rpc GetBlockNullifiers(BlockID) returns (CompactBlock) {}
     // Return a list of consecutive compact blocks
     rpc GetBlockRange(BlockRange) returns (stream CompactBlock) {}
+    // Same as GetBlockRange except actions contain only nullifiers
+    rpc GetBlockRangeNullifiers(BlockRange) returns (stream CompactBlock) {}
 
     // Return the requested full (not compact) transaction (as from zcashd)
     rpc GetTransaction(TxFilter) returns (RawTransaction) {}
@@ -195,9 +195,10 @@ service CompactTxStreamer {
     // values also (even though they can be obtained using GetBlock).
     // The block can be specified by either height or hash.
     rpc GetTreeState(BlockID) returns (TreeState) {}
+    rpc GetLatestTreeState(Empty) returns (TreeState) {}
 
-    // Returns a stream of information about roots of subtrees of the Sapling
+    // Returns a stream of information about roots of subtrees of the Sapling and Orchard
-    // and Orchard note commitment trees.
+    // note commitment trees.
     rpc GetSubtreeRoots(GetSubtreeRootsArg) returns (stream SubtreeRoot) {}
 
     rpc GetAddressUtxos(GetAddressUtxosArg) returns (GetAddressUtxosReplyList) {}
@@ -207,4 +208,4 @@ service CompactTxStreamer {
     rpc GetLightdInfo(Empty) returns (LightdInfo) {}
     // Testing-only, requires lightwalletd --ping-very-insecure (do not enable in production)
     rpc Ping(Duration) returns (PingResponse) {}
-}
+}

zebrad/tests/common/lightwalletd/send_transaction_test.rs

@@ -84,8 +84,7 @@ pub async fn run() -> Result<()> {
         "running gRPC send transaction test using lightwalletd & zebrad",
     );
 
-    let mut transactions =
+    let transactions = load_transactions_from_future_blocks(network, test_type, test_name).await?;
-        load_transactions_from_future_blocks(network, test_type, test_name).await?;
 
     tracing::info!(
         transaction_count = ?transactions.len(),
@@ -148,8 +147,22 @@ pub async fn run() -> Result<()> {
 
     let mut rpc_client = connect_to_lightwalletd(lightwalletd_rpc_port).await?;
 
-    // To avoid filling the mempool queue, limit the transactions to be sent to the RPC and mempool queue limits
+    // Call GetMempoolTx so lightwalletd caches the empty mempool state.
-    transactions.truncate(max_sent_transactions());
+    // This is a workaround for a bug where lightwalletd will skip calling `get_raw_transaction`
+    // the first time GetMempoolTx is called because it replaces the cache early and only calls the
+    // RPC method for transaction ids that are missing in the old cache as keys.
+    // <https://github.com/zcash/lightwalletd/blob/master/frontend/service.go#L495-L502>
+    //
+    // TODO: Fix this issue in lightwalletd and delete this
+    rpc_client
+        .get_mempool_tx(Exclude { txid: vec![] })
+        .await?
+        .into_inner();
+
+    // Lightwalletd won't call `get_raw_mempool` again until 2 seconds after the last call
+    // <https://github.com/zcash/lightwalletd/blob/master/frontend/service.go#L482>
+    let sleep_until_lwd_last_mempool_refresh =
+        tokio::time::sleep(std::time::Duration::from_secs(2));
 
     let transaction_hashes: Vec<transaction::Hash> =
         transactions.iter().map(|tx| tx.hash()).collect();
@@ -160,9 +173,14 @@ pub async fn run() -> Result<()> {
         "connected gRPC client to lightwalletd, sending transactions...",
     );
 
+    let mut has_tx_with_shielded_elements = false;
     for transaction in transactions {
         let transaction_hash = transaction.hash();
 
+        // See <https://github.com/zcash/lightwalletd/blob/master/parser/transaction.go#L367>
+        has_tx_with_shielded_elements |= transaction.version() >= 4
+            && (transaction.has_shielded_inputs() || transaction.has_shielded_outputs());
+
         let expected_response = wallet_grpc::SendResponse {
             error_code: 0,
             error_message: format!("\"{transaction_hash}\""),
@@ -177,9 +195,14 @@ pub async fn run() -> Result<()> {
         assert_eq!(response, expected_response);
     }
 
-    // Check if some transaction is sent to mempool
+    // Check if some transaction is sent to mempool,
+    // Fails if there are only coinbase transactions in the first 50 future blocks
     tracing::info!("waiting for mempool to verify some transactions...");
     zebrad.expect_stdout_line_matches("sending mempool transaction broadcast")?;
+    // Wait for more transactions to verify, `GetMempoolTx` only returns txs where tx.HasShieldedElements()
+    // <https://github.com/zcash/lightwalletd/blob/master/frontend/service.go#L537>
+    tokio::time::sleep(std::time::Duration::from_secs(1)).await;
+    sleep_until_lwd_last_mempool_refresh.await;
 
     tracing::info!("calling GetMempoolTx gRPC to fetch transactions...");
     let mut transactions_stream = rpc_client
@@ -191,7 +214,7 @@ pub async fn run() -> Result<()> {
     zebrad.expect_stdout_line_matches("answered mempool request .*req.*=.*TransactionIds")?;
 
     // GetMempoolTx: make sure at least one of the transactions were inserted into the mempool.
-    let mut _counter = 0;
+    let mut counter = 0;
     while let Some(tx) = transactions_stream.message().await? {
         let hash: [u8; 32] = tx.hash.clone().try_into().expect("hash is correct length");
         let hash = transaction::Hash::from_bytes_in_display_order(&hash);
@@ -202,15 +225,15 @@ pub async fn run() -> Result<()> {
              in isolated mempool: {tx:?}",
         );
 
-        _counter += 1;
+        counter += 1;
     }
 
-    // TODO: This check sometimes works and sometimes it doesn't so we can't just enable it.
+    // TODO: Update `load_transactions_from_future_blocks()` to return block height offsets and,
-    // https://github.com/ZcashFoundation/zebra/issues/7529
+    //       only check if a transaction from the first block has shielded elements
-    //assert!(
+    assert!(
-    //    counter >= 1,
+        !has_tx_with_shielded_elements || counter >= 1,
-    //    "all transactions from future blocks failed to send to an isolated mempool"
+        "failed to read v4+ transactions with shielded elements from future blocks in mempool via lightwalletd"
-    //);
+    );
 
     // GetMempoolTx: make sure at least one of the transactions were inserted into the mempool.
     tracing::info!("calling GetMempoolStream gRPC to fetch transactions...");
@@ -222,13 +245,6 @@ pub async fn run() -> Result<()> {
         _counter += 1;
     }
 
-    // TODO: This check sometimes works and sometimes it doesn't so we can't just enable it.
-    // https://github.com/ZcashFoundation/zebra/issues/7529
-    //assert!(
-    //    counter >= 1,
-    //    "all transactions from future blocks failed to send to an isolated mempool"
-    //);
-
     Ok(())
 }
 
@@ -263,6 +279,6 @@ fn prepare_send_transaction_request(transaction: Arc<Transaction>) -> wallet_grp
 
     wallet_grpc::RawTransaction {
         data: transaction_bytes,
-        height: -1,
+        height: 0,
     }
 }

zebrad/tests/common/lightwalletd/wallet_grpc_test.rs

@@ -340,7 +340,7 @@ pub async fn run() -> Result<()> {
     assert_eq!(treestate.time, 1540779438);
     // Check that the note commitment tree is correct.
     assert_eq!(
-        treestate.tree,
+        treestate.sapling_tree,
         *zebra_test::vectors::SAPLING_TREESTATE_MAINNET_419201_STRING
     );