Auto merge of #4628 - therealyingtong:block-903002-bug, r=str4d

Pass HistoryNode through Rust FFI as a C array

`std::array<T>` is guaranteed to store `T` contiguously. However, there is
no guarantee that `sizeof(std::array<unsigned char, N>) == N`, which
prevents us from interpreting `std::array<std::array<unsigned char, N>, 32>`
as `&[[u8; N]]` on the Rust side of the FFI.

Instead, we define `HistoryNode` as a struct wrapping a C array, which
(as checked by `static_assert`) contains no padding.

This is equivalent to 82fe37d22b, which
fixed this issue when passing a slice of `HistoryEntry`s from C++ to Rust;
the bug fixed here is writing `HistoryNodes` from Rust into C++ memory.

src/coins.cpp

@@ -467,7 +467,7 @@ void CCoinsViewCache::PushHistoryNode(uint32_t epochId, const HistoryNode node)
         // special case, it just goes into the cache right away
         historyCache.Extend(node);
 
-        if (librustzcash_mmr_hash_node(epochId, node.data(), historyCache.root.begin()) != 0) {
+        if (librustzcash_mmr_hash_node(epochId, &node, historyCache.root.begin()) != 0) {
             throw std::runtime_error("hashing node failed");
         };
 
@@ -480,7 +480,7 @@ void CCoinsViewCache::PushHistoryNode(uint32_t epochId, const HistoryNode node)
     PreloadHistoryTree(epochId, false, entries, entry_indices);
 
     uint256 newRoot;
-    std::array<HistoryNode, 32> appendBuf;
+    std::array<HistoryNode, 32> appendBuf = {};
 
     uint32_t appends = librustzcash_mmr_append(
         epochId, 
@@ -488,9 +488,9 @@ void CCoinsViewCache::PushHistoryNode(uint32_t epochId, const HistoryNode node)
         entry_indices.data(),
         entries.data(),
         entry_indices.size(),
-        node.data(),
+        &node,
         newRoot.begin(),
-        appendBuf.data()->data()
+        appendBuf.data()
     );
 
     for (size_t i = 0; i < appends; i++) {
@@ -506,6 +506,7 @@ void CCoinsViewCache::PopHistoryNode(uint32_t epochId) {
 
     switch (historyCache.length) {
         case 0:
+        {
             // Caller is generally not expected to pop from empty tree! Caller
             // should switch to previous epoch and pop history from there.
 
@@ -517,22 +518,29 @@ void CCoinsViewCache::PopHistoryNode(uint32_t epochId) {
             // back.
             
             // Sensible action is to truncate the history cache:
+        }
         case 1:
+        {
             // Just resetting tree to empty
             historyCache.Truncate(0);
             historyCache.root = uint256();
             return;
+        }
         case 2:
+        {
             // - A tree with one leaf has length 1.
             // - A tree with two leaves has length 3.
             throw std::runtime_error("a history tree cannot have two nodes");
+        }
         case 3:
+        {
+            const HistoryNode tmpHistoryRoot = GetHistoryAt(epochId, 0);
             // After removing a leaf from a tree with two leaves, we are left
             // with a single-node tree, whose root is just the hash of that
             // node.
             if (librustzcash_mmr_hash_node(
                 epochId,
-                GetHistoryAt(epochId, 0).data(),
+                &tmpHistoryRoot,
                 newRoot.begin()
             ) != 0) {
                 throw std::runtime_error("hashing node failed");
@@ -540,7 +548,9 @@ void CCoinsViewCache::PopHistoryNode(uint32_t epochId) {
             historyCache.Truncate(1);
             historyCache.root = newRoot;
             return;
+        }
         default:
+        {
             // This is a non-elementary pop, so use the full tree logic.
             std::vector<HistoryEntry> entries;
             std::vector<uint32_t> entry_indices;
@@ -562,6 +572,7 @@ void CCoinsViewCache::PopHistoryNode(uint32_t epochId) {
             return;
         }
     }
+}
 
 template<typename Tree, typename Cache, typename CacheEntry>
 void CCoinsViewCache::AbstractPopAnchor(

src/rust/include/librustzcash.h

@@ -8,7 +8,16 @@
   #include <stdalign.h>
 #endif
 
-#define ENTRY_SERIALIZED_LENGTH 180
+#define NODE_SERIALIZED_LENGTH 171
+#define ENTRY_SERIALIZED_LENGTH (NODE_SERIALIZED_LENGTH + 9)
+
+typedef struct HistoryNode {
+    unsigned char bytes[NODE_SERIALIZED_LENGTH];
+}  HistoryNode;
+static_assert(
+    sizeof(HistoryNode) == NODE_SERIALIZED_LENGTH,
+    "HistoryNode struct is not the same size as the underlying byte array");
+static_assert(alignof(HistoryNode) == 1, "HistoryNode struct alignment is not 1");
 
 typedef struct HistoryEntry {
     unsigned char bytes[ENTRY_SERIALIZED_LENGTH];
@@ -331,9 +340,9 @@ extern "C" {
         const uint32_t *ni_ptr,
         const HistoryEntry *n_ptr,
         size_t p_len,
-        const unsigned char *nn_ptr,
+        const HistoryNode *nn_ptr,
         unsigned char *rt_ret,
-        unsigned char *buf_ret
+        HistoryNode *buf_ret
     );
 
     uint32_t librustzcash_mmr_delete(
@@ -348,7 +357,7 @@ extern "C" {
 
     uint32_t librustzcash_mmr_hash_node(
         uint32_t cbranch,
-        const unsigned char *n_ptr,
+        const HistoryNode *n_ptr,
         unsigned char *h_ret
     );
 

src/txdb.cpp

@@ -139,16 +139,21 @@ HistoryIndex CCoinsViewDB::GetHistoryLength(uint32_t epochId) const {
 }
 
 HistoryNode CCoinsViewDB::GetHistoryAt(uint32_t epochId, HistoryIndex index) const {
-    HistoryNode mmrNode;
+    HistoryNode mmrNode = {};
 
     if (index >= GetHistoryLength(epochId)) {
         throw runtime_error("History data inconsistent - reindex?");
     }
 
-    if (!db.Read(make_pair(DB_MMR_NODE, make_pair(epochId, index)), mmrNode)) {
+    // Read mmrNode into tmp std::array
+    std::array<unsigned char, NODE_SERIALIZED_LENGTH> tmpMmrNode;
+
+    if (!db.Read(make_pair(DB_MMR_NODE, make_pair(epochId, index)), tmpMmrNode)) {
         throw runtime_error("History data inconsistent (expected node not found) - reindex?");
     }
 
+    std::copy(std::begin(tmpMmrNode), std::end(tmpMmrNode), mmrNode.bytes);
+
     return mmrNode;
 }
 
@@ -205,7 +210,10 @@ void BatchWriteHistory(CDBBatch& batch, CHistoryCacheMap& historyCacheMap) {
 
         // replace/append new/updated entries
         for (auto it = historyCache.appends.begin(); it != historyCache.appends.end(); it++) {
-            batch.Write(make_pair(DB_MMR_NODE, make_pair(epochId, it->first)), it->second);
+            // Write mmrNode into tmp std::array
+            std::array<unsigned char, NODE_SERIALIZED_LENGTH> tmpMmrNode;
+            std::copy((it->second).bytes, (it->second).bytes + NODE_SERIALIZED_LENGTH, std::begin(tmpMmrNode));
+            batch.Write(make_pair(DB_MMR_NODE, make_pair(epochId, it->first)), tmpMmrNode);
         }
 
         // write new length

src/zcash/History.cpp

@@ -54,7 +54,7 @@ HistoryNode NewNode(
     )
 {
     CDataStream buf(SER_DISK, 0);
-    HistoryNode result;
+    HistoryNode result = {};
 
     buf << subtreeCommitment;
     buf << startTime;
@@ -68,7 +68,8 @@ HistoryNode NewNode(
     buf << COMPACTSIZE(endHeight);
     buf << COMPACTSIZE(saplingTxCount);
 
-    std::copy(buf.begin(), buf.end(), result.begin());
+    assert(buf.size() <= NODE_SERIALIZED_LENGTH);
+    std::copy(std::begin(buf), std::end(buf), result.bytes);
     return result;
 }
 
@@ -104,7 +105,11 @@ HistoryEntry NodeToEntry(const HistoryNode node, uint32_t left, uint32_t right)
     buf << code;
     buf << left;
     buf << right;
-    buf << node;
+
+    std::array<unsigned char, NODE_SERIALIZED_LENGTH> tmpMmrNode;
+    std::copy(node.bytes, node.bytes + NODE_SERIALIZED_LENGTH, std::begin(tmpMmrNode));
+
+    buf << tmpMmrNode;
 
     assert(buf.size() <= ENTRY_SERIALIZED_LENGTH);
     std::copy(std::begin(buf), std::end(buf), result.bytes);
@@ -118,7 +123,11 @@ HistoryEntry LeafToEntry(const HistoryNode node) {
 
     uint8_t code = 1;
     buf << code;
-    buf << node;
+
+    std::array<unsigned char, NODE_SERIALIZED_LENGTH> tmpMmrNode;
+    std::copy(node.bytes, node.bytes + NODE_SERIALIZED_LENGTH, std::begin(tmpMmrNode));
+
+    buf << tmpMmrNode;
 
     assert(buf.size() <= ENTRY_SERIALIZED_LENGTH);
     std::copy(std::begin(buf), std::end(buf), result.bytes);

src/zcash/History.hpp

@@ -13,10 +13,6 @@
 
 namespace libzcash {
 
-const int NODE_SERIALIZED_LENGTH = 171;
-
-typedef std::array<unsigned char, NODE_SERIALIZED_LENGTH> HistoryNode;
-
 typedef uint64_t HistoryIndex;
 
 class HistoryCache {
@@ -66,6 +62,5 @@ HistoryEntry LeafToEntry(const HistoryNode node);
 
 typedef libzcash::HistoryCache HistoryCache;
 typedef libzcash::HistoryIndex HistoryIndex;
-typedef libzcash::HistoryNode HistoryNode;
 
 #endif /* ZC_HISTORY_H_ */