2
\$\begingroup\$

I want to rewrite blockchain source code from Golang to C++11. Please, review my code and help me to improve it.

Source in Golang:

package main

import "time"

type Block struct {
  Timestamp     int64
  Data          []byte
  PrevBlockHash []byte
  Hash          []byte
  Nonce         int
}

func NewBlock(data string, prevBlockHash []byte) *Block {
  block := &Block{ time.Now().Unix(), []byte(data), prevBlockHash, []byte{}, 0}

  proof := NewProofOfWork(block)
  nonce, hash := proof.Run()

  block.Hash = hash[:]
  block.Nonce = nonce

  return block
}

func NewGenesisBlock() *Block {
  return NewBlock("Genesis Block", []byte{})
}

My source in C++:

blockchain.h:

#ifndef BLOCKCHAIN_H
#define BLOCKCHAIN_H

#ifdef USE_BLOCK

#include <cstdint>
#include <vector>
#include <array>
#include <string>

using sha_t  = std::array<unsigned char, 32>;
using data_t = std::vector<unsigned char>;

struct Block
{
    uint64_t timestamp;
    data_t   data;
    sha_t    prevBlockHash;
    sha_t    hash;
    int      nonce;
};

Block make_block(std::string const& data,
                 sha_t const& prevBlockHash);
Block make_genesis_block();

#endif

#endif

block.cpp:

#define USE_BLOCK

#include "blockchain.h"

#include <chrono>


Block make_block(std::string const& data,
                 sha_t const& prevBlockHash)
{
    using namespace std::chrono;

    milliseconds ms = duration_cast<milliseconds>(system_clock::now().time_since_epoch());
    uint64_t timestamp = ms.count();
    data_t d(data.begin(), data.end());
    Block block{timestamp, d, prevBlockHash, sha_t(), 0};

    return block;
}

Block make_genesis_block()
{
    return make_block("Genesis Block", sha_t());
}

My questions:

  1. How to improve code structure and architecture?
  2. How to improve performance? Use constexpr or something else?
  3. How to store the data that all are byte arrays, but some of arrays have fixed length (such as SHA256 hashes), and some have dynamic size (such as data)?
  4. How to pass empty byte array? sha_t() is ok?
  5. How to deal with time and make code short and fast?
  6. What SHA256 library should I use?
\$\endgroup\$
  • 2
    \$\begingroup\$ C++11 or C++14? The title suggests C++11, but the tags and description tell a different story. \$\endgroup\$ – Daniel May 12 '18 at 11:23
  • \$\begingroup\$ @Coal_, C++11. I know C++11 better than C++14 \$\endgroup\$ – typemoon May 12 '18 at 11:35
  • \$\begingroup\$ Do you have to stick with C++11 only, rather than the current language and libraries? It’s 2018… \$\endgroup\$ – JDługosz May 13 '18 at 0:16
  • \$\begingroup\$ @JDługosz, no I'm free to use any standards and libraries. But I know better C++11. I want to learn C++17, but I dont know what use cases of new features are. \$\endgroup\$ – typemoon May 13 '18 at 11:33
7
\$\begingroup\$

How to improve code structure and architecture?

Three suggestions...

1. Recognize your code's logical concepts and do so as idiomatically as possible.

For example, the make_block and make_genesis_block seem to both be making a block. Here making-the-block is the concept.

You can more idiomatically recognize this make block concept via parametric polymorphism. 200_success suggests this via renaming make_genesis_block so it has the same name as the other block making function: make_block. And then having the genesis making block function take no arguments.

Now you have two make_block functions which the compiler distinguishes by the parameters (ADL):

Block make_block();
Block make_block(std::string const& data, sha_t const& prevBlockHash);

2. Recognize your code's structural invariants, and then enforce those using encapsulation.

Take the Block class for instance. I'd guess that the hash field is dependent on the data field. If correct, then make these inter-dependent fields private. Provide accessor methods for these values and a mutator method for the data field which updates the hash.

If nothing else depends on the data or hash fields, then I'd suggest making this tuple of info its own class and put the encapsulation within it. Then compositionally incorporate this new class into your block class. This way Block can still be a struct with its members publicly accessible.

Use Non-Member functions unless the function needs direct access to the representation of a class (C.4). For my favorite explanation of what I'm suggesting here, please see Scott Meyer's article: How Non-Member Functions Improve Encapsulation.

