Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Fix keygen usb panic (debug only) #31194

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

CriesofCarrots
Copy link
Contributor

Problem

$ solana-keygen pubkey usb://ledger
thread 'main' panicked at '`confirm_key` is not a name of an argument or a group.
Make sure you're using the name of the argument itself and not the name of short or long flags.', /Users/tyeraeulberg/.cargo/registry/src/git.1-hub.cn-1ecc6299db9ec823/clap-3.1.8/src/parse/matches/arg_matches.rs:659:14

In clap v3+, ArgMatches::is_present() panics if the arg named is not actually present in the arg definition: https://docs.rs/clap/3.1.5/clap/struct.ArgMatches.html#method.is_present
We were using is_present under the hood in signer resolution to check whether a remote wallet should prompt the user to confirm a used key. But not all callers of those signer-resolution methods are Args that include the confirm_key definition; solana-keygen, for example, does not. So when #24479 upgraded keygen to clap-v3-utils, it introduced a potential panic (only in debug, at least in our current clap version, v3.1.5).

Summary of Changes

Update clap-v3 to newest patch, which includes a fallible ArgMatches::try_contains_id() method
Use it in usb signer resolution

Closes #31192

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

:shipit:

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Apr 14, 2023
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Apr 14, 2023
@mergify
Copy link
Contributor

mergify bot commented Apr 14, 2023

automerge label removed due to a CI failure

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #31194 (7bafd7c) into master (768405c) will increase coverage by 0.0%.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##           master   #31194   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         729      729           
  Lines      206304   206321   +17     
=======================================
+ Hits       168281   168336   +55     
+ Misses      38023    37985   -38     

@mergify mergify bot merged commit 2147f0d into solana-labs:master Apr 14, 2023
mergify bot pushed a commit that referenced this pull request Apr 14, 2023
* Bump clap v3

* Use fallible method to check for confirm_key

(cherry picked from commit 2147f0d)

# Conflicts:
#	Cargo.lock
#	clap-v3-utils/Cargo.toml
CriesofCarrots pushed a commit that referenced this pull request Apr 14, 2023
* Fix keygen usb panic (debug only) (#31194)

* Bump clap v3

* Use fallible method to check for confirm_key

(cherry picked from commit 2147f0d)

# Conflicts:
#	Cargo.lock
#	clap-v3-utils/Cargo.toml

* Fix conflicts

---------

Co-authored-by: Tyera <tyera@solana.com>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Apr 14, 2023
* Bump clap v3

* Use fallible method to check for confirm_key
@CriesofCarrots CriesofCarrots deleted the fix-keygen-usb branch May 25, 2023 22:11
bw-solana pushed a commit to bw-solana/solana that referenced this pull request Jan 10, 2025
…) (solana-labs#31195)

* Fix keygen usb panic (debug only) (solana-labs#31194)

* Bump clap v3

* Use fallible method to check for confirm_key

(cherry picked from commit 2147f0d)

* Fix conflicts

---------

Co-authored-by: Tyera <tyera@solana.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants