Merge pull request libbitcoin#1503 from evoskuil/master

Comments, refactor to clean up from_data optimizations.

include/bitcoin/system/chain/block.hpp

@@ -188,7 +188,7 @@ class BC_API block
         std::unordered_set<hash_cref, hash_hash>;
 
     void assign_data(reader& source, bool witness) NOEXCEPT;
-    ////static block from_data(reader& source, bool witness) NOEXCEPT;
+    static block from_data(reader& source, bool witness) NOEXCEPT;
     static sizes serialized_size(const chain::transaction_cptrs& txs) NOEXCEPT;
 
     // context free

include/bitcoin/system/chain/header.hpp

@@ -140,7 +140,6 @@ class BC_API header
 
 private:
     void assign_data(reader& source) NOEXCEPT;
-    ////static header from_data(reader& source) NOEXCEPT;
 
     // Header should be stored as shared (adds 16 bytes).
     // copy: 4 * 32 + 2 * 256 + 1 = 81 bytes (vs. 16 when shared).
@@ -153,7 +152,6 @@ class BC_API header
     bool valid_;
 
     // Identity hash caching.
-    // TODO: use std::optional to avoid this pointer allocation.
     mutable std::shared_ptr<const hash_digest> hash_{};
 };
 

include/bitcoin/system/chain/input.hpp

@@ -124,31 +124,22 @@ class BC_API input
         bool valid) NOEXCEPT;
 
 private:
-    // So that witness may be set late in deserialization.
-    friend class transaction;
-    size_t nominal_size() const NOEXCEPT;
-    size_t witnessed_size() const NOEXCEPT;
-    void set_witness(reader& source) NOEXCEPT;
-
-    inline const chain::witness& get_witness() const NOEXCEPT
-    {
-        return witness_ ? *witness_ : no_witness();
-    }
-
-    inline const chain::witness::cptr& get_witness_cptr() const NOEXCEPT
-    {
-        return witness_ ? witness_ : no_witness_cptr();
-    }
-
     typedef struct { size_t nominal; size_t witnessed; } sizes;
 
-    ////void assign_data(reader& source) NOEXCEPT;
-    ////static input from_data(reader& source) NOEXCEPT;
     static sizes serialized_size(const chain::script& script) NOEXCEPT;
     static sizes serialized_size(const chain::script& script,
         const chain::witness& witness) NOEXCEPT;
     static const chain::witness& no_witness() NOEXCEPT;
     static const chain::witness::cptr& no_witness_cptr() NOEXCEPT;
+
+    // So that witness may be set late in deserialization.
+    friend class transaction;
+    size_t nominal_size() const NOEXCEPT;
+    size_t witnessed_size() const NOEXCEPT;
+    void set_witness(reader& source) NOEXCEPT;
+
+    const chain::witness& get_witness() const NOEXCEPT;
+    const chain::witness::cptr& get_witness_cptr() const NOEXCEPT;
     bool extract_sigop_script(chain::script& out,
         const chain::script& prevout_script) const NOEXCEPT;
 

include/bitcoin/system/chain/operation.hpp

@@ -158,27 +158,6 @@ class BC_API operation
     friend class script;
     static bool count_op(reader& source) NOEXCEPT;
 
-    inline bool data_empty() const NOEXCEPT
-    {
-        return !data_ || data_->empty();
-    }
-
-    inline size_t data_size() const NOEXCEPT
-    {
-        return data_ ? data_->size() : zero;
-    }
-
-    inline const data_chunk& get_data() const NOEXCEPT
-    {
-        return data_ ? *data_ : no_data();
-    }
-
-    inline const chunk_cptr& get_data_cptr() const NOEXCEPT
-    {
-        return data_ ? data_ : no_data_cptr();
-    }
-
-    void assign_data(reader& source) NOEXCEPT;
     static operation from_data(reader& source) NOEXCEPT;
     static operation from_push_data(const chunk_cptr& data,
         bool minimal) NOEXCEPT;
@@ -190,14 +169,19 @@ class BC_API operation
     static const chunk_cptr& no_data_cptr() NOEXCEPT;
     static const chunk_cptr& any_data_cptr() NOEXCEPT;
     static uint32_t read_data_size(opcode code, reader& source) NOEXCEPT;
-
     static inline opcode opcode_from_data(const data_chunk& push_data,
         bool minimal) NOEXCEPT
     {
         return minimal ? minimal_opcode_from_data(push_data) :
             nominal_opcode_from_data(push_data);
     }
 
+    bool data_empty() const NOEXCEPT;
+    size_t data_size() const NOEXCEPT;
+    const data_chunk& get_data() const NOEXCEPT;
+    const chunk_cptr& get_data_cptr() const NOEXCEPT;
+    void assign_data(reader& source) NOEXCEPT;
+
     // Operation should not be stored as shared (adds 16 bytes).
     // copy: 8 + 2 * 64 + 1 = 18 bytes (vs. 16 when shared).
     opcode code_;

include/bitcoin/system/chain/output.hpp

@@ -96,7 +96,6 @@ class BC_API output
 
 private:
     void assign_data(reader& source) NOEXCEPT;
-    ////static output from_data(reader& source) NOEXCEPT;
     static size_t serialized_size(const chain::script& script,
         uint64_t value) NOEXCEPT;
 

include/bitcoin/system/chain/point.hpp

@@ -94,7 +94,6 @@ class BC_API point
 
 private:
     void assign_data(reader& source) NOEXCEPT;
-    ////static point from_data(reader& source) NOEXCEPT;
 
     // The index is consensus-serialized as a fixed 4 bytes, however it is
     // effectively bound to 2^17 by the block byte size limit.

include/bitcoin/system/chain/script.hpp

@@ -172,9 +172,6 @@ class BC_API script
         size_t size) NOEXCEPT;
 
 private:
-    void assign_data(reader& source, bool prefix) NOEXCEPT;
-    ////static script from_data(reader& source, bool prefix) NOEXCEPT;
-
     // TODO: move to config serialization wrapper.
     static script from_string(const std::string& mnemonic) NOEXCEPT;
 
@@ -185,6 +182,8 @@ class BC_API script
         return ceilinged_add(total, op.serialized_size());
     };
 
+    void assign_data(reader& source, bool prefix) NOEXCEPT;
+
     // Script should be stored as shared.
     operations ops_;
 

include/bitcoin/system/chain/transaction.hpp

@@ -232,13 +232,13 @@ class BC_API transaction
         hash_digest sequences;
     } sighash_cache;
 
-    void assign_data(reader& source, bool witness) NOEXCEPT;
-    ////static transaction from_data(reader& source, bool witness) NOEXCEPT;
     static bool segregated(const chain::inputs& inputs) NOEXCEPT;
     static bool segregated(const chain::input_cptrs& inputs) NOEXCEPT;
     static sizes serialized_size(const chain::input_cptrs& inputs,
         const chain::output_cptrs& outputs, bool segregated) NOEXCEPT;
 
+    void assign_data(reader& source, bool witness) NOEXCEPT;
+
     // signature hash
     hash_digest output_hash(const input_iterator& input) const NOEXCEPT;
     input_iterator input_at(uint32_t index) const NOEXCEPT;
@@ -274,7 +274,6 @@ class BC_API transaction
     bool valid_;
     sizes size_;
 
-    // TODO: use std::optional to avoid these pointer allocations (0.16%).
     // Signature and identity hash caching (witness hash if witnessed).
     mutable std::unique_ptr<const hash_digest> nominal_hash_{};
     mutable std::unique_ptr<const hash_digest> witness_hash_{};

include/bitcoin/system/chain/witness.hpp