3. Prefer value semantics over reference semantics.

You haven't shown any code which exemplifies reference semantics but for the sake of C++ developers everywhere (or at least for my preference), please keep with value semantics as long as you can: don't expose to your API any classes that inherit from other class (see inheritance is the base class of evil).

How to improve performance? Use constexpr or something else?

  • Setup a benchmarking system for your code for the platforms you need the performance on.
  • Make sure to compile your code with the highest available optimization settings turned.
  • If your compiler will take profiling data for optimizing code, give it that too.
  • Understand the difference between return value optimization (RVO) and named return value optimization (NRVO).
  • Understand how RVO and NRVO fits in with copy elision and when you're guaranteed copy elision (not until C++17).

As an example of NRVO vs. RVO, your first make_block definition does:

Block block{timestamp, d, prevBlockHash, sha_t(), 0};
return block;

Here block is a named return value. This can be rewritten instead as:

return Block{timestamp, d, prevBlockHash, sha_t(), 0};

Which is an anonymous/unnamed return value.

While your question is tagged C++11, I'll opine that it's a nice practice still to be aware of the newer standards and that with C++17 you're guaranteed copy elision. I suspect that compilers of earlier standard implementation are also likely more able to support copy elision when using unnamed return values. Copy elision tends to help with performance.

Moreover, returning output through return values encourages value semantics, aids with constness, and I find it makes it easier for people to reason about the code.

As to constexpr, I haven't seen it increase performance much of code I've used it with. OTOH, I love what recognizing code as constexpr means — computable at compile time — and I agree with Scott Meyers recommendation: use constexpr whenever possible (from Item 15 of his Effective Modern C++ book). A related nicety IMO is that types having constexpr constructors can then satisfy the LiteralType concept.

How to store the data that all are byte arrays, but some of arrays have fixed length (such as SHA256 hashes), and some have dynamic size (such as data)?

I like how you've already done this — you're using std::array for the fixed length types, and std::vector for the dynamic types. It's a preferable starting point so long as these types' access don't allow your data invariants to be violated.

Prior to C++17, you may just want to update the type you're using in the std::array and std::vector containers to std::uint8_t. Since C++17, we can use std::byte for this type as one of the commentators suggests.

Deeper in, you may need to answer whether speed is more important or data correctness is. A structure of arrays (SOA) often lends itself better to speed, while an array of structures (AOS) can lend itself better IMO to correctness.

How to pass empty byte array? sha_t() is ok?

It's not clear what you're asking with this question. Sounds like you may want to read about value initialization (introduced in C++03).

How to deal with time and make code short and fast?

Seems like this question is asking again about how to improve performance. If so, please see my response in that section.

What SHA256 library should I use?

The code from the most recent OpenSSL library is what I'd go with just simply because I suspect it's had the most eyes on it. If your code is license compatible, you may find it easier to just copy the OpenSSL code for SHA256 into your code instead of trying to build the entire OpenSSL library and then linking with it.

\$\endgroup\$
  • 1
    \$\begingroup\$ I’ll add to the notes about byte arrays: use std::byte instead of char. \$\endgroup\$ – JDługosz May 13 '18 at 0:19
  • \$\begingroup\$ @JDługosz I like your suggestion and updated my answer to include the use of std::byte. \$\endgroup\$ – Louis Langholtz May 13 '18 at 3:02
  • \$\begingroup\$ You should not be building most libraries. All modern systems have a way to install standard libraries. brew or yum or apt-get. Don't import these libraries into your own code base, this just means your code will not get critical fixes as the libraries are fixed. \$\endgroup\$ – Martin York May 13 '18 at 16:23
  • 1
    \$\begingroup\$ In point one you completely miss @200_success 's point. \$\endgroup\$ – Martin York May 13 '18 at 16:26
  • 1
    \$\begingroup\$ Provide accessor methods for these values. Providing accessor methods (eg getX() and setX()) are anti patterns in C++. You are exposing implementation details of the block outside the block. All mutation of the block should be actions that can be performed on the block by members. \$\endgroup\$ – Martin York May 13 '18 at 16:29
3
\$\begingroup\$

C++ code should be object-oriented. The make_block(…, …) and make_genesis_block() functions should be two-arg and no-arg constructors of the Block class.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.