Allow verify-commit.sh to just verify the HEAD commit (Use non-recursive verification by default) #9928

jonasschnelli commented at 12:38 PM on March 6, 2017: contributor

Currently, verify-commits.sh leads to a segfault 11 on my machine because of the limits of recursions (proof is here).

Until we have ported this to python (or fixed it in another way), we should offer a only-head-commit verification.

Allow verify-commit.sh to just verify the HEAD commit

Use non-recursive verification by default (for now)

08473ea60e

jonasschnelli added the label Scripts and tools on Mar 6, 2017

laanwj commented at 1:27 PM on March 6, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/9928/commits/08473ea60e667a80f7bc06696b6039ccd7b248d0

TheBlueMatt commented at 1:59 PM on March 6, 2017: member

Instead of defaulting to only enforce the top commit be signed, can we just remove the recursion? I changed it a while back to only follow the first merge branch which enforces structure on our git tree as well as checks signatures all the way down, so this should be trivial.

On March 6, 2017 8:27:43 AM EST, "Wladimir J. van der Laan" notifications@github.com wrote:

utACK https://github.com/bitcoin/bitcoin/pull/9928/commits/08473ea60e667a80f7bc06696b6039ccd7b248d0

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9928#issuecomment-284395335

in contrib/verify-commits/verify-commits.sh:None in 08473ea60e

  24 | @@ -25,10 +25,14 @@ IS_SIGNED () {
  25 |  	if ! git -c "gpg.program=${DIR}/gpg.sh" verify-commit $1 > /dev/null 2>&1; then
  26 |  		return 1;
  27 |  	fi
  28 | +  if [ $2 = false ]; then

TheBlueMatt commented at 3:29 PM on March 6, 2017:

The rest of the file uses tabs, you've introduced spaces.

laanwj commented at 4:04 PM on March 6, 2017: member

Instead of defaulting to only enforce the top commit be signed, can we just remove the recursion?

Sure, go ahead. The only point here is to stop travis from failing ASAP. Checking just the top commit is a workaround until the script can be fixed properly.

If you want to properly fix the script in one go, that's great, but please do it soon because the failling Travis is an annoyance and people will just ignore this check if it reports false positive after false positive.

MarcoFalke commented at 4:10 PM on March 6, 2017: member

@laanwj, travis specifically fails because bbd7579 and 589cd63 are not yet merged into master, not due to excessive recursion. Those are part of #9880, so imo either #9880 should be merged or the two commits cherry-picked into a separate pull.

On Mon, Mar 6, 2017 at 5:05 PM, Wladimir J. van der Laan notifications@github.com wrote:

Instead of defaulting to only enforce the top commit be signed, can we just remove the recursion?

Sure, go ahead. The only point here is to stop travis from failing ASAP. Checking just the top commit is a workaround until the script can be fixed properly.

If you want to properly fix the script in one go, that's great, but please do it soon because the failling Travis is an annoyance and people will just ignore this check if it reports false positive after false positive.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

laanwj commented at 4:12 PM on March 6, 2017: member

@MarcoFalke Thanks. It was not clear to me that #9880 was a fix. I held off merging it because I first wanted it to pass again before making the test more comphrehensive.

MarcoFalke commented at 4:15 PM on March 6, 2017: member

Note that the checks added in #9880 are off by default.

On Mon, Mar 6, 2017 at 5:12 PM, Wladimir J. van der Laan < notifications@github.com> wrote:

@MarcoFalke https://github.com/MarcoFalke Thanks. It was not clear to me that #9880 https://github.com/bitcoin/bitcoin/pull/9880 was a fix. I held off merging it because I first wanted it to pass again before making the test more comphrehensive.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9928#issuecomment-284444287, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv76DFGUNDYaJ2wYQhzISqB4dB_Qiks5rjDCKgaJpZM4MUGK4 .

MarcoFalke commented at 9:09 AM on March 7, 2017: member

Concept NACK. It is important that all commits are properly signed, changing it such that only HEAD needs to be signed is a loss in security by default.

laanwj commented at 9:33 AM on March 7, 2017: member

Concept NACK. It is important that all commits are properly signed, changing it such that only HEAD needs to be signed is a loss in security by default.

It was still a better security check than failing-all-the-time.

Anyhow, closing in favor of #9932

laanwj closed this on Mar 7, 2017

MarcoFalke commented at 10:40 AM on March 7, 2017: member

We should not use travis for security checks at all. I think the check was added to give maintainers a timely warning in case of an erroneous push.

On Tue, Mar 7, 2017 at 10:33 AM, Wladimir J. van der Laan < notifications@github.com> wrote:

Closed #9928 https://github.com/bitcoin/bitcoin/pull/9928.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9928#event-989230650, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvxIbaoWhYoMf3f2K8QQ6VQCyag0Aks5rjSR6gaJpZM4MUGK4 .

MarcoFalke commented at 10:41 AM on March 7, 2017: member

I think @jonasschnelli's issue is not yet solved. Can you check with master again, Jonas?

On Tue, Mar 7, 2017 at 11:40 AM, Marco Falke marco.falke@tum.de wrote:

We should not use travis for security checks at all. I think the check was added to give maintainers a timely warning in case of an erroneous push.

On Tue, Mar 7, 2017 at 10:33 AM, Wladimir J. van der Laan < notifications@github.com> wrote:

Closed #9928 https://github.com/bitcoin/bitcoin/pull/9928.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9928#event-989230650, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvxIbaoWhYoMf3f2K8QQ6VQCyag0Aks5rjSR6gaJpZM4MUGK4 .

jonasschnelli commented at 10:53 AM on March 7, 2017: contributor

Two problems I have on my local machine:

./contrib/verify-commits/verify-commits.sh: line 68: sha512sum: command not found (fixed it by changing sha512sum into shasum -a 512.
./contrib/verify-commits/verify-commits.sh Using verify-commits data from /Users/jonasschnelli/Documents/bitcoin/bitcoin/./contrib/verify-commits Segmentation fault: 11 (still have the recursive limit crash)

Either we remove the recursion (how easy?) or we migrate it to python. The recursion problem seems to be also happen on non-OSX operating systems.

TheBlueMatt commented at 2:22 PM on March 7, 2017: member

Yea, I think we should remove the recursion (should be easy, I'll look later today) and have a fallback for missing sha512sum to try the perl shasum. I'm not convinced we should only check the top commit.

On March 7, 2017 5:53:24 AM EST, Jonas Schnelli notifications@github.com wrote:

Two problems I have on my local machine:

./contrib/verify-commits/verify-commits.sh: line 68: sha512sum: command not found (fixed it by changing sha512sum into shasum -a 512.

./contrib/verify-commits/verify-commits.sh Using verify-commits data from /Users/jonasschnelli/Documents/bitcoin/bitcoin/./contrib/verify-commits Segmentation fault: 11 (still have the recursive limit crash)

Either we remove the recursion (how easy?) or we migrate it to python. The recursion problem seems to be also happen on non-OSX operating systems.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9928#issuecomment-284688473

MarcoFalke locked this on Sep 8, 2021