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 force-pushed on Jul 14, 2016
  7. 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

  8. jtimon force-pushed on Jul 14, 2016
  9. 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.
  10. NicolasDorier commented at 8:27 am on July 15, 2016: contributor
    this #8342 would remove the dependency to boost
  11. 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.
  12. jtimon force-pushed on Jul 15, 2016
  13. 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.

  14. btcdrak commented at 11:57 am on August 29, 2016: contributor
    needs rebase
  15. jtimon force-pushed on Aug 30, 2016
  16. jtimon commented at 1:04 pm on August 30, 2016: contributor
    Rebased
  17. jtimon force-pushed on Sep 1, 2016
  18. jtimon commented at 4:17 pm on September 1, 2016: contributor
    Needed rebase again.
  19. jtimon force-pushed on Oct 19, 2016
  20. 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 .
  21. jtimon force-pushed on Nov 21, 2016
  22. jtimon commented at 7:07 pm on November 21, 2016: contributor
    Needed rebase.
  23. 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.
  24. MarcoFalke commented at 10:25 am on November 25, 2016: member
    utACK 08f4cf00daed646f25ae25d40c700bed1458f4cc
  25. afk11 commented at 11:08 am on November 25, 2016: contributor
    utACK 08f4cf0
  26. btcdrak commented at 2:33 pm on November 25, 2016: contributor
    utACK 08f4cf00daed646f25ae25d40c700bed1458f4cc
  27. 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:

    0set<COutPoint> vInOutPoints;

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

  28. 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
  29. in src/main.cpp: 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?
  30. morcos commented at 10:00 pm on November 26, 2016: member
    utACK 08f4cf00da
  31. jtimon force-pushed on Nov 27, 2016
  32. jtimon commented at 6:58 pm on November 27, 2016: contributor
    Fixed @morcos ’s nits.
  33. morcos commented at 10:44 pm on November 27, 2016: member
    utACK 4ed06095
  34. jtimon force-pushed on Dec 3, 2016
  35. jtimon commented at 6:13 am on December 3, 2016: contributor
    Needed rebase after renaming main.o, see #9260
  36. jtimon commented at 4:19 pm on January 3, 2017: contributor
    Needs rebase again
  37. jtimon force-pushed on Jan 24, 2017
  38. jtimon force-pushed on Jan 31, 2017
  39. jtimon commented at 11:29 pm on January 31, 2017: contributor
    Rebased
  40. MOVEONLY: tx functions to consensus/tx_verify.o 
    Functions related to transaction verification.
    618d07faa2
  41. jtimon force-pushed on Apr 6, 2017
  42. jtimon commented at 9:37 pm on April 6, 2017: contributor
    Needed rebase
  43. 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
  44. 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.
  45. ryanofsky commented at 7:27 pm on May 8, 2017: member
    utACK 618d07faa2cc2f061a2e8035c3edbffc192480d7. Confirmed this is move only, except for the EvaluateSequenceLocks changes noted.
  46. ryanofsky commented at 3:08 pm on May 10, 2017: member
    This seems ready to be merged. Has like 6 acks.
  47. 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.

  48. 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)
  49. laanwj merged this on May 18, 2017
  50. laanwj closed this on May 18, 2017
  51. laanwj referenced this in commit ea6fde3f1d on May 18, 2017
  52. jtimon deleted the branch on May 18, 2017
  53. fanquake moved this from the "In progress" to the "Done" column in a project
  54. PastaPastaPasta referenced this in commit c2ba1012fb on Jun 10, 2019
  55. PastaPastaPasta referenced this in commit 36f39e3307 on Jun 20, 2019
  56. PastaPastaPasta referenced this in commit b206fa58bc on Jul 13, 2019
  57. PastaPastaPasta referenced this in commit 2c62cefe7b on Jul 17, 2019
  58. PastaPastaPasta referenced this in commit 74a394129c on Jul 17, 2019
  59. UdjinM6 referenced this in commit 7e4318dda8 on Jul 23, 2019
  60. barrystyle referenced this in commit 381790c4b1 on Jan 22, 2020
  61. DrahtBot locked this on Sep 8, 2021


jtimon afk11 NicolasDorier btcdrak MarcoFalke paveljanik morcos ryanofsky mchrostowski laanwj

Labels
Refactoring

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: 2024-11-23 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me