Consensus: MOVEONLY: Move functions for tx verification #8329

pull jtimon wants to merge 1 commits into bitcoin:master from jtimon:0.12.99-consensus-moveonly-tx changing 13 files +336 −297
  1. jtimon commented at 4:46 PM on July 11, 2016: contributor

    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.

  2. jtimon commented at 4:48 PM on July 11, 2016: contributor

    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.

  3. jonasschnelli added the label Refactoring on Jul 12, 2016
  4. jtimon force-pushed on Jul 13, 2016
  5. jtimon commented at 12:43 PM on July 13, 2016: contributor

    Updated, squashed the doc commits into a single one.

  6. jtimon cross-referenced this on Jul 13, 2016 from issue Consensus: MOVEONLY: Move functions for header verification by jtimon
  7. jtimon force-pushed on Jul 14, 2016
  8. jtimon cross-referenced this on Jul 14, 2016 from issue Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress] by NicolasDorier
  9. afk11 commented at 6:10 PM on July 14, 2016: contributor

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

    tACK

  10. jtimon force-pushed on Jul 14, 2016
  11. jtimon commented at 9:30 PM on July 14, 2016: contributor

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

  12. NicolasDorier commented at 8:27 AM on July 15, 2016: contributor

    this #8342 would remove the dependency to boost

  13. NicolasDorier commented at 8:29 AM on July 15, 2016: contributor

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

  14. jtimon cross-referenced this on Jul 15, 2016 from issue Consensus: Trivial transform BOOST_FOREACH into for loop by NicolasDorier
  15. jtimon force-pushed on Jul 15, 2016
  16. jtimon commented at 12:06 PM on July 15, 2016: contributor

    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.

  17. jtimon cross-referenced this on Jul 16, 2016 from issue Introduce Consensus::GetFlags() and hide IsSuperMajority() by jtimon
  18. jtimon cross-referenced this on Jul 28, 2016 from issue Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs by jtimon
  19. btcdrak commented at 11:57 AM on August 29, 2016: contributor

    needs rebase

  20. jtimon force-pushed on Aug 30, 2016
  21. jtimon commented at 1:04 PM on August 30, 2016: contributor

    Rebased

  22. jtimon force-pushed on Sep 1, 2016
  23. jtimon commented at 4:17 PM on September 1, 2016: contributor

    Needed rebase again.

  24. jtimon force-pushed on Oct 19, 2016
  25. jtimon commented at 5:58 PM on October 19, 2016: contributor

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

  26. jtimon force-pushed on Nov 21, 2016
  27. jtimon commented at 7:07 PM on November 21, 2016: contributor

    Needed rebase.

  28. NicolasDorier commented at 6:16 AM on November 25, 2016: contributor

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

  29. MarcoFalke commented at 10:25 AM on November 25, 2016: member

    utACK 08f4cf00daed646f25ae25d40c700bed1458f4cc

  30. afk11 commented at 11:08 AM on November 25, 2016: contributor

    utACK 08f4cf0

  31. btcdrak commented at 2:33 PM on November 25, 2016: contributor

    utACK 08f4cf00daed646f25ae25d40c700bed1458f4cc

  32. paveljanik commented at 2:44 PM on November 25, 2016: contributor

    Nice!

    using namespace std; can be removed, because there is only one set -> std::set here:

    set<COutPoint> vInOutPoints;

    ACK https://github.com/bitcoin/bitcoin/commit/08f4cf00daed646f25ae25d40c700bed1458f4cc

  33. jtimon commented at 7:42 PM on November 26, 2016: contributor

    @NicolasDorier Nothing is blocking this nor #8337 to my knowledge. @paveljanik I was happy removing using namespace but it makes the moveonly slightly harder to verify and it can be done later. For more context, see: #6051 (comment)
    #6672 (comment) #6672 (comment) #7310#r54867087

  34. in src/main.cpp:None in 08f4cf00da outdated 
      10 | @@ -11,8 +11,8 @@
  11 |  #include "chainparams.h"
  12 |  #include "checkpoints.h"
  13 |  #include "checkqueue.h"
  14 | -#include "consensus/consensus.h"
    morcos commented at 9:43 PM on November 26, 2016:

    Why is it ok to remove this from main.cpp?

    EDIT: I guess the idea is anytime we include policy.h, we don't need to bother including consensus.h? It's a tiny nit, but that seems an unrelated change to me from the rest of this PR.

    jtimon commented at 11:00 PM on November 26, 2016:

    No, you are right, it shouldn't be removed. I used to have the new declarations on consensus/consensus.h instead of consensus/tx_verify.h and I think I just replaced it in main like I did in other places (but in main it makes sense to have both). Good catch, will change.

    morcos commented at 11:21 PM on November 26, 2016:

    I think that applies to miner.cpp and txmempool.cpp too then?

  35. morcos commented at 10:00 PM on November 26, 2016: member

    utACK 08f4cf00da

  36. jtimon force-pushed on Nov 27, 2016
  37. jtimon commented at 6:58 PM on November 27, 2016: contributor

    Fixed @morcos 's nits.

  38. morcos commented at 10:44 PM on November 27, 2016: member

    utACK 4ed06095

  39. jtimon force-pushed on Dec 3, 2016
  40. jtimon commented at 6:13 AM on December 3, 2016: contributor

    Needed rebase after renaming main.o, see #9260

  41. jtimon commented at 4:19 PM on January 3, 2017: contributor

    Needs rebase again

  42. jtimon force-pushed on Jan 24, 2017
  43. jtimon force-pushed on Jan 31, 2017
  44. jtimon commented at 11:29 PM on January 31, 2017: contributor

    Rebased

  45. MOVEONLY: tx functions to consensus/tx_verify.o
    Functions related to transaction verification.
    618d07faa2
  46. jtimon force-pushed on Apr 6, 2017
  47. jtimon commented at 9:37 PM on April 6, 2017: contributor

    Needed rebase

  48. in src/consensus/tx_verify.h:71 in 618d07faa2 
      66 | + * Also removes from the vector of input heights any entries which did not
  67 | + * correspond to sequence locked inputs as they do not affect the calculation.
  68 | + */
  69 | +std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block);
  70 | +
  71 | +bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair);
    ryanofsky commented at 7:21 PM on May 8, 2017:

    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.

    mchrostowski commented at 8:15 PM on May 16, 2017:

    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

  49. in src/consensus/tx_verify.cpp:92 in 618d07faa2 
      87 | +    }
  88 | +
  89 | +    return std::make_pair(nMinHeight, nMinTime);
  90 | +}
  91 | +
  92 | +bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair)
    ryanofsky commented at 7:24 PM on May 8, 2017:

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

  50. ryanofsky commented at 7:27 PM on May 8, 2017: contributor

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

  51. ryanofsky commented at 3:08 PM on May 10, 2017: contributor

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

  52. mchrostowski commented at 10:20 PM on May 16, 2017: contributor

    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.

  53. laanwj commented at 6:59 PM on May 18, 2017: member

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

  54. laanwj merged this on May 18, 2017
  55. laanwj closed this on May 18, 2017
  56. laanwj referenced this in commit ea6fde3f1d on May 18, 2017
  57. jtimon deleted the branch on May 18, 2017
  58. ryanofsky cross-referenced this on May 23, 2017 from issue Near-Bugfix: Optimization: Minimize the number of times it is checked that no money... by jtimon
  59. fanquake moved this from the "In progress" to the "Done" column in a project
  60. dagurval cross-referenced this on Apr 11, 2018 from issue MOVEONLY: Move functions for tx verification by dagurval
  61. PastaPastaPasta referenced this in commit c2ba1012fb on Jun 10, 2019
  62. PastaPastaPasta referenced this in commit 36f39e3307 on Jun 20, 2019
  63. PastaPastaPasta referenced this in commit b206fa58bc on Jul 13, 2019
  64. PastaPastaPasta referenced this in commit 2c62cefe7b on Jul 17, 2019
  65. PastaPastaPasta referenced this in commit 74a394129c on Jul 17, 2019
  66. UdjinM6 referenced this in commit 7e4318dda8 on Jul 23, 2019
  67. barrystyle referenced this in commit 381790c4b1 on Jan 22, 2020
  68. bitcoin locked this on Sep 8, 2021
Contributors
jtimonafk11NicolasDorierbtcdrakMarcoFalkepaveljanikmorcosryanofskymchrostowskilaanwj
Labels
Refactoring
Linked (view graph)
#6051 MOVEONLY: Consensus: Move most of consensus functions (pre-block)#6672 Consensus: Separate most consensus functions to consensus.cpp#7310 MOVEONLY: Move consensus functions out of main#8328 Consensus: Rename: Move consensus code to the consensus directory#8337 Consensus: MOVEONLY: Move functions for header verification#8339 Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress] #8342 Consensus: Trivial transform BOOST_FOREACH into for loop#8345 Introduce Consensus::GetFlags() and hide IsSuperMajority()#8346 Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs#8498 Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...#9260 Mrs Peacock in The Library with The Candlestick (killed main.{h,cpp})
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC