Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verify-commits: Bump trusted git root to after most recent laanwj merge #27076

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

achow101
Copy link
Member

To prepare for the removal of laanwj's key from trusted key (#27054), the trusted git root needs to be newer than the most recent merge commit signed by his key.

This can be tested by removing the laanwj's key from trusted keys (e.g. by merging with #27054) and running verify-commits.py with --clean-merge 0: ./contrib/verify-commits/verify-commits.py --clean-merge 0 HEAD~. (--clean-merge 0 disables the clean merge check which will checkout some commits, which results in the trusted-keys used in checking of subsequent commits to be different than expected).

To prepare for the removal of laanwj's key from trusted key, the trusted
git root needs to be newer than the most recent merge commit signed by
his key.
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 10, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake, hebasto
Approach ACK MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@Sjors
Copy link
Member

Sjors commented Feb 11, 2023

I would prefer a solution (#27058?) that allows checking commits by retired maintainers, at least up to a few years back (e.g. the branch-off point for the last backport supported version).

@maflcko
Copy link
Member

maflcko commented Feb 11, 2023

You can do git checkout "$(cat contrib/verify-commits/trusted-git-root)~1" and then verify the previous commits, as often as you want, to go as far back as you want.

@Sjors
Copy link
Member

Sjors commented Feb 11, 2023

That's a good hint to add to the README. But having the root key not too recent still seems better, all things equal. I think we should only bump it if a former maintainer revokes their key or it expires, plus maybe every couple of years to preempt such issues. So far #27058 seems compatible with that approach (update: not anymore, moving discussion there)

@maflcko
Copy link
Member

maflcko commented Feb 11, 2023

I think we should only bump

Is there any reason for this? See also #27058 (comment)
What would the alternative be? Listing hundreds or thousands of "revsig" commits in a file, to ensure it is impossible to review manually, only with special git commands, potentially making it trivial to sneak in malicious commits that are not actually revsig commits? And then, as you say, bump the root anyway later.

@Sjors
Copy link
Member

Sjors commented Feb 11, 2023

@MarcoFalke answered in #27058 (comment). Depending on what we do there this PR can either be closed or merged.

@maflcko
Copy link
Member

maflcko commented Feb 14, 2023

lgtm, Approach ACK

I haven't tested this, because I use a trusted git root set by myself anyway, but I can't see a reason not to do this. This was also done last time in commit d4b3dc5, so it makes sense to do the same approach again.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6ada37d

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 6ada37d44cce7fa3a729de72cede4c1f3bc675ce
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEz7FuIclQ9n+pXlWPLuufXMCVJsEFAmPrwdkACgkQLuufXMCV
JsHyExAApfaVBqnmRs6raV8ky90kLLiesvYjgebHyJAq059OZvlROziNC43FAlls
xnhDxMYqUB8rFRGpPccqI4oi+tpSFUrS44MP+UgMzkq1mrt7Tfx7cjTQPkB21YFD
FYrTZT5hE7FLQvkf7yDD8OobkZOZQl5wIKE+VNW3TW0oBWnfjXY5tw1vh51Qta1x
9B7UrSRJJ3SFD/xmLYDtCtzRDraKg44fRxKf9A5rvXaWdfR3s8pBZuVXJePveCTv
YQjKQtFxRIjiWaFNW+ERD0xxq0B6z3/LvocqXJwo2Mryc4tUMsqZuopze6V8oIUB
GWxSRxDV6bumWxxBqQbvfZJhwJY+Jstm2JD7TxDtwvgJe0Wnb0sTsGBpkFFNXORy
9QPCcYPlW3+YIT2Z/fDhg1DQd4+2JhWtaGyguB/yHly7j2A+jRGBqtFATNFUtz/r
cfTdDPXR3fvXFh3hjcKKCvcwKrPbfU4SXqAU0qlXX7DprbWkpQpt01Jg5rNBINlC
j/tsKNxeMj1kNeCYy4muDqC5HRA2gsqlb+bEeyndg3PlPyypi0d8Aj2S7/BgXUU/
FKSgXWMEagBXHklmYJnMRtt85RDsOfDKW7znecGDqDBlX2qA8pZ1SNaVWYhFvU2Y
iePmdIgEuhpn19xQwjSu+M4ITVNb0C9jSqY0IUF5gK3OZh70WGw=
=Bln+
-----END PGP SIGNATURE-----

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6ada37d, I've verified the history of laanwj's merge commits.

@fanquake fanquake merged commit 2b0cd76 into bitcoin:master Feb 15, 2023
@glozow
Copy link
Member

glozow commented Feb 15, 2023

post merge ACK 6ada37d

@bitcoin bitcoin locked and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants