Consensus: MOVEONLY: Move functions for header verification #8337

jtimon · 2016-07-13T13:28:06Z

Moves code for header verification out of main, to consensus.

EDIT:
Partially replaces #7310, only for the header verification part.
~~Like in core_io.h we can separate the cpp files instead of using a single consensus.cpp.~~

~~Continues #8329~~
It's analogous to #8329 but with the functions necessary for VerifyHeader().

jonasschnelli · 2016-07-14T11:50:42Z

This PR contains identical commits then #8328 and #8329.
Can you explain why there are multiple PRs?
IMO opening a single small PR and trying to get this merge would be more efficient (and only open/PR further work when the first part has been merged).

jtimon · 2016-07-14T13:20:06Z

Each PR contains more things than the previous one. I could make them
independent for easier review instead of each one building on top of the
previous, but that would more work. Or I can just close them. Only the
first one seems to be getting any review anyway...
On Jul 14, 2016 1:52 PM, "Jonas Schnelli" [email protected] wrote:

This PR contains identical commits then #8328
#8328 and #8329
#8329.
Can you explain why there are multiple PRs?
IMO opening a single small PR and trying to get this merge would be more
efficient (and only open/PR further work when the first part has been
merged).

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8337 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA9jSkECkHcKe1bhOGYzTNkqWpxVKaa2ks5qViK9gaJpZM4JLak4
.

afk11 · 2016-07-14T18:15:26Z

A diff wrt. #8329 (since it builds on it) jtimon/bitcoin@jtimon:0.12.99-consensus-moveonly-tx...0.12.99-consensus-moveonly-header

jtimon · 2016-07-14T23:12:24Z

Updated as independent from #8329 and #8328 (closed).
@afk11 Thanks for the selected diffs, I think they're very useful for review.
Perhaps I should do the same on any PR I make dependent on another one.
But please keep looking for diffs you think you're interesting, I'm always open to squashes and interactive rebasing for things that have not been merged.

btcdrak · 2016-08-29T11:57:29Z

needs rebase

jtimon · 2016-08-30T12:23:17Z

Rebased.

CodeShark · 2016-10-10T14:15:59Z

Verified it is move only. ACK

btcdrak · 2016-10-10T19:20:05Z

utACK, move only 6d559b2af4925404ba2542d3c644da15a337dbc7

jtimon · 2016-10-20T01:37:29Z

Sorry for breaking the reviews, but it was suggested that declaring the functions in consensus/header_verify.h instead of consensus/consensus.h would be more clear.

morcos · 2016-11-27T01:28:07Z

src/main.cpp

same issue here. this should not be removed.

morcos · 2016-11-27T01:41:37Z

utACK d324e016a

jtimon · 2016-11-27T19:02:32Z

Fixed @morcos 's nit.

NicolasDorier · 2016-11-29T16:35:15Z

utACK 6a66d7a242387ea73bf5f40154ef011b246c1a02

jtimon · 2016-12-03T06:17:53Z

Needed rebase after renaming main.o, see #9260 (although needed, the rebase was clean in this case)

dcousens · 2017-03-21T05:56:58Z

utACK 4d17675

jtimon · 2017-03-23T19:05:55Z

Needed rebase after #9908 and after [some other PR to locate that introduces the assert in ContextualCheckBlockHeader].
After #9908 , added a commit to move MAX_FUTURE_BLOCK_TIME from chain.h to consensus/consensus.h

jtimon · 2017-05-18T20:36:06Z

Needed trivial rebase in the makefile after #8329 was merged. For next rebase (which will hopefully be needed soon, see #9717 and #10339 ), I'm thinking of directly moving CheckBlockHeader to pow.o instead of consensus/header_verify.o. People are even talking about removing it. Thoughts?

ryanofsky

utACK dd7836a confirmed move only.

ryanofsky · 2017-05-18T21:15:48Z

src/consensus/consensus.h

+ * Maximum amount of time that a block timestamp is allowed to exceed the
+ * current network-adjusted time before the block will be accepted.
+ */
+static const int64_t MAX_FUTURE_BLOCK_TIME = 2 * 60 * 60;


In commit "MOVEONLY: MAX_FUTURE_BLOCK_TIME to consensus/consensus.h"

Not sure if if it's exactly right to make this a consensus constant, because even in the worst case if this is set inconsistently, consensus should eventually be reached when system clock times catch up. IIRC, this was the reasoning @morcos gave me for not defining this constant in consensus.h.

The goal is more to take it out of chain.h than to put it in consensus/consensus.h. consensus/header_verify would be fine as well. As an example, #8493 puts the check in libconsensus without putting chain.o in libconsensus too. Whether blocks eventually become valid when violating this rule or not, I think the question that should be asked is "in a hypothetical finished libconsensus, is this check included or not?", and I also think the answer is yes.

jtimon · 2017-06-19T06:57:13Z

Needs rebase again and this was for #8493 anyway, closing.

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from 19ed8f4 to 8697b78 Compare July 14, 2016 02:09

jtimon changed the title ~~Consensus: MOVEONLY: Move functions for tx verification~~ Consensus: MOVEONLY: Move functions for header verification Jul 14, 2016

jtimon mentioned this pull request Jul 14, 2016

Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress] #8339

Closed

jonasschnelli added the Refactoring label Jul 14, 2016

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from 8697b78 to d935e1d Compare July 14, 2016 22:49

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from d935e1d to ffe96f0 Compare July 15, 2016 12:08

jtimon mentioned this pull request Jul 16, 2016

Introduce Consensus::GetFlags() and hide IsSuperMajority() #8345

Closed

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from ffe96f0 to 6bd9296 Compare August 30, 2016 12:21

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from 6bd9296 to ee5ba72 Compare August 31, 2016 21:12

jtimon mentioned this pull request Aug 31, 2016

Untested: libconsensus: Expose VerifyHeader #8493

Closed

2 tasks

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from ee5ba72 to 6d559b2 Compare September 1, 2016 16:21

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from 6d559b2 to d324e01 Compare October 20, 2016 01:35

jtimon mentioned this pull request Nov 26, 2016

Consensus: MOVEONLY: Move functions for tx verification #8329

Merged

morcos reviewed Nov 27, 2016

View reviewed changes

src/main.cpp Outdated

Copy link

Contributor

morcos Nov 27, 2016

Choose a reason for hiding this comment

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

same issue here. this should not be removed.

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from d324e01 to 6a66d7a Compare November 27, 2016 19:02

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from 6a66d7a to 4d17675 Compare December 3, 2016 06:16

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from 4d17675 to 7365a8b Compare March 23, 2017 19:03

jtimon added 2 commits May 18, 2017 22:36

MOVEONLY: Header verification to consensus/header_verify.o

3eade54

MOVEONLY: MAX_FUTURE_BLOCK_TIME to consensus/consensus.h

dd7836a

jtimon force-pushed the 0.12.99-consensus-moveonly-header branch from 7365a8b to dd7836a Compare May 18, 2017 20:38

ryanofsky reviewed May 18, 2017

View reviewed changes

jtimon closed this Jun 19, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Consensus: MOVEONLY: Move functions for header verification #8337

Consensus: MOVEONLY: Move functions for header verification #8337

Uh oh!

Conversation

jtimon commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jul 14, 2016

Uh oh!

jtimon commented Jul 14, 2016

Uh oh!

afk11 commented Jul 14, 2016

Uh oh!

jtimon commented Jul 14, 2016

Uh oh!

btcdrak commented Aug 29, 2016

Uh oh!

jtimon commented Aug 30, 2016

Uh oh!

CodeShark commented Oct 10, 2016

Uh oh!

btcdrak commented Oct 10, 2016

Uh oh!

jtimon commented Oct 20, 2016

Uh oh!

morcos Nov 27, 2016

Choose a reason for hiding this comment

Uh oh!

morcos commented Nov 27, 2016

Uh oh!

jtimon commented Nov 27, 2016

Uh oh!

NicolasDorier commented Nov 29, 2016

Uh oh!

jtimon commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcousens commented Mar 21, 2017

Uh oh!

jtimon commented Mar 23, 2017

Uh oh!

jtimon commented May 18, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 18, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon May 20, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jun 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jtimon commented Jul 13, 2016 •

edited

Loading

jtimon commented Dec 3, 2016 •

edited

Loading