Parametrize timespan constants. #965

toxeus · 2018-06-06T10:56:30Z

The constants are parametrized such that libbitcoin can be used by various altcoins by solely editing the config file.

Further constants will be parametrized in upcoming pull requests.

As soon as this patch is in a satisfactory state I'll fix the unit tests and will open PRs in other libbitcoin repos to propagate these changes. Please comment.

toxeus · 2018-06-06T11:00:18Z

src/chain/header.cpp

@@ -41,7 +41,7 @@ using wall_clock = std::chrono::system_clock;
 //-----------------------------------------------------------------------------

 header::header()
-  : header(0, null_hash, null_hash, 0, 0, 0)
+  : header(0, null_hash, null_hash, 0, 0, 0, settings{})


header::header() is used by the factory functions below. I added this dummy value such that we can have the discussion here based on the code. I'd fix this by adding a settings parameter to this constructor and the factory functions. @evoskuil what do you think?

Yes, require the parameter until all other repos have been updated, so that you do not end up missing the necessary settings passage. But at that point you can also add a defaulted construction.

Alright, I have propagated the settings as a parameter wherever needed. This blew up the PR to some extend. Now, I know what you meant by this refactoring being "pervasive" :) Pushing new version now.

evoskuil · 2018-06-06T15:02:53Z

include/bitcoin/bitcoin/chain/chain_state.hpp

@@ -124,24 +125,26 @@ class BC_API chain_state
    };

    /// Checkpoints must be ordered by height with greatest at back.
-    static map get_map(size_t height, const checkpoints& checkpoints,
+    map get_map(size_t height, const checkpoints& checkpoints,


Keep this static and parameterize necessary settings (or settings object).

evoskuil · 2018-06-06T15:03:07Z

include/bitcoin/bitcoin/chain/chain_state.hpp

@@ -181,29 +184,29 @@ class BC_API chain_state

    static activations activation(const data& values, uint32_t forks);
    static uint32_t median_time_past(const data& values, uint32_t forks);
-    static uint32_t work_required(const data& values, uint32_t forks);
+    uint32_t work_required(const data& values, uint32_t forks);


Keep this static and parameterize necessary settings (or settings object).

evoskuil · 2018-06-06T15:09:21Z

src/chain/header.cpp

@@ -41,7 +41,7 @@ using wall_clock = std::chrono::system_clock;
 //-----------------------------------------------------------------------------

 header::header()
-  : header(0, null_hash, null_hash, 0, 0, 0)
+  : header(0, null_hash, null_hash, 0, 0, 0, settings{})


Yes, require the parameter until all other repos have been updated, so that you do not end up missing the necessary settings passage. But at that point you can also add a defaulted construction.

evoskuil · 2018-06-06T15:09:50Z

src/chain/header.cpp

 {
 }

 header::header(uint32_t version, hash_digest&& previous_block_hash,
-    hash_digest&& merkle, uint32_t timestamp, uint32_t bits, uint32_t nonce)
+    hash_digest&& merkle, uint32_t timestamp, uint32_t bits, uint32_t nonce, const settings& settings)


Style: honor the 80 character line limit (more above and below).

evoskuil · 2018-06-06T15:10:41Z

include/bitcoin/bitcoin/settings.hpp

+    uint32_t work_limit(bool retarget=true) const
+    {
+        return retarget ? retarget_proof_of_work_limit : no_retarget_proof_of_work_limit;
+    }


Move implementation to cpp.

evoskuil · 2018-06-06T15:11:09Z

include/bitcoin/bitcoin/settings.hpp

+    // The target number of blocks for 2 weeks of work (2016 blocks).
+    size_t retargeting_interval;
+
+    uint32_t work_limit(bool retarget=true) const


Style: move declaration above fields.

evoskuil · 2018-06-06T15:12:11Z

include/bitcoin/bitcoin/settings.hpp

+    {
+        return retarget ? retarget_proof_of_work_limit : no_retarget_proof_of_work_limit;
+    }
+


Style: remove blank line.

evoskuil · 2018-06-06T15:13:16Z

include/bitcoin/bitcoin/settings.hpp

+class BC_API settings
+{
+
+private:


Style: private after public: https://github.com/libbitcoin/libbitcoin/wiki/Style-Guide#hpp-format

evoskuil · 2018-06-06T15:13:40Z

include/bitcoin/bitcoin/settings.hpp

+/// Common database configuration settings, properties not thread safe.
+class BC_API settings
+{
+


Style: remove blank line: https://github.com/libbitcoin/libbitcoin/wiki/Style-Guide#hpp-format

toxeus · 2018-06-08T03:42:52Z

I'm still postponing to fix the tests until I know that the main changes are ok.

evoskuil · 2018-06-08T07:13:41Z

examples/main.cpp

@@ -45,7 +45,8 @@ int bc::main(int argc, char* argv[])
 #endif

    // Extracting Satoshi's words from genesis block.
-    const auto block = bc::chain::block::genesis_mainnet();
+    const auto block =
+        bc::chain::block::genesis_mainnet(libbitcoin::settings());


use bc::settings()

The genesis block needs to itself become a setting, meaning that genesis_mainnet will no longer exist. The genesis block will be deserialized from the base16-encoded setting stream.

Yes, the genesis block is on my todo list. I prefer to approach the migration of the constants iteratively. Once this is merged I'd open a new PR where the genesis block becomes a setting.

evoskuil · 2018-06-08T07:17:13Z

include/bitcoin/bitcoin/chain/chain_state.hpp

-    static uint32_t work_required_retarget(const data& values);
-    static uint32_t retarget_timespan(const chain_state::data& values);
+    static uint32_t work_required_retarget(const data& values,
+        const settings& settings);


Inconsistency here, pass retarget_proof_of_work_limit.

If you check the implementation then you can see that actually four values from settings are used in work_required_retarget():

settings.retarget_proof_of_work_limit

settings.min_timespan

settings.max_timespan

settings.target_timespan_seconds.
I'm happy to pass them individually but my judgement was that they would bloat the function signature. Your call.

Yeah, these aren't always obvious calls. I've tried for a consistent approach whereby classes are constructed with the the settings object given sufficient use, but functions (including static members) are parameterized so as to avoid a dependency on the setting object.

Alright, let's go for consistency here. Will adapt.

evoskuil · 2018-06-08T07:19:43Z

src/message/compact_block.cpp

 }

-bool compact_block::from_data(uint32_t version, reader& source)
+bool compact_block::from_data(uint32_t version, reader& source, const settings& settings)


Style: 80 char line length.

evoskuil · 2018-06-08T07:29:21Z

See warnings:

  CXX      src/src_libbitcoin_la-settings.lo
src/settings.cpp:33:44: warning: field 'retargeting_factor_' is uninitialized when used here [-Wuninitialized]
    min_timespan(target_timespan_seconds / retargeting_factor_),
                                           ^
src/settings.cpp:34:44: warning: field 'retargeting_factor_' is uninitialized when used here [-Wuninitialized]
    max_timespan(target_timespan_seconds * retargeting_factor_),
                                           ^
src/settings.cpp:35:52: warning: field 'target_spacing_seconds_' is uninitialized when used here [-Wuninitialized]
    retargeting_interval(target_timespan_seconds / target_spacing_seconds_)
                                                   ^
3 warnings generated.

toxeus · 2018-06-08T09:27:37Z

@evoskuil I was wondering why I didn't see those warnings. Turns out that they are enabled for C only, i.e. they're assigned to CFLAGS as opposed to CXXFLAGS which is used above and below the snipped I highlighted in the link. Also, CFLAGS is not eventually appended to CXXFLAGS. Now, I'm wondering why you did see the warnings!

evoskuil · 2018-06-08T15:24:40Z

See the Travis builds for the warnings.

toxeus · 2018-06-09T03:55:21Z

See the Travis builds for the warnings.

@evoskuil that was not my point. I've fixed the warnings in the second commit already. I just wanted to raise awareness that configure.ac is currently not set up correctly. Maybe @pmienk can confirm that?

evoskuil · 2018-06-09T04:53:12Z

My point was that the warnings are raised in the Travis builds - they are not limited to C sources.

toxeus · 2018-06-09T05:38:15Z

gotcha 👍

toxeus · 2018-06-10T05:26:35Z

@evoskuil I've looked now into the travis output of the build where the warnings have not yet been fixed. I cannot find the warnings you mentioned in any of the build flavours:

 for i in $(seq 0 5); do curl -s https://api.travis-ci.org/v3/job/38870787$i/log.txt | grep 'uninitialized'; done

evoskuil · 2018-06-10T05:43:01Z

@toxeus The warnings (in fact errors) were the result of field ordering in the header file. Fields are initialized in the header order, not in the order listed in the constructor initializers. I always try to keep these orders the same to avoid this type of failure. The errors were resolved in the most recent build by moving the initialization of these values into the constructor body.

There should not be private fields in the settings class. Making these public and matching the declaration order to the initialization order will resolve the issue properly.

toxeus · 2018-06-10T16:41:53Z

@evoskuil ok, I'm happy to fix the initialization issue the way you've suggested instead of moving it to the constructor body. But what about the build systemt's bug where it doesn't enable warnings for cpp files? Do you prefer me to open an issue for that?

evoskuil · 2018-06-10T17:09:20Z

@toxeus I’m not seeing the issue you are describing. These warnings are in a cpp file. But if you want to open an issue with explanation the libbitcoin-build repository is the best place to do it. Thanks!

toxeus · 2018-06-10T17:13:31Z

@evoskuil I gave you a curl command to see the issue I'm describing. I'll open an issue in the repo you've suggested. Thanks!

evoskuil · 2018-06-10T17:17:06Z

@toxeus I pointed out above the the warnings do appear in the Travis builds. The specific builds from the curl script did not have the warnings only because you inadvertently resolved them.

toxeus · 2018-06-10T17:20:02Z

@evoskuil the curl script fetches the logs for the commit where the warnings should have been present. Visit the travis build link and then click on commit.

evoskuil · 2018-06-10T17:24:30Z

@toxeus I already investigated this, which is how I discovered the inadvertent fix. The previous builds do have the warnings. In fact the Travis builds are the only place where I have seen the warnings as I haven’t been building locally.

toxeus · 2018-06-10T17:29:22Z

@evoskuil Strange. Maybe travis overwrites the logs.

evoskuil · 2018-06-10T17:41:43Z

Just look back at previous builds, the warnings are there in the logs.

toxeus · 2018-06-11T03:50:04Z

@evoskuil wrote:

Just look back at previous builds, the warnings are there in the logs.

Actually, it's not in the previous build but in the next one.

Now, I remember how this all came about!
I originally had an initialization order that did match the declaration order in the header files in my first version because I also always try to make them match (that's why I couldn't find the warning in the first travis build). Then, you asked me to adhere to the libbitcoin style and move the private members to the end of the header file. I've blindly implemented that suggestion which obviously introduced a mismatch in order between declaration and initialization. And I didn't see the corresponding warnings because I'm using gcc for development and the build system doesn't enable the warnings for gcc. See opened issue here. So, it turns out that we were both right about the presence of the warnings! 😉

which is how I discovered the inadvertent fix

The fix is correct if private member variables are used and the libbitcoin header file style is adhered to.

evoskuil · 2018-06-11T18:35:54Z

Great, thanks for tracking it down. I was only looking at the clang builds. I'll check out the -build issue.

evoskuil · 2018-06-11T21:17:31Z

Time to update the tests and get it passing.

toxeus · 2018-06-12T13:14:02Z

@evoskuil I've fixed the test. Will start work on the PRs for the other repos.

coveralls · 2018-06-12T13:51:07Z

Coverage decreased (-0.02%) to 81.48% when pulling 48903d4 on toxeus:master into 1b5f3df on libbitcoin:master.

evoskuil · 2018-06-12T20:59:22Z

src/settings.cpp

+{
+}
+
+uint32_t settings::work_limit(bool retarget) const


We should move this function out of settings and into chain::header (see chain::header::proof). Prob should do that in an independent commit.

Independent commit as in independent PR?

@evoskuil I've been now looking into moving this function into chain::header as you've suggested. The problem is that it is also used in chain::chain_state where there's no access to a header object. However, there's a comment which states that bits.self is unused. If this assignment can be removed then moving work_limit() becomes trivial.

Yes, you can set it to zero in that case. Self has no purpose in the context of a pool chain state and when the pool state is upgraded to a block the blocks bits are re-populated.

evoskuil · 2018-06-12T21:01:06Z

test/chain/block.cpp

    BOOST_REQUIRE(!instance.from_data(data));
    BOOST_REQUIRE(!instance.is_valid());
 }

 BOOST_AUTO_TEST_CASE(block__genesis__mainnet__valid_structure)
 {
-    const auto genesis = bc::chain::block::genesis_mainnet();
+    const auto genesis = bc::chain::block::genesis_mainnet(settings());


Use consistent style (settings() and settings{}).

My style is very consistent. I always use settings() except for cases where the vexing parse occurs.

evoskuil · 2018-06-12T21:02:56Z

include/bitcoin/bitcoin/settings.hpp

@@ -0,0 +1,55 @@
+/**


Need to add tests for settings class (see other repos).

Of course! The big refactoring work did distract my attention from the tests.

The constants are parametrized such that libbitcoin can be used by various altcoins by solely editing the config file.

toxeus · 2018-07-09T13:21:36Z

Rebased and fixed conflicts.

toxeus commented Jun 6, 2018

View reviewed changes

evoskuil requested changes Jun 6, 2018

View reviewed changes

toxeus force-pushed the master branch from 583c974 to 292f1af Compare June 8, 2018 03:40

evoskuil requested changes Jun 8, 2018

View reviewed changes

toxeus force-pushed the master branch from 7e81a7f to 16b250f Compare June 8, 2018 10:13

toxeus force-pushed the master branch from 2141ffe to d856360 Compare June 12, 2018 12:50

evoskuil requested changes Jun 12, 2018

View reviewed changes

Parametrize timespan constants.

48903d4

The constants are parametrized such that libbitcoin can be used by various altcoins by solely editing the config file.

toxeus force-pushed the master branch from 5ba61b4 to 48903d4 Compare July 9, 2018 13:20

evoskuil merged commit a9cdbe5 into libbitcoin:master Jul 9, 2018

Parametrize timespan constants. #965

Parametrize timespan constants. #965

Conversation

toxeus commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toxeus commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evoskuil commented Jun 8, 2018 • edited Loading

toxeus commented Jun 8, 2018

evoskuil commented Jun 8, 2018

toxeus commented Jun 9, 2018

evoskuil commented Jun 9, 2018

toxeus commented Jun 9, 2018

toxeus commented Jun 10, 2018

evoskuil commented Jun 10, 2018 • edited Loading

toxeus commented Jun 10, 2018

evoskuil commented Jun 10, 2018

toxeus commented Jun 10, 2018

evoskuil commented Jun 10, 2018

toxeus commented Jun 10, 2018

evoskuil commented Jun 10, 2018

toxeus commented Jun 10, 2018

evoskuil commented Jun 10, 2018

toxeus commented Jun 11, 2018 • edited Loading

evoskuil commented Jun 11, 2018

evoskuil commented Jun 11, 2018

toxeus commented Jun 12, 2018

coveralls commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evoskuil Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toxeus commented Jul 9, 2018

evoskuil commented Jun 8, 2018 •

edited

Loading

evoskuil commented Jun 10, 2018 •

edited

Loading

toxeus commented Jun 11, 2018 •

edited

Loading

coveralls commented Jun 12, 2018 •

edited

Loading

evoskuil Jul 16, 2018 •

edited

Loading