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

key, rpc: fix key parsing #2682

Merged
merged 3 commits into from Apr 23, 2023
Merged

Conversation

div72
Copy link
Member

@div72 div72 commented Apr 23, 2023

Closes #2681.

This reverts commit 8674101.

This assumption is not solid for any pre-5.4.0.0 wallets which can
contain OpenSSL generated keys.
Rationale:
    HEX encoded keys do not denote enough information on whether the key
    should be used with compressed or uncompressed public keys. Even
    though these interfaces should not be affected, better be consistent
    than sorry.
Rationale:
    Our DER parser is not sophisticated enough to determine whether the
    provided key should be accompanied by a compressed or uncompressed
    public key, so just accept WIF which already has that data.
@div72 div72 requested a review from jamescowens April 23, 2023 01:19
@div72 div72 self-assigned this Apr 23, 2023
@jamescowens jamescowens added this to the Miss Piggy milestone Apr 23, 2023
@div72 div72 changed the base branch from development to hotfix April 23, 2023 01:38
@jamescowens
Copy link
Member

jamescowens commented Apr 23, 2023

  • Tested dumpprivkey for wallet with known discrepancies <= 5.4.3.0. Passed.
  • Checked dumpwallet with 5.4.3.0 and this PR. Identical key output. Passed.
  • Create blank non HD wallet in 5.4.1.0. Change to this PR and then load problem keys individually using importprivkey. Verify keys imported correctly. Passed.
  • Create blank HD wallet with this PR. Load problem keys individually using importprivkey. Verify keys imported correctly. Passed

@jamescowens jamescowens merged commit 678b102 into gridcoin-community:hotfix Apr 23, 2023
9 checks passed
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

tACK

jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Apr 23, 2023
This is an _important_ leisure release and follows right on the heals of
5.4.4.0, because we fixed one thing only to break another in a worse way. By
worse I mean that some people with old keys could get a corrupt wallet message
on startup. In troubleshooting this issue, as it turns out, there is more to
the story than just the compressed flag not being set properly. Gridcoin has
been around a long time. A number of older wallets may have keys generated by
openssl that aren't handled correctly by the secp256k1 DER parser. We switched
over to secp256k1 from openssl for keys in 5.4.0.0, to align with Bitcoin
upstream. It is not entirely clear why Bitcoin does not see this issue with old
keys. Regardless, to be safe, we have disabled the ability to import HEX
formatted keps with importprivkey. This loss of HEX import functionality should
affect essentially no one, since the Base58, WIF, form is what everybody uses.

*It is highly recommended that EVERYONE upgrade to this release.*

Added
none

Changed
none

Removed
none

Fixed
 - key, rpc: fix key parsing gridcoin-community#2682 (@div72)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wallet states that wallet.dat is corrupted
2 participants