Make verify-commits.sh test that merges are clean #12708

sipa · 2018-03-16T17:28:41Z

Unsure if we want this.

This modifies verify-commits.sh to redo all merges along the leftmost commit branch (which includes all PR merges), and verify whether they match the merge commit's trees.

The benefit is that it will detect a case where one of the maintainers merges a PR, but makes an unrelated change inside the merge commit. This on itself is not very strong, as unrelated changes can also be included in the merged branch itself - but perhaps the merge commit is not something that people are otherwise likely to look at.

Fixes #8089

maflcko · 2018-03-16T17:33:29Z

Thanks. Concept ACK

…

On Mar 16, 2018 18:29, "Pieter Wuille" ***@***.***> wrote: Unsure if we want this. This modifies verify-commits.sh to redo all merges along the leftmost commit branch (so it should contain all PR merges), and verify whether the match the result of the merge. The benefit is that it will detect a case where one of the maintainers merges a PR, but makes an unrelated change inside the merge commit. This on itself is not very strong, as unrelated changes can also be included in the merged branch itself, but perhaps the merge commit is not something that people are otherwise likely to look. ------------------------------ You can view, comment on, or merge this pull request online at: #12708 Commit Summary - Make verify-commits.sh test that merges are clean File Changes - *M* contrib/verify-commits/verify-commits.sh <https://github.com/bitcoin/bitcoin/pull/12708/files#diff-0> (32) Patch Links: - https://github.com/bitcoin/bitcoin/pull/12708.patch - https://github.com/bitcoin/bitcoin/pull/12708.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#12708>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmv3mzklUc9NUFx-O3YjjuqTA6YX-Nks5te_aFgaJpZM4SuKjf> .

donaloconnor · 2018-03-16T18:09:10Z

Concept Ack

laanwj · 2018-03-22T09:11:44Z

Concept ACK, this makes sense because the merge script github-merge.py (that everyone is supposed to use) will only do clean merges.

How does this affect performance of the verify script? it's already very slow, so the difference might be negligible)
How deep should this check be done?

sipa · 2018-03-22T09:34:30Z

The script becomes several times slower (I didn't measure; just my impression from waiting... maybe a few minutes). Perhaps we should restrict it to just one day of commits or so?

maflcko · 2018-03-22T09:58:42Z

Similar to the SHA512 hash, we could only check it for HEAD. (Or maybe make that configurable to check all the way down to trusted_root if you have time)

maflcko · 2018-03-22T19:00:18Z

Fixes #8089

ryanofsky

utACK 577f111, just reviewing the shell script code. Concept also seems good. I'm almost surprised there is no standalone tool like git-is-clean-merge <commit>, since merge commits are more cumbersome to verify than normal commits.

laanwj · 2018-04-07T16:48:23Z

utACK 577f111

577f111 Make verify-commits.sh test that merges are clean (Pieter Wuille) Pull request description: Unsure if we want this. This modifies verify-commits.sh to redo all merges along the leftmost commit branch (which includes all PR merges), and verify whether they match the merge commit's trees. The benefit is that it will detect a case where one of the maintainers merges a PR, but makes an unrelated change inside the merge commit. This on itself is not very strong, as unrelated changes can also be included in the merged branch itself - but perhaps the merge commit is not something that people are otherwise likely to look at. Fixes #8089 Tree-SHA512: 2c020f5ac3f771ac775aa726832916bb8e03a311b2745d7a9825047239bd0660d838f086f3456f2bb05cea14c1529f74436b8cdd74cc94b70e40b4617309f62c

sipa · 2018-04-07T16:52:02Z

Let's observe how much this adds to Travis checks of master (it does not run on PR branches). If it's unacceptably slow, we can modify the script to only test merge cleanliness for the last few merges or days or so.

laanwj · 2018-04-07T17:20:21Z

Yes, I had the same reasoning, it's good to have this check in, if it becomes too slow we can always tone that down.

ken2812221 · 2018-04-23T12:57:49Z

#8672 is not clean, maybe we should ignore this

Merge commit 6052d509105790a26b3ad5df43dd61e7f1b24a12 is not clean
diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp
index 65144e7..32b3955 100644
--- a/src/qt/transactiondesc.cpp
+++ b/src/qt/transactiondesc.cpp
@@ -241,7 +241,7 @@ QString TransactionDesc::toHTML(CWallet *wallet, CWalletTx &wtx, TransactionReco
         strHTML += "<br><b>" + tr("Comment") + ":</b><br>" + GUIUtil::HtmlEscape(wtx.mapValue["comment"], true) + "<br>";
 
     strHTML += "<b>" + tr("Transaction ID") + ":</b> " + rec->getTxID() + "<br>";
-    strHTML += "<b>" + tr("Transaction total size") + ":</b> " + QString::number(wtx.GetTotalSize()) + " bytes<br>";
+    strHTML += "<b>" + tr("Transaction size") + ":</b> " + QString::number(wtx.GetTotalSize()) + " bytes<br>";
     strHTML += "<b>" + tr("Output index") + ":</b> " + QString::number(rec->getOutputIndex()) + "<br>";
 
     // Message from normal bitcoin:URI (bitcoin:123...?message=example)

e5b2cd8 Use python instead of slow shell script on verify-commits (Chun Kuan Lee) Pull request description: The cron job that runs every day would fail because of git checkout a single commit, not a branch. #12708 introduce a method to check whether merges are clean. However, there are four merges are not clean. So, I add a list of merges that are dirty and ignore them. Also, I modify the current shell script to python, it makes the script speed up a lot. The python code `tree_sha512sum` was copied from `github-merge.py` I've re-designed this. Now we verify all the things by default. - Add `--disable-tree-check` option, not to check SHA-512 tree - Add `--clean-merge NUMBER` option, only verify commits after <NUMBER> days ago Travis running time: |option|time| |-|-| |verify-commits.py|[25m47.02s(1547.02s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| |verify-commits.py --disable-tree-check|[19m10.08s(1150.08s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| |verify-commits.py --clean-merge 30|[9m18.18s(558.18s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| |verify-commits.py --disable-tree-check --clean-merge 30|[1m16.51s(76.51s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| Since the cron job always fail, I've created a respository to verify this daily. [![Build Status](https://travis-ci.org/ken2812221/bitcoin-verify-commits.svg?branch=master)](https://travis-ci.org/ken2812221/bitcoin-verify-commits) Tree-SHA512: 476bcf707d92ed3d431ca5642e013036df1506120d3dd2aa718f74240063ce856abd78f4c948336c2a6230dfe5c60c6f2d52d19bdb52d647a1c5f838eaa02e3b

…travis e5b2cd8 Use python instead of slow shell script on verify-commits (Chun Kuan Lee) Pull request description: The cron job that runs every day would fail because of git checkout a single commit, not a branch. bitcoin#12708 introduce a method to check whether merges are clean. However, there are four merges are not clean. So, I add a list of merges that are dirty and ignore them. Also, I modify the current shell script to python, it makes the script speed up a lot. The python code `tree_sha512sum` was copied from `github-merge.py` I've re-designed this. Now we verify all the things by default. - Add `--disable-tree-check` option, not to check SHA-512 tree - Add `--clean-merge NUMBER` option, only verify commits after <NUMBER> days ago Travis running time: |option|time| |-|-| |verify-commits.py|[25m47.02s(1547.02s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| |verify-commits.py --disable-tree-check|[19m10.08s(1150.08s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| |verify-commits.py --clean-merge 30|[9m18.18s(558.18s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| |verify-commits.py --disable-tree-check --clean-merge 30|[1m16.51s(76.51s)](https://travis-ci.org/ken2812221/bitcoin/jobs/373321423)| Since the cron job always fail, I've created a respository to verify this daily. [![Build Status](https://travis-ci.org/ken2812221/bitcoin-verify-commits.svg?branch=master)](https://travis-ci.org/ken2812221/bitcoin-verify-commits) Tree-SHA512: 476bcf707d92ed3d431ca5642e013036df1506120d3dd2aa718f74240063ce856abd78f4c948336c2a6230dfe5c60c6f2d52d19bdb52d647a1c5f838eaa02e3b

577f111 Make verify-commits.sh test that merges are clean (Pieter Wuille) Pull request description: Unsure if we want this. This modifies verify-commits.sh to redo all merges along the leftmost commit branch (which includes all PR merges), and verify whether they match the merge commit's trees. The benefit is that it will detect a case where one of the maintainers merges a PR, but makes an unrelated change inside the merge commit. This on itself is not very strong, as unrelated changes can also be included in the merged branch itself - but perhaps the merge commit is not something that people are otherwise likely to look at. Fixes bitcoin#8089 Tree-SHA512: 2c020f5ac3f771ac775aa726832916bb8e03a311b2745d7a9825047239bd0660d838f086f3456f2bb05cea14c1529f74436b8cdd74cc94b70e40b4617309f62c

Make verify-commits.sh test that merges are clean

577f111

fanquake added the Scripts and tools label Mar 16, 2018

laanwj mentioned this pull request Mar 29, 2018

doc: Switch release-notes.md to union merge #12823

Closed

ryanofsky reviewed Apr 3, 2018

View reviewed changes

laanwj merged commit 577f111 into bitcoin:master Apr 7, 2018

This was referenced Apr 23, 2018

Let travis run verify-commits.sh without errors #13065

Closed

Migrate verify-commits script to python, run in travis #13066

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make verify-commits.sh test that merges are clean #12708

Make verify-commits.sh test that merges are clean #12708

Uh oh!

sipa commented Mar 16, 2018 •

edited by laanwj

Loading

Uh oh!

maflcko commented Mar 16, 2018 via email

Uh oh!

donaloconnor commented Mar 16, 2018

Uh oh!

laanwj commented Mar 22, 2018 •

edited

Loading

Uh oh!

sipa commented Mar 22, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

ryanofsky left a comment

Uh oh!

laanwj commented Apr 7, 2018

Uh oh!

sipa commented Apr 7, 2018

Uh oh!

laanwj commented Apr 7, 2018

Uh oh!

ken2812221 commented Apr 23, 2018 •

edited

Loading

Uh oh!

Uh oh!

Make verify-commits.sh test that merges are clean #12708

Make verify-commits.sh test that merges are clean #12708

Uh oh!

Conversation

sipa commented Mar 16, 2018 • edited by laanwj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 16, 2018 via email

Uh oh!

donaloconnor commented Mar 16, 2018

Uh oh!

laanwj commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Mar 22, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

maflcko commented Mar 22, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 7, 2018

Uh oh!

sipa commented Apr 7, 2018

Uh oh!

laanwj commented Apr 7, 2018

Uh oh!

ken2812221 commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sipa commented Mar 16, 2018 •

edited by laanwj

Loading

laanwj commented Mar 22, 2018 •

edited

Loading

ken2812221 commented Apr 23, 2018 •

edited

Loading