Merge pull request #1805 from evoskuil/master

Fix/update tx confirm & metadata for use in network.

Author: evoskuil

include/bitcoin/system/chain/block.hpp

@@ -198,9 +198,6 @@ class BC_API block
     bool is_signature_operations_limited(bool bip16,
         bool bip141) const NOEXCEPT;
 
-    /// Requires input.metadata.spent (prevout confirmation).
-    bool is_unspent_coinbase_collision() const NOEXCEPT;
-
 private:
     typedef struct { size_t nominal; size_t witnessed; } sizes;
 

include/bitcoin/system/chain/input.hpp

@@ -113,7 +113,7 @@ class BC_API input
     /// Assumes coinbase if prevout not populated (returns only legacy sigops).
     size_t signature_operations(bool bip16, bool bip141) const NOEXCEPT;
 
-    /// Requires metadata.height and median_time_past (otherwise returns true).
+    /// Requires metadata.prevout_height and median_time_past (otherwise true).
     bool is_relative_locked(size_t height,
         uint32_t median_time_past) const NOEXCEPT;
 
@@ -156,9 +156,8 @@ class BC_API input
 
 public:
     /// Public mutable metadata access, copied but not compared for equality.
-    /// Defaults are set so non-population issues usually imply invalidity.
     mutable chain::output::cptr prevout{};
-    mutable chain::prevout metadata{ zero, max_uint32, true, true, true };
+    mutable chain::prevout metadata{};
 };
 
 typedef std_vector<input> inputs;

include/bitcoin/system/chain/prevout.hpp

@@ -35,12 +35,13 @@ class BC_API prevout final
     ///************************************************************************
     union
     {
-        /// The confirmed chain height of the prevout (zero if not found).
-        /// Unused if the input owning this prevout is null (coinbase).
-        size_t height;
+        /// Unused for coinbase.
+        /// The confirmed height of the prevout or max_uint32.
+        size_t prevout_height{ max_uint32 };
 
-        /// node: populated with a database identifier for the parent tx.
-        uint32_t parent{ zero };
+        /// node: populated with database identifier for the parent tx.
+        /// Link::terminal must derive from default of max_uint32.
+        uint32_t parent_tx;
     };
 
     ///************************************************************************
@@ -49,35 +50,27 @@ class BC_API prevout final
     ///************************************************************************
     union
     {
-        /// The median time past at height (max_uint32 if not found/confirmed).
-        /// Unused if the input owning this prevout is null (coinbase).
+        /// Unused for coinbase.
+        /// The median time past at confirmed prevout block or max_uint32.
         uint32_t median_time_past{ max_uint32 };
 
-        /// node: set to the database record of the input association.
-        /// node: it is necessary that Link::terminal derives from max_uint32.
-        uint32_t link;
+        /// node: populated with database identifier for the input/point.
+        /// Link::terminal must derive from default of max_uint32.
+        uint32_t point_link;
     };
 
     ///************************************************************************
     /// CONSENSUS: 
     /// An unspent coinbase collision is immature (unspendable) and spent
     /// collision is mature [bip30]. CB collision presumed precluded by bip34.
+    /// This is NOT guarded by system::chain confirmation checks.
     ///************************************************************************
-    union
-    {
-        /// If input owning this prevout is null (coinbase), this implies that
-        /// all outputs of any duplicate txs are fully spent at height.
-        /// If the input owning this prevout is not null (not coinbase), this
-        /// indicates whether the prevout is spent at height (double spend).
-        bool spent{ true };
-
-        /// node: indicates that the input spends output inside same block.
-        bool inside;
-    };
+    /// The confirmed height of the spender or max_uint32.
+    uint32_t spender_height{ max_uint32 };
 
-    /// node: set via block.populate() as internal spends do not
-    /// require prevout block association for relative locktime checks.
-    /// So median_time_past is not required as locked is determined here.
+    /// node: set via block.populate() as internal spends do not require
+    /// prevout block association for relative locktime checks. So
+    /// median_time_past is not required as locked is determined here.
     bool locked{ true };
 
     /// The previous output is of a coinbase transaction.

