devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private. #10189

1. 
   theuni commented at 8:35 pm on April 11, 2017: member

   This should really be 2 PRs, but the CNode change is helpful to show the script’s usefulness.

   Scriptable changes verifier

   We often make simple changes that produce large diffs, but are easily scriptable. A large search+replace like #9902 is the most obvious example.

   It’s useful both for reviewers, and for posterity, to include these transforming scripts in the commit message. And once there, we can use c-i to verify the scripted changes before merging.

   A simple script checks for commits containing the line: -BEGIN VERIFY SCRIPT-, and reads until it the line: -END VERIFY SCRIPT-, or the end of the commit message.

   The resulting script should exactly transform the previous commit into the current one. Any remaining diff signals an error.

   Travis will check each commit for these scripts, and fail if any script fails.

   Make CNode’s id field private

   This was useful to check the safety of #10176 (making it easier to see where the node’s id is used), and is a general encapsulation improvement.

2. theuni force-pushed on Apr 11, 2017
3. 
   jtimon commented at 9:02 pm on April 11, 2017: contributor

   This is a great idea I plan to heavily use. utACK 4759716 besides the script itself which I still have some doubts about. Probably instead of learning the git-fu involved now I could simply go ahead and test it by writing something based on this.
4. theuni force-pushed on Apr 11, 2017
5. 
   theuni commented at 9:21 pm on April 11, 2017: member

   @jtimon You’re right to doubt the script, it’s quite kludgy. it works by rewinding to the previous commit, running the supplied transform script, and comparing the result to the actual commit.

   Because the script needs to work on real files, I’m not sure how to avoid working on the current repo, other than maybe setting up a dummy index/workdir.

6. theuni force-pushed on Apr 11, 2017
7. 
   jtimon commented at 4:09 am on April 12, 2017: contributor

   ACK 6ca7f42 conditional to 74214a2 failing on travis but the whole PR squashed as a single commit passing. See #10193
8. fanquake added the label Refactoring on Apr 12, 2017
9. fanquake added the label Scripts and tools on Apr 12, 2017
10. 
    jtimon commented at 8:03 pm on April 12, 2017: contributor

    ACK 6ca7f42 Travis fails if not all the changes performed by the script are included in the commit as expected https://travis-ci.org/bitcoin/bitcoin/jobs/221211199 I will now squash #10193 so that it passes.
11. 
    TheBlueMatt commented at 1:55 pm on April 13, 2017: member

    As discussed on IRC, making the script require some prefix in the commit title (ie first line) would help make sure people see it.
12. theuni force-pushed on Apr 18, 2017
13. 
    theuni commented at 10:07 pm on April 20, 2017: member

    Added enforcement of the prefix: “scripted-diff:” in the first line.
14. 
    luke-jr commented at 10:44 pm on April 20, 2017: member

    Relying on Travis to do review is a bad thing, and as-implemented, it’s not very safe for reviewers to run manually.

    But more annoyingly, can the prefix be on a not-first-line where it pollutes the commit summary? :/

15. 
    theuni commented at 10:50 pm on April 20, 2017: member

    @luke-jr it’s enforced in the subject line so that reviewers can’t miss it. That was the resounding request during the meeting.

    It can be fixed up to (optionally) require approval before running. Would that make you comfortable enough to run it locally?

16. theuni force-pushed on Apr 20, 2017
17. 
    theuni commented at 10:52 pm on April 20, 2017: member

    rebased at @jtimon’s request
18. 
    luke-jr commented at 11:03 pm on April 20, 2017: member

    I wouldn’t be comfortable with anything less than reading the script and manually running it explicitly…
19. 
    theuni commented at 11:13 pm on April 20, 2017: member

    @luke-jr You’re free to c/p the script and run it, then. I understand your hesitance, but this doesn’t take away anything, only adds a check which can be ignored.
20. 
    luke-jr commented at 11:20 pm on April 20, 2017: member

    It takes away from readability of single-line summaries. And gives people a false sense of security if Travis is trusted.
21. in contrib/devtools/commit-script-check.sh:33 in e65b300d0f outdated

    28+        echo "Failed"
    29+        RET=1
    30+    else
    31+        echo "Running script for: $i"
    32+        echo "$SCRIPT"
    33+        eval "$SCRIPT"
    

    ---

    ---

    

    MarcoFalke commented at 1:45 pm on April 23, 2017:

    Maybe wait for user input prior to eval to give the user time to actually read the script and exit on doubt. (travis can use –force-eval)

    see discussion in #10193 (comment)

22. fanquake added this to the "Done" column in a project

    ---

23. fanquake moved this from the "Done" to the "In progress" column in a project

    ---

24. fanquake removed this from the "In progress" column in a project

    ---

25. 
    laanwj commented at 9:41 am on April 26, 2017: member

    Concept ACK

    @luke-jr You’re free to c/p the script and run it, then. I understand your hesitance, but this doesn’t take away anything, only adds a check which can be ignored.

    I agree. We already use Travis for many checks, adding one more isn’t problematic. It should not take away from people manually reviewing or running tests locally.

