Merge pull request #1164 from AntelopeIO/validate_action_mroot

Validate finality mroot of Savanna blocks as a part of header validation

Author: linh2931

libraries/chain/controller.cpp

@@ -3959,7 +3959,7 @@ struct controller_impl {
    // Called from net-threads. It is thread safe as signed_block is never modified after creation.
    // -----------------------------------------------------------------------------
    std::optional<qc_t> verify_basic_proper_block_invariants( const block_id_type& id, const signed_block_ptr& b,
-                                                             const block_header_state& prev ) {
+                                                             const block_state& prev ) {
       assert(b->is_proper_svnn_block());
 
       auto qc_ext_id = quorum_certificate_extension::extension_id();
@@ -4054,6 +4054,20 @@ struct controller_impl {
                   "QC is_strong (${s1}) in block extension does not match is_strong_qc (${s2}) in header extension. Block number: ${b}",
                   ("s1", qc_proof.is_strong())("s2", new_qc_claim.is_strong_qc)("b", block_num) );
 
+      // `valid` structure can be modified while this function is running on net thread.
+      // Use is_valid() instead. It uses atomic `validated` and when it is true, `valid`
+      // has been constructed.
+      if (prev.is_valid()) {
+         assert(prev.valid);
+
+         // compute finality mroot using previous block state and new qc claim
+         auto        computed_finality_mroot = prev.get_finality_mroot_claim(new_qc_claim);
+         const auto& supplied_finality_mroot  = b->action_mroot;
+         EOS_ASSERT( computed_finality_mroot == supplied_finality_mroot, block_validate_exception,
+                     "computed finality mroot (${computed}) does not match supplied finality mroot ${supplied} by header extension. Block number: ${b}, block id: ${id}",
+                     ("computed", computed_finality_mroot)("supplied", supplied_finality_mroot)("b", block_num)("id", id) );
+      }
+
       return std::optional{qc_proof};
    }
 

unittests/block_tests.cpp

@@ -421,4 +421,28 @@ BOOST_FIXTURE_TEST_CASE( invalid_qc_claim_block_num_test, validating_tester ) {
                            }) ;
 }
 
+// Verify that a block with an invalid action mroot is rejected
+BOOST_FIXTURE_TEST_CASE( invalid_action_mroot_test, tester )
+{
+   // Create a block with transaction
+   create_account("newacc"_n);
+   auto b = produce_block();
+
+   // Make a copy of the block and corrupt its action mroot
+   auto copy_b = b->clone();
+   copy_b->action_mroot = digest_type::hash("corrupted");
+
+   // Re-sign the block
+   copy_b->producer_signature = get_private_key(config::system_account_name, "active").sign(copy_b->calculate_id());
+
+   // Push the block containing corruptted action mroot. It should fail
+   BOOST_REQUIRE_EXCEPTION(push_block(signed_block::create_signed_block(std::move(copy_b))),
+                           fc::exception,
+                           [] (const fc::exception &e)->bool {
+                              return e.code() == block_validate_exception::code_value &&
+                                     e.to_detail_string().find("computed finality mroot") != std::string::npos &&
+                                     e.to_detail_string().find("does not match supplied finality mroot") != std::string::npos;
+                           });
+}
+
 BOOST_AUTO_TEST_SUITE_END()

unittests/forked_tests_savanna.cpp

@@ -115,8 +115,10 @@ BOOST_FIXTURE_TEST_CASE(fork_with_bad_block_savanna, savanna_cluster::cluster_t)
          if (j <= i) {
             auto copy_b = b->clone();
             if (j == i) {
-               // corrupt this block (forks[j].blocks[j] is corrupted)
-               copy_b->action_mroot._hash[0] ^= 0x1ULL;
+               // Corrupt this block (forks[j].blocks[j] is corrupted).
+               // Do not corrupt the block by modifying action_mroot, as action_mroot is checked
+               // by block header validation, _nodes[0].push_block(b) would fail.
+               copy_b->confirmed++;
             } else if (j < i) {
                // link to a corrupted chain (fork.blocks[j] was corrupted)
                copy_b->previous = fork.blocks.back()->calculate_id();
@@ -177,7 +179,7 @@ BOOST_FIXTURE_TEST_CASE(fork_with_bad_block_savanna, savanna_cluster::cluster_t)
 
          // push the block which should attempt the corrupted fork and fail
          BOOST_REQUIRE_EXCEPTION(_nodes[0].push_block(fork.blocks.back()), fc::exception,
-                                 fc_exception_message_starts_with( "finality_mroot does not match"));
+                                 fc_exception_message_starts_with( "Block ID does not match"));
          BOOST_REQUIRE_EQUAL(_nodes[0].head().id(), node0_head);
       }
    }