include/bitcoin/system/chain/transaction.hpp

@@ -224,17 +224,17 @@ class BC_API transaction
     /// Confirm (requires confirmation order).
     /// -----------------------------------------------------------------------
 
-    /// Requires input.metadata.height/median_time_past (prevout confirmation).
+    /// Requires input.metadata.prevout_height/median_time_past.
     bool is_relative_locked(size_t height,
         uint32_t median_time_past) const NOEXCEPT;
 
-    /// Requires input.metadata.height (prevout confirmation).
+    /// Requires input.metadata.prevout_height.
     bool is_immature(size_t height) const NOEXCEPT;
 
-    /// Requires input.metadata.height (prevout confirmation).
+    /// Requires input.metadata.prevout_height.
     bool is_unconfirmed_spend(size_t height) const NOEXCEPT;
 
-    /// Requires input.metadata.height/spent (prevout confirmation).
+    /// Requires input.metadata.spender_height.
     bool is_confirmed_double_spend(size_t height) const NOEXCEPT;
 
 private:

src/chain/block.cpp

@@ -139,7 +139,7 @@ void block::assign_data(reader& source, bool witness) NOEXCEPT
     auto txs = to_non_const_raw_ptr(txs_);
     txs->reserve(count);
 
-    for (size_t tx = 0; tx < count; ++tx)
+    for (size_t tx{}; tx < count; ++tx)
         txs->emplace_back(CREATE(transaction, allocator, source, witness));
 
     size_ = serialized_size(*txs_);
@@ -750,20 +750,6 @@ bool block::is_signature_operations_limited(bool bip16,
     return signature_operations(bip16, bip141) > limit;
 }
 
-//*****************************************************************************
-// CONSENSUS: check excluded under bip30 exception blocks and bip30_deactivate
-// until bip30_reactivate. Those conditions are rolled up into the bip30 flag.
-//*****************************************************************************
-bool block::is_unspent_coinbase_collision() const NOEXCEPT
-{
-    if (txs_->empty() || txs_->front()->inputs_ptr()->empty())
-        return false;
-
-    // May only commit duplicate coinbase that is already confirmed spent.
-    // Metadata population defaults coinbase to spent (not a collision).
-    return !txs_->front()->inputs_ptr()->front()->metadata.spent;
-}
-
 // Search is unordered, forward refs (and duplicates) caught by block.check.
 void block::populate() const NOEXCEPT
 {
@@ -798,6 +784,8 @@ code block::populate_with_metadata(const chain::context& ctx) const NOEXCEPT
     if (txs_->empty())
         return error::block_success;
 
+    const auto& self = txs_->front()->get_hash(false);
+    constexpr auto matures = !is_zero(coinbase_maturity);
     const auto bip68 = ctx.is_enabled(chain::flags::bip68_rule);
     unordered_map_of_cref_point_to_output_cptr_cref points(outputs());
     uint32_t index{};
@@ -807,27 +795,24 @@ code block::populate_with_metadata(const chain::context& ctx) const NOEXCEPT
         for (const auto& out: *(*tx)->outputs_ptr())
             points.emplace(cref_point{ (*tx)->get_hash(false), index++ }, out);
 
-    // Populate input prevouts from hash table and obtain maturity.
+    // Populate prevouts from hash table, determine get locked and maturity.
     for (auto tx = std::next(txs_->begin()); tx != txs_->end(); ++tx)
     {
         for (const auto& in: *(*tx)->inputs_ptr())
         {
             // Map chain::point to cref_point for search, should optimize away.
-            const auto point = points.find({ in->point().hash(),
-                in->point().index() });
+            const cref_point key{ in->point().hash(), in->point().index() };
 
-            if (point != points.end())
+            if (const auto it = points.find(key); it != points.end())
             {
-                // Zero maturity coinbase spend is immature.
+                const auto immature = (matures && in->point().hash() == self);
                 const auto lock = (bip68 && (*tx)->is_internally_locked(*in));
-                const auto immature = !is_zero(coinbase_maturity) &&
-                    (in->point().hash() == txs_->front()->get_hash(false));
+                in->prevout = it->second;
 
-                in->prevout = point->second;
+                // If invalid shortcircuit population and return above error.
                 if ((in->metadata.locked = (immature || lock)))
                 {
-                    // Shortcircuit population and return above error.
-                    return immature ? error::coinbase_maturity : 
+                    return immature ? error::coinbase_maturity :
                         error::relative_time_locked;
                 }
             }
@@ -996,11 +981,6 @@ code block::accept(const context& ctx, size_t subsidy_interval,
 // This assumes that prevout and metadata caching are completed on all inputs.
 code block::confirm(const context& ctx) const NOEXCEPT
 {
-    const auto bip30 = ctx.is_enabled(bip30_rule);
-
-    if (bip30 && is_unspent_coinbase_collision())
-        return error::unspent_coinbase_collision;
-
     return confirm_transactions(ctx);
 }
 

src/chain/input.cpp

@@ -427,9 +427,8 @@ bool input::is_relative_locked(size_t height,
     uint32_t median_time_past) const NOEXCEPT
 {
     // Prevout must be found and height/median_time_past metadata populated.
-    ////BC_ASSERT(!is_zero(metadata.height));
     return is_relative_locked(sequence_, height, median_time_past,
-        metadata.height, metadata.median_time_past);
+        metadata.prevout_height, metadata.median_time_past);
 }
 
 bool input::reserved_hash(hash_cref& out) const NOEXCEPT

src/chain/transaction.cpp

@@ -716,8 +716,8 @@ bool transaction::is_immature(size_t height) const NOEXCEPT
     const auto mature = [=](const auto& input) NOEXCEPT
     {
         return input->metadata.coinbase ?
-            is_coinbase_mature(input->metadata.height, height) :
-            is_non_coinbase_mature(input->metadata.height, height);
+            is_coinbase_mature(input->metadata.prevout_height, height) :
+            is_non_coinbase_mature(input->metadata.prevout_height, height);
     };
 
     return !std::all_of(inputs_->begin(), inputs_->end(), mature);
@@ -744,7 +744,7 @@ bool transaction::is_internally_locked(const input& in) const NOEXCEPT
         return false;
 
     // Internal spends have no relative height/mtp (own metadata vs. itself).
-    return in.is_relative_locked(in.metadata.height,
+    return in.is_relative_locked(in.metadata.prevout_height,
         in.metadata.median_time_past);
 }
 
@@ -767,18 +767,15 @@ bool transaction::is_relative_locked(size_t height,
     return std::any_of(inputs_->begin(), inputs_->end(), locked);
 }
 
-// Spends internal to a block are handled by block validation.
 bool transaction::is_unconfirmed_spend(size_t height) const NOEXCEPT
 {
     BC_ASSERT(!is_coinbase());
 
-    // Zero is either genesis or not found.
-    // Test maturity first to obtain proper error code.
-    // Spends internal to a block are handled by block validation.
     const auto unconfirmed = [=](const auto& input) NOEXCEPT
     {
-        const auto prevout_height = input->metadata.height;
-        return is_zero(prevout_height) && !(height > prevout_height);
+        // Spends internal to a block are handled by block validation.
+        // The lack of equality check here prevents conflict with self.
+        return height < input->metadata.prevout_height;
     };
 
     return std::any_of(inputs_->begin(), inputs_->end(), unconfirmed);
@@ -788,10 +785,11 @@ bool transaction::is_confirmed_double_spend(size_t height) const NOEXCEPT
 {
     BC_ASSERT(!is_coinbase());
 
-    // Spends internal to a block are handled by block validation.
     const auto spent = [=](const auto& input) NOEXCEPT
     {
-        return input->metadata.spent && height > input->metadata.height;
+        // Spends internal to a block are handled by block validation.
+        // The lack of equality check here prevents conflict with self.
+        return height > input->metadata.spender_height;
     };
 
     return std::any_of(inputs_->begin(), inputs_->end(), spent);

src/chain/transaction_sighash.cpp

@@ -94,7 +94,7 @@ void transaction::signature_hash_single(writer& sink,
         const auto index = input_index(input);
         sink.write_variable(add1(index));
 
-        for (size_t output = 0; output < index; ++output)
+        for (size_t output{}; output < index; ++output)
             sink.write_bytes(null_output());
 
         // Guarded by unversioned_sighash().

src/chain/witness.cpp

@@ -157,7 +157,7 @@ void witness::skip(reader& source, bool prefix) NOEXCEPT
     {
         const auto count = source.read_size(max_block_weight);
 
-        for (size_t element = 0; element < count; ++element)
+        for (size_t element{}; element < count; ++element)
             source.read_bytes(source.read_size(max_block_weight));
     }
     else
@@ -191,7 +191,7 @@ void witness::assign_data(reader& source, bool prefix) NOEXCEPT
         const auto count = source.read_size(max_block_weight);
         stack_.reserve(count);
 
-        for (size_t element = 0; element < count; ++element)
+        for (size_t element{}; element < count; ++element)
             if (!push_witness())
                 break;
     }

test/chain/block.cpp

@@ -74,78 +74,19 @@ class accessor
 public:
     // Use base class constructors.
     using block::block;
-
-    bool is_empty() const NOEXCEPT
-    {
-        return block::is_empty();
-    }
-
-    bool is_oversized() const NOEXCEPT
-    {
-        return block::is_oversized();
-    }
-
-    bool is_first_non_coinbase() const NOEXCEPT
-    {
-        return block::is_first_non_coinbase();
-    }
-
-    bool is_extra_coinbases() const NOEXCEPT
-    {
-        return block::is_extra_coinbases();
-    }
-
-    bool is_forward_reference() const NOEXCEPT
-    {
-        return block::is_forward_reference();
-    }
-
-    bool is_internal_double_spend() const NOEXCEPT
-    {
-        return block::is_internal_double_spend();
-    }
-
-    bool is_invalid_merkle_root() const NOEXCEPT
-    {
-        return block::is_invalid_merkle_root();
-    }
-
-    bool is_overweight() const NOEXCEPT
-    {
-        return block::is_overweight();
-    }
-
-    bool is_invalid_coinbase_script(size_t height) const NOEXCEPT
-    {
-        return block::is_invalid_coinbase_script(height);
-    }
-
-    bool is_hash_limit_exceeded() const NOEXCEPT
-    {
-        return block::is_hash_limit_exceeded();
-    }
-
-    bool is_invalid_witness_commitment() const NOEXCEPT
-    {
-        return block::is_invalid_witness_commitment();
-    }
-
-    bool is_overspent(size_t height, uint64_t subsidy_interval,
-        uint64_t initial_block_subsidy_satoshi, bool bip42) const NOEXCEPT
-    {
-        return block::is_overspent(height, subsidy_interval,
-            initial_block_subsidy_satoshi, bip42);
-    }
-
-    size_t is_signature_operations_limited(bool bip16, bool bip141) const NOEXCEPT
-    {
-        return block::is_signature_operations_limited(bip16, bip141);
-    }
-
-    bool is_unspent_coinbase_collision() const NOEXCEPT
-    {
-        return block::is_unspent_coinbase_collision();
-    }
+    using block::is_empty;
+    using block::is_oversized;
+    using block::is_first_non_coinbase;
+    using block::is_extra_coinbases;
+    using block::is_forward_reference;
+    using block::is_internal_double_spend;
+    using block::is_invalid_merkle_root;
+    using block::is_overweight;
+    using block::is_invalid_coinbase_script;
+    using block::is_hash_limit_exceeded;
+    using block::is_invalid_witness_commitment;
+    using block::is_overspent;
+    using block::is_signature_operations_limited;
 };
 
 // constructors