26. 
    jtimon commented at 1:16 pm on April 26, 2017: contributor

    Needs rebase, re-utACK except for the script itself, which I don’t fully understand but I have tested using travis. I don’t plan to ever run it locally. The idea of using shell scripts for certain refactor changes and put the commands used in the commit message for easier review is awesome and I’m going to use it whether this check is done by travis or not. Even if reviewers don’t run the scripts locally to make sure the changes are complete, they can point it out if the commit is doing something else than what would running the script could possibly do.

    It takes away from readability of single-line summaries.

    No, the single line summary can still be very clear, and the script in the detailed message just says the same thing in a more formal way.

    And gives people a false sense of security if Travis is trusted.

    It doesn’t give me any false sense of security and from what you say it doesn’t give it to you either. Perhaps we should make clearer somewhere that travis doesn’t replace review and testing and that commits prefixed with “scripted-diff” need review like any other commit.

    If we’re going to keep discussing it, maybe the PR should be separated in the script itself and the changes to net_processing, since the scripted-diff commits can be merged before the script itself is merged anyway (and the script itself will much more unlikely require rebase).

27. theuni force-pushed on Apr 28, 2017
28. 
    theuni commented at 0:59 am on April 28, 2017: member

    Rebased. If this ends up needing another rebase later due to another net change, I’ll split it up.

    I’d rather hold off on making the interactive change until this gets a little real-world testing. I’m sure some other things worth addressing will crop up.

29. 
    TheBlueMatt commented at 8:28 pm on May 1, 2017: member

    utACK 644cb250e708b5724a0879f26449baa5544d35cf (needs squash, of course)
30. devtools: add script to verify scriptable changes e50c33ea27
31. scripted-diff: net: Use accessor rather than node's id directly

    -BEGIN VERIFY SCRIPT-
    sed -i "s/\(node\|to\|from\)->id/\1->GetId()/" src/net.cpp src/net_processing.cpp
    -END VERIFY SCRIPT-

    9ff0a51164
32. net: make CNode's id private 0f3471f3ad
33. theuni force-pushed on May 4, 2017
34. 
    theuni commented at 5:06 am on May 4, 2017: member

    Squashed with no changes.
35. 
    jtimon commented at 7:39 pm on May 4, 2017: contributor

    As mentioned on IRC, I drafted some documentation for this, not sure where to put it, maybe contributing.md : https://0bin.net/paste/mTtIoaSoedZ3al-y#dWgD7aBKElhKofVS5nHJBvuvlDLRFmgRy8a03KroOFM
36. jtimon referenced this in commit 15e43508fa on May 6, 2017
37. 
    MarcoFalke commented at 11:27 am on May 6, 2017: member

    utACK e50c33ea27564a32a1d40e112a5547f0003093af
38. laanwj merged this on May 7, 2017
39. laanwj closed this on May 7, 2017

    ---

40. laanwj referenced this in commit 750c5a5b84 on May 7, 2017
41. PastaPastaPasta referenced this in commit d868a826bf on Jun 10, 2019
42. PastaPastaPasta referenced this in commit c503307064 on Jun 10, 2019
43. PastaPastaPasta referenced this in commit 9a458ffede on Jun 10, 2019
44. PastaPastaPasta referenced this in commit f378fd1039 on Jun 10, 2019
45. PastaPastaPasta referenced this in commit 292c401119 on Jun 10, 2019
46. PastaPastaPasta referenced this in commit 91577e8db1 on Jun 10, 2019
47. PastaPastaPasta referenced this in commit 2741afb070 on Jun 11, 2019
48. PastaPastaPasta referenced this in commit 77c5a7ad36 on Jun 11, 2019
49. PastaPastaPasta referenced this in commit fa146c38f3 on Jun 15, 2019
50. PastaPastaPasta referenced this in commit b734b9c109 on Jun 19, 2019
51. PastaPastaPasta referenced this in commit 94417e544c on Jun 19, 2019
52. PastaPastaPasta referenced this in commit 98c936c1e2 on Jun 19, 2019
53. PastaPastaPasta referenced this in commit dc178d4dbb on Jun 19, 2019
54. PastaPastaPasta referenced this in commit 62f7937259 on Jun 19, 2019
55. PastaPastaPasta referenced this in commit 2edd094a21 on Jun 19, 2019
56. PastaPastaPasta referenced this in commit b2b5932441 on Jun 19, 2019
57. PastaPastaPasta referenced this in commit 90562be424 on Jun 20, 2019
58. barrystyle referenced this in commit dac3b78daf on Jan 22, 2020
59. zkbot referenced this in commit 311a079dd5 on Oct 27, 2020
60. MarcoFalke locked this on Sep 8, 2021

Contributors
theuni jtimon TheBlueMatt luke-jr MarcoFalke laanwj

Labels
Refactoring Scripts and tools