@@ -124,9 +124,6 @@ class BC_API witness
     witness(const chunk_cptrs& stack, bool valid, size_t size) NOEXCEPT;
 
 private:
-    void assign_data(reader& source, bool prefix) NOEXCEPT;
-    ////static witness from_data(reader& source, bool prefix) NOEXCEPT;
-
     // TODO: move to config serialization wrapper.
     static witness from_string(const std::string& mnemonic) NOEXCEPT;
 
@@ -143,6 +140,8 @@ class BC_API witness
     BC_POP_WARNING()
     BC_POP_WARNING()
 
+    void assign_data(reader& source, bool prefix) NOEXCEPT;
+
     // Witness should be stored as shared.
     chunk_cptrs stack_;
 

src/chain/block.cpp

@@ -44,6 +44,7 @@ namespace system {
 namespace chain {
 
 BC_PUSH_WARNING(NO_THROW_IN_NOEXCEPT)
+BC_PUSH_WARNING(NO_UNGUARDED_POINTERS)
 
 // Constructors.
 // ----------------------------------------------------------------------------
@@ -98,15 +99,11 @@ block::block(std::istream& stream, bool witness) NOEXCEPT
 }
 
 block::block(reader&& source, bool witness) NOEXCEPT
-  : block(source, witness/*from_data(source, witness)*/)
+  : block(source, witness)
 {
 }
 
-// Initializing here prevents default initialization for header_/txs_, which
-// would be redundant in light of later in-place initialization. This can be
-// avoided altogether by not default-initializing, and moving to assign_data.
 block::block(reader& source, bool witness) NOEXCEPT
-////: block(from_data(source, witness))
   : header_(
         source.get_allocator().new_object<chain::header>(source),
         source.get_allocator().deleter<chain::header>(source.get_arena())),
@@ -144,55 +141,10 @@ bool block::operator!=(const block& other) const NOEXCEPT
 // Deserialization.
 // ----------------------------------------------------------------------------
 
-////// static/private
-////block block::from_data(reader& source, bool witness) NOEXCEPT
-////{
-////    const auto read_transactions = [witness](reader& source) NOEXCEPT
-////    {
-////        // Allocate arena ctxs shared_ptr and std_vector(captures arena).
-////        auto ctxs = to_allocated<transaction_cptrs>(source.get_arena());
-////
-////        BC_PUSH_WARNING(NO_UNGUARDED_POINTERS)
-////        auto txs = to_non_const_raw_ptr(ctxs);
-////        BC_POP_WARNING()
-////
-////        // Allocate txs capacity(uses arena).
-////        const auto capacity = source.read_size(max_block_size);
-////        txs->reserve(capacity);
-////
-////        // Allocate each shared_ptr<tx> and move ptr to reservation.
-////        // Each tx is constructed in place as allocated by/with its pointer.
-////        for (size_t tx = 0; tx < capacity; ++tx)
-////            txs->push_back(to_allocated<transaction>(source.get_arena(),
-////                source, witness));
-////
-////        return ctxs;
-////    };
-////
-////    // These two pointers are discarded on assignment to allocated block.
-////    return
-////    {
-////        // Allocate header shared_ptr with header struct.
-////        to_allocated<chain::header>(source.get_arena(), source),
-////        read_transactions(source),
-////        source
-////    };
-////}
-
 // private
-BC_PUSH_WARNING(NO_UNGUARDED_POINTERS)
 void block::assign_data(reader& source, bool witness) NOEXCEPT
 {
     auto& allocator = source.get_allocator();
-
-    ////allocator.construct<chain::header::cptr>(&header_,
-    ////    allocator.new_object<chain::header>(source),
-    ////    allocator.deleter<chain::header>(source.get_arena()));
-    ////
-    ////allocator.construct<transactions_cptr>(&txs_,
-    ////    allocator.new_object<transaction_cptrs>(),
-    ////    allocator.deleter<transaction_cptrs>(source.get_arena()));
-
     const auto count = source.read_size(max_block_size);
     auto txs = to_non_const_raw_ptr(txs_);
     txs->reserve(count);
@@ -205,7 +157,6 @@ void block::assign_data(reader& source, bool witness) NOEXCEPT
     size_ = serialized_size(*txs_);
     valid_ = source;
 }
-BC_POP_WARNING()
 
 // Serialization.
 // ----------------------------------------------------------------------------
@@ -899,6 +850,7 @@ code block::connect(const context& ctx) const NOEXCEPT
     return connect_transactions(ctx);
 }
 
+BC_POP_WARNING()
 BC_POP_WARNING()
 
 // JSON value convertors.

src/chain/header.cpp

@@ -87,12 +87,11 @@ header::header(std::istream& stream) NOEXCEPT
 }
 
 header::header(reader&& source) NOEXCEPT
-  : header(source/*from_data(source)*/)
+  : header(source)
 {
 }
 
 header::header(reader& source) NOEXCEPT
-////: header(from_data(source))
 {
     assign_data(source);
 }
@@ -146,23 +145,7 @@ bool header::operator!=(const header& other) const NOEXCEPT
 // Deserialization.
 // ----------------------------------------------------------------------------
 
-////// static/private
-////header header::from_data(reader& source) NOEXCEPT
-////{
-////    return
-////    {
-////        source.read_4_bytes_little_endian(),
-////        source.read_hash(),
-////        source.read_hash(),
-////        source.read_4_bytes_little_endian(),
-////        source.read_4_bytes_little_endian(),
-////        source.read_4_bytes_little_endian(),
-////        source
-////    };
-////}
-
 // private
-// This denormalization eliminates allocation and copy of 64 bytes per block.
 void header::assign_data(reader& source) NOEXCEPT
 {
     // Hashes are copied directly into to header-allocated space.

src/chain/input.cpp

@@ -42,6 +42,9 @@ static_assert(max_script_size <
     max_size_t / multisig_default_sigops / heavy_sigops_factor,
     "input sigop overflow guard");
 
+// Null witness helpers.
+// ----------------------------------------------------------------------------
+
 // static/private
 const witness& input::no_witness() NOEXCEPT
 {
@@ -61,6 +64,16 @@ const witness::cptr& input::no_witness_cptr() NOEXCEPT
     return empty;
 }
 
+const chain::witness& input::get_witness() const NOEXCEPT
+{
+    return witness_ ? *witness_ : no_witness();
+}
+
+const chain::witness::cptr& input::get_witness_cptr() const NOEXCEPT
+{
+    return witness_ ? witness_ : no_witness_cptr();
+}
+
 // Constructors.
 // ----------------------------------------------------------------------------
 

src/chain/operation.cpp

@@ -39,6 +39,9 @@ BC_PUSH_WARNING(NO_THROW_IN_NOEXCEPT)
 // Gotta set something when invalid minimal result, test is_valid.
 static constexpr auto any_invalid = opcode::op_xor;
 
+// Null data helpers.
+// ----------------------------------------------------------------------------
+
 // static/private
 const data_chunk& operation::no_data() NOEXCEPT
 {
@@ -71,6 +74,26 @@ const chunk_cptr& operation::any_data_cptr() NOEXCEPT
     return any;
 }
 
+bool operation::data_empty() const NOEXCEPT
+{
+    return !data_ || data_->empty();
+}
+
+size_t operation::data_size() const NOEXCEPT
+{
+    return data_ ? data_->size() : zero;
+}
+
+const data_chunk& operation::get_data() const NOEXCEPT
+{
+    return data_ ? *data_ : no_data();
+}
+
+const chunk_cptr& operation::get_data_cptr() const NOEXCEPT
+{
+    return data_ ? data_ : no_data_cptr();
+}
+
 // Constructors.
 // ----------------------------------------------------------------------------