Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Fix logic used to determine if Aleth should request dao fork block header from potential peer #5539

Merged
merged 5 commits into from
Apr 2, 2019

Conversation

halfalicious
Copy link
Contributor

@halfalicious halfalicious commented Apr 1, 2019

Fix #5271

Update the logic which Aleth uses to determine if it should request the DAO fork block header from a peer before starting a sync based on the value of the daoHardFork chain configuration parameter. Aleth needs to check for 0 or c_infiniteBlockNumber (rather than just 0) because Aleth initializes the daoHardFork configuration parameter to c_infiniteBlockNumber by default.

The impact of these changes is that one doesn't have to explicitly specify daoHardFork = 0x0 in any config.json that they wish to use with Aleth.

We need this check because all hard fork chain parameter fields are initialized to c_infiniteBlockNumber to indicate not set.

Fork block chain params members are initialized to
Also move BlockchainSync constants inside of existing anonymous namespace.
@halfalicious halfalicious changed the title Fix dao fork block bug Fix logic used to determine if Aleth should request dao fork block header from potential peer Apr 1, 2019
@halfalicious halfalicious requested review from gumb0 and chfast April 1, 2019 05:47
@codecov-io
Copy link

Codecov Report

Merging #5539 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5539      +/-   ##
==========================================
- Coverage    61.9%   61.88%   -0.03%     
==========================================
  Files         344      344              
  Lines       28757    28769      +12     
  Branches     3267     3269       +2     
==========================================
+ Hits        17803    17804       +1     
- Misses       9784     9795      +11     
  Partials     1170     1170

@@ -63,7 +63,7 @@ class PrecompiledContract
u256 m_startingBlock = 0;
};

static constexpr int64_t c_infiniteBlockNumer = std::numeric_limits<int64_t>::max();
static constexpr int64_t c_infiniteBlockNumber = std::numeric_limits<int64_t>::max();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static is redundant for constexpr.

Copy link
Contributor Author

@halfalicious halfalicious Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that static and constexpr are orthogonal concepts? I agree that static doesn't make sense here (regardless of the constexpr) since this is a header file and c_infiniteBlockNumber is defined outside of a class - from what I understand, static when used this way limits the scope of the variable to the translation unit which means that each cpp file which includes the header will have a separate instance of c_infiniteBlockNumber (though each instance will have the same value) which seems unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also confused now. On the language level that might be true. But also constexpr implies inline and inline implies static.

I check than in practice the constexpr symbols are not exported.

@@ -52,6 +30,10 @@ std::ostream& dev::eth::operator<<(std::ostream& _out, SyncStatus const& _sync)
namespace // Helper functions.
{

unsigned constexpr c_maxPeerUknownNewBlocks = 1024; /// Max number of unknown new blocks peer can give us
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic mode: should be constexpr unsigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we put constexpr before the type but const after the type?

@chfast chfast merged commit b09e9ad into master Apr 2, 2019
@chfast chfast deleted the dao-fork-block branch April 2, 2019 08:10
@gumb0 gumb0 mentioned this pull request Apr 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants