Consensus: MOVEONLY: Move functions for tx verification #8329

jtimon · 2016-07-11T16:46:54Z

Partially replaces #7310, only for the transaction verification part.
Like in core_io.h we can separate the cpp files instead of using a single consensus.cpp. I would like to know what is preferred in that reward as soon as possible: I won't fight either way because I don't care (until something has been decided then I will become a fanatic against changing the decision because I may waste seconds renaming things while rebasing interactively with some of my outdated branches).

Based in #8328 out of laziness, it can be easily separated if necessary.

jtimon · 2016-07-11T16:48:26Z

ping @MarcoFalke this time the moveonly commit should be easier to verify than in #7310. We can squash additial doc or not separately at the end as you pointed out.

jtimon · 2016-07-13T12:43:28Z

Updated, squashed the doc commits into a single one.

afk11 · 2016-07-14T18:10:39Z

Slightly easier to review when compared with #8328 jtimon/bitcoin@jtimon:0.12.99-consensus-rename...0.12.99-consensus-moveonly-tx

tACK

jtimon · 2016-07-14T21:30:43Z

Updated as independent from closed #8328.
Single commit now, should be easier to review.

NicolasDorier · 2016-07-15T08:27:51Z

this #8342 would remove the dependency to boost

NicolasDorier · 2016-07-15T08:29:52Z

I think move should be done only after we get rid of the dependencies you intend to remove.

jtimon · 2016-07-15T12:06:45Z

If your fix is done after the move, it will be visible in git blame. That would be a reason for moving first. What are the reasons in favor of moving later?

Sorry, updated again adding a couple of lines of documentation as separators.

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

needs rebase

jtimon · 2016-08-30T13:04:29Z

Rebased

jtimon · 2016-09-01T16:17:18Z

Needed rebase again.

jtimon · 2016-10-19T17:58:23Z

Rebased.
Also moved the declarations from consensus.h to tx_verify.h as suggested by @sipa .

jtimon · 2016-11-21T19:07:59Z

Needed rebase.

NicolasDorier · 2016-11-25T06:16:52Z

utACK, what is blocking that? Old PR, simple to review and definitively a step in right direction.

maflcko · 2016-11-25T10:25:21Z

utACK 08f4cf00daed646f25ae25d40c700bed1458f4cc

afk11 · 2016-11-25T11:08:48Z

utACK 08f4cf0

btcdrak · 2016-11-25T14:33:02Z

utACK 08f4cf00daed646f25ae25d40c700bed1458f4cc

morcos · 2016-11-26T22:00:22Z

utACK 08f4cf00da

jtimon · 2016-11-27T18:58:10Z

Fixed @morcos 's nits.

morcos · 2016-11-27T22:44:56Z

utACK 4ed06095

jtimon · 2016-12-03T06:13:07Z

Needed rebase after renaming main.o, see #9260

jtimon · 2017-01-03T16:19:12Z

Needs rebase again

jtimon · 2017-01-31T23:29:27Z

Rebased

Functions related to transaction verification.

jtimon · 2017-04-06T21:37:30Z

Needed rebase

ryanofsky

utACK 618d07f. Confirmed this is move only, except for the EvaluateSequenceLocks changes noted.

ryanofsky · 2017-05-08T19:21:51Z

src/consensus/tx_verify.h

+ */
+std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block);
+
+bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair);


This declaration seems like it might have been added here by accident. It isn't neccessary, isn't documented, and wasn't in the original header file.

validation.cpp uses this call, thought it would be nice if the header additions were committed separately from the moved code to make this clear and easier to review (verify moveonly) @jtimon

ryanofsky · 2017-05-08T19:24:58Z

src/consensus/tx_verify.cpp

+    return std::make_pair(nMinHeight, nMinTime);
+}
+
+bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair)


Maybe declare this static again. The declaration isn't documented, and there doesn't seem to be a reason to expose it.

ryanofsky · 2017-05-10T15:08:11Z

This seems ready to be merged. Has like 6 acks.

mchrostowski · 2017-05-16T22:20:00Z

ACK Code is just a move with a few minor changes that include:

Addition of declarations that need to be exposed from their new compilation unit
Removal of static keyword to change linkage for above mentioned declarations
Cleanup/optimization of #include's

I checked that the changes were only moves, compiled, ran tests, ran bitcoind.

laanwj · 2017-05-18T18:59:47Z

utACK 618d07f, verified that the consensus function moves to tx_verify.cpp are move-only (besides some comment and namespace changes)

618d07f MOVEONLY: tx functions to consensus/tx_verify.o (Jorge Timón) Tree-SHA512: 63fa2777c070a344dbfe61974526a770d962e049881c6f371b0034b1682c1e6e24f47454f01ee35ded20ade34488e023d4467a05369662906b99a73bb5de8497

…cation 618d07f MOVEONLY: tx functions to consensus/tx_verify.o (Jorge Timón) Tree-SHA512: 63fa2777c070a344dbfe61974526a770d962e049881c6f371b0034b1682c1e6e24f47454f01ee35ded20ade34488e023d4467a05369662906b99a73bb5de8497

…ication (#3030) * Merge bitcoin#8329: Consensus: MOVEONLY: Move functions for tx verification 618d07f MOVEONLY: tx functions to consensus/tx_verify.o (Jorge Timón) Tree-SHA512: 63fa2777c070a344dbfe61974526a770d962e049881c6f371b0034b1682c1e6e24f47454f01ee35ded20ade34488e023d4467a05369662906b99a73bb5de8497 * remove GetTransactionSigOpCost Signed-off-by: Pasta <[email protected]> * fix, properly copy (methods moved had diverged from upstream), more fixing Signed-off-by: Pasta <[email protected]>

jonasschnelli added the Refactoring label Jul 12, 2016

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 8af7c64 to 68513f9 Compare July 13, 2016 12:39

jtimon mentioned this pull request Jul 13, 2016

Consensus: MOVEONLY: Move functions for header verification #8337

Closed

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 68513f9 to cdcfecf Compare July 14, 2016 02:10

jtimon mentioned this pull request Jul 14, 2016

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

Closed

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from cdcfecf to a50602c Compare July 14, 2016 21:29

jtimon mentioned this pull request Jul 15, 2016

Consensus: Trivial transform BOOST_FOREACH into for loop #8342

Merged

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from a50602c to 43bfd1f Compare July 15, 2016 12:05

jtimon mentioned this pull request Jul 16, 2016

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

Closed

jtimon mentioned this pull request Jul 28, 2016

Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs #8346

Merged

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 43bfd1f to ee69709 Compare August 30, 2016 13:02

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from ee69709 to 3fc8ee2 Compare September 1, 2016 16:17

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 3fc8ee2 to 6f8b51c Compare October 19, 2016 17:57

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 6f8b51c to 08f4cf0 Compare November 21, 2016 19:07

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 08f4cf0 to 4ed0609 Compare November 27, 2016 18:57

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 4ed0609 to 26997cc Compare December 3, 2016 06:11

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 26997cc to af2b32f Compare January 24, 2017 21:33

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from af2b32f to 572c528 Compare January 31, 2017 23:28

MOVEONLY: tx functions to consensus/tx_verify.o

618d07f

Functions related to transaction verification.

jtimon force-pushed the 0.12.99-consensus-moveonly-tx branch from 572c528 to 618d07f Compare April 6, 2017 21:37

ryanofsky reviewed May 8, 2017

View reviewed changes

laanwj merged commit 618d07f into bitcoin:master May 18, 2017

jtimon deleted the 0.12.99-consensus-moveonly-tx branch May 18, 2017 20:23

ryanofsky mentioned this pull request May 23, 2017

Near-Bugfix: Optimization: Minimize the number of times it is checked that no money... #8498

Merged

dagurval mentioned this pull request Apr 11, 2018

MOVEONLY: Move functions for tx verification bitcoinxt/bitcoinxt#379

Merged

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

Consensus: MOVEONLY: Move functions for tx verification #8329

Consensus: MOVEONLY: Move functions for tx verification #8329

Uh oh!

Conversation

jtimon commented Jul 11, 2016

Uh oh!

jtimon commented Jul 11, 2016

Uh oh!

jtimon commented Jul 13, 2016

Uh oh!

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

Uh oh!

Uh oh!

jtimon commented Jul 14, 2016

Uh oh!

NicolasDorier commented Jul 15, 2016

Uh oh!

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

Uh oh!

Uh oh!

jtimon commented Jul 15, 2016

Uh oh!

btcdrak commented Aug 29, 2016

Uh oh!

jtimon commented Aug 30, 2016

Uh oh!

jtimon commented Sep 1, 2016

Uh oh!

jtimon commented Oct 19, 2016

Uh oh!

jtimon commented Nov 21, 2016

Uh oh!

NicolasDorier commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 25, 2016

Uh oh!

afk11 commented Nov 25, 2016

Uh oh!

btcdrak commented Nov 25, 2016

Uh oh!

morcos commented Nov 26, 2016

Uh oh!

jtimon commented Nov 27, 2016

Uh oh!

morcos commented Nov 27, 2016

Uh oh!

jtimon commented Dec 3, 2016

Uh oh!

jtimon commented Jan 3, 2017

Uh oh!

jtimon commented Jan 31, 2017

Uh oh!

jtimon commented Apr 6, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 8, 2017

Choose a reason for hiding this comment

Uh oh!

mchrostowski May 16, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 8, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented May 10, 2017

Uh oh!

mchrostowski commented May 16, 2017

Uh oh!

laanwj commented May 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

afk11 commented Jul 14, 2016 •

edited

Loading

NicolasDorier commented Jul 15, 2016 •

edited

Loading

NicolasDorier commented Nov 25, 2016 •

edited

Loading