Merge pull request #1473 from evoskuil/master

Rem get_hash() where result copied, check malleated32 in bypass.

include/bitcoin/system/chain/block.hpp

@@ -172,8 +172,6 @@ class BC_API block
     using hash_cref = std::reference_wrapper<const hash_digest>;
     using hash_hash = unique_hash_t<>;
 
-    using unordered_map_of_constant_referenced_points =
-        std::unordered_map<point_cref, output::cptr, point_hash>;
     using unordered_set_of_constant_referenced_points =
         std::unordered_set<point_cref, point_hash>;
     using unordered_set_of_constant_referenced_hashes =

include/bitcoin/system/data/memory.hpp

@@ -31,15 +31,6 @@ namespace system {
 
 BC_PUSH_WARNING(NO_THROW_IN_NOEXCEPT)
 
-// shared_ptr moves are avoided in vector population by using 'new' and passing
-// to shared_pointer construct a raw pointer via std::vector.emplace_back:
-// std::vector.emplace_back(new block{...}).
-// Otherwise emplace_back copies the shared pointer, just as would push_back:
-// std::vector.emplace_back(std::make_shared<block>(block{...})).
-// Vector emplace constructs the instance using its allocator, invoking new.
-// So in this case it constructs the shared_pointer which accepts the raw
-// pointer as its constructor argument, passed by std::vector.emplace_back.
-
 /// shared_ptr
 /// ---------------------------------------------------------------------------
 
@@ -75,10 +66,7 @@ inline std::shared_ptr<const Type> to_shared(const Type& value) NOEXCEPT
 template <typename Type, typename... Args>
 inline std::shared_ptr<const Type> to_shared(Args&&... values) NOEXCEPT
 {
-    BC_PUSH_WARNING(NO_NEW_OR_DELETE)
-    return std::shared_ptr<const Type>(
-        new const Type(std::forward<Args>(values)...));
-    BC_POP_WARNING()
+    return std::make_shared<const Type>(Type{ std::forward<Args>(values)... });
 }
 
 /// Create shared pointer to vector of const shared pointers from moved vector.
@@ -108,6 +96,13 @@ inline std::unique_ptr<const Type> to_unique(const Type& value) NOEXCEPT
     return std::make_unique<const Type>(value);
 }
 
+/// Construct unique pointer to const from moved constructor parameters.
+template <typename Type, typename... Args>
+inline std::unique_ptr<const Type> to_unique(Args&&... values) NOEXCEPT
+{
+    return std::make_unique<const Type>(Type{ std::forward<Args>(values)... });
+}
+
 BC_POP_WARNING()
 
 } // namespace system

include/bitcoin/system/error/block_error_t.hpp

@@ -55,6 +55,7 @@ enum block_error_t : uint8_t
     forward_reference,
     merkle_mismatch,
     block_legacy_sigop_limit,
+    type32_malleated,
 
     // accept block
     block_non_final,

src/chain/block.cpp

@@ -145,11 +145,7 @@ block block::from_data(reader& source, bool witness) NOEXCEPT
         txs->reserve(capacity);
 
         for (size_t tx = 0; tx < capacity; ++tx)
-        {
-            BC_PUSH_WARNING(NO_NEW_OR_DELETE)
-            txs->emplace_back(new transaction{ source, witness });
-            BC_POP_WARNING()
-        }
+            txs->push_back(to_shared<transaction>(source, witness));
 
         // This is a pointer copy (non-const to const).
         return txs;
@@ -426,7 +422,7 @@ bool block::is_hash_limit_exceeded() const NOEXCEPT
         return false;
 
     // A set is used to collapse duplicates.
-    unordered_set_of_constant_referenced_hashes hashes;
+    unordered_set_of_constant_referenced_hashes hashes{};
 
     // Just the coinbase tx hash, skip its null input hashes.
     hashes.emplace(txs_->front()->get_hash(false));
@@ -471,8 +467,7 @@ size_t block::malleated32_size() const NOEXCEPT
 {
     const auto malleated = txs_->size();
     for (auto mally = one; mally <= to_half(malleated); mally *= two)
-        if (is_malleable32(malleated - mally, mally) &&
-            is_malleated32(mally))
+        if (is_malleable32(malleated - mally, mally) && is_malleated32(mally))
             return mally;
 
     return zero;
@@ -632,21 +627,21 @@ bool block::is_unspent_coinbase_collision() const NOEXCEPT
 // Search is not ordered, forward references are caught by block.check.
 void block::populate() const NOEXCEPT
 {
-    unordered_map_of_constant_referenced_points points{};
+    std::unordered_map<point, output::cptr> points{};
     uint32_t index{};
 
     // Populate outputs hash table.
     for (auto tx = txs_->begin(); tx != txs_->end(); ++tx, index = 0)
         for (const auto& out: *(*tx)->outputs_ptr())
-            points.emplace(std::pair{ point{ (*tx)->get_hash(false),
-                index++ }, out });
+            points.emplace(std::pair{ point{ (*tx)->hash(false), index++ },
+                out });
 
     // Populate input prevouts from hash table.
     for (auto tx = txs_->begin(); tx != txs_->end(); ++tx)
     {
         for (const auto& in: *(*tx)->inputs_ptr())
         {
-            const auto point = points.find(std::cref(in->point()));
+            const auto point = points.find(in->point());
             if (point != points.end())
                 in->prevout = point->second;
         }
@@ -723,15 +718,22 @@ code block::confirm_transactions(const context& ctx) const NOEXCEPT
 // ----------------------------------------------------------------------------
 // The block header is checked/accepted independently.
 
+// context free.
+// TODO: use of get_hash() in is_forward_reference makes this thread unsafe.
 code block::check(bool bypass) const NOEXCEPT
 {
     if (bypass)
     {
-        return is_invalid_merkle_root() ? error::merkle_mismatch :
-            error::block_success;
+        // type32_malleated is subset of is_internal_double_spend, assuming
+        // otherwise valid txs, as that will catch any duplicated transaction.
+        if (is_invalid_merkle_root())
+            return error::merkle_mismatch;
+        if (is_malleated32())
+            return error::type32_malleated;
+
+        return error::block_success;
     }
 
-    // context free.
     // empty_block is redundant with first_not_coinbase.
     //if (is_empty())
     //    return error::empty_block;
@@ -756,20 +758,24 @@ code block::check(bool bypass) const NOEXCEPT
 // timestamp
 // median_time_past
 
+// context required.
+// TODO: use of get_hash() in is_hash_limit_exceeded makes this thread unsafe.
 code block::check(const context& ctx, bool bypass) const NOEXCEPT
 {
     if (bypass)
     {
         const auto bip141 = ctx.is_enabled(bip141_rule);
-        return bip141 && is_invalid_witness_commitment() ?
-            error::invalid_witness_commitment : error::block_success;
+
+        if (bip141 && is_invalid_witness_commitment())
+            return error::invalid_witness_commitment;
+
+        return error::block_success;
     }
 
     const auto bip34 = ctx.is_enabled(bip34_rule);
     const auto bip50 = ctx.is_enabled(bip50_rule);
     const auto bip141 = ctx.is_enabled(bip141_rule);
 
-    // context required.
     if (bip141 && is_overweight())
         return error::block_weight_limit;
     if (bip34 && is_invalid_coinbase_script(ctx.height))

src/chain/transaction.cpp

@@ -251,11 +251,7 @@ read_puts(Source& source) NOEXCEPT
 
     puts->reserve(capacity);
     for (auto put = zero; put < capacity; ++put)
-    {
-        BC_PUSH_WARNING(NO_NEW_OR_DELETE)
-        puts->emplace_back(new Put{ source });
-        BC_POP_WARNING()
-    }
+        puts->push_back(to_shared<Put>(source));
 
     // This is a pointer copy (non-const to const).
     return puts;
@@ -860,19 +856,16 @@ hash_digest transaction::unversioned_signature_hash(
 // TODO: taproot requires both single and double hash of each.
 void transaction::initialize_sighash_cache() const NOEXCEPT
 {
-    // This overconstructs the cache (anyone or !all), however it is simple and
-    // the same criteria applied by satoshi.
-    if (segregated_)
-    {
-        BC_PUSH_WARNING(NO_NEW_OR_DELETE)
-        sighash_cache_.reset(new sighash_cache
-        {
-            outputs_hash(),
-            points_hash(),
-            sequences_hash()
-        });
-        BC_POP_WARNING()
-    }
+    // This overconstructs the cache (anyone or !all), however it is simple.
+    if (!segregated_)
+        return;
+
+    sighash_cache_ = to_unique<sighash_cache>
+    (
+        outputs_hash(),
+        points_hash(),
+        sequences_hash()
+    );
 }
 
 // private

src/error/block_error_t.cpp

@@ -49,6 +49,7 @@ DEFINE_ERROR_T_MESSAGE_MAP(block_error)
     { forward_reference, "transactions out of order" },
     { merkle_mismatch, "merkle root mismatch" },
     { block_legacy_sigop_limit, "too many block legacy signature operations" },
+    { type32_malleated, "block is type32 malleated" },
 
     // accept block
     { block_non_final, "block contains a non-final transaction" },

test/chain/transaction.cpp

@@ -1149,7 +1149,7 @@ BOOST_AUTO_TEST_CASE(transaction__is_missing_prevouts__default_inputs__true)
 BOOST_AUTO_TEST_CASE(transaction__is_missing_prevouts__valid_prevout__false)
 {
     const input input{ { hash_digest{}, 42 }, {}, 0 };
-    input.prevout = to_shared<output>(42, script{});
+    input.prevout = to_shared<output>(42_u64, script{});
     const accessor instance
     {
         0,

test/error/block_error_t.cpp

@@ -172,6 +172,15 @@ BOOST_AUTO_TEST_CASE(block_error_t__code__block_legacy_sigop_limit__true_exected
     BOOST_REQUIRE_EQUAL(ec.message(), "too many block legacy signature operations");
 }
 
+BOOST_AUTO_TEST_CASE(block_error_t__code__type32_malleated__true_exected_message)
+{
+    constexpr auto value = error::type32_malleated;
+    const auto ec = code(value);
+    BOOST_REQUIRE(ec);
+    BOOST_REQUIRE(ec == value);
+    BOOST_REQUIRE_EQUAL(ec.message(), "block is type32 malleated");
+}
+
 // accept block
 
 BOOST_AUTO_TEST_CASE(block_error_t__code__block_non_final__true_exected_message)