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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Opened as draft because I suspect this can be done less verbosely.

Added Serialize for completeness. This only saves me a MakeUCharSpan(key).

Switched to the approach suggested by @vasild. Also supports uncompressed keys. Added test.

Keeping this draft pending #29307.

Come to think of it, this PR only changes serialization, while #29307 deals with storage. So this is ready for review.

I need to read and write two private keys to/from disk

Not familiar with the stratumv2 spec, but this seems odd to me and also not a good fit for CKey. FWIW, there is CPrivKey for producing a serialized OpenSSL private key

I added Check() when deserialising.

Not familiar with the stratumv2 spec, but this seems odd to me

In Stratum v2 a pool or Template Provider (implemented in #28983) may (optionally) have an identity. This is necessary to prevent stealing hash rate (at least for the pool). In order to protect that identity in the long run, you can keep its private key on a different machine, generate a static key for the server and sign a certificate for that static key.

not a good fit for CKey

Why would you not use a CKey to hold a private key?

FWIW, there is CPrivKey for producing a serialized OpenSSL private key

TIL. There's two issues with CPrivKey:

1. It's designed to always be used in conjunction with a public key. That makes sense for the wallet which has to deal with encrypted key, and where any data corruption is catastrophic.

With Stratum v2 you can just make a new static key if something goes wrong. It's more similar to how we handle Tor v3 and I2P private keys.

It's possible to use the confusingly named GetPrivKey() instead of <<, and it's possible to Load() with a dummy public key and fSkipCheck = false. But having straight-forward << and >> seems cleaner.

2. It uses the needlessly large OpenSSL encoding. At least in Stratum v2 there's never a need to import or export private keys. We just need to store them. The encoding is also incorrect (for Stratum v2): it just pretends to be a compressed key, but in reality public keys are encoded as x-only. There's no point in shoe-horning data into a standard we don't need and use incorrectly. On the other hand, there's no harm in it - the wallet does it too.

I could switch to OpenSSL encoding. Either by moving the code from GetPrivKey to Serialize(), or vice versa, call GetPrivKey() from Serialize. Load would need a more involved refactor to separate out the dependency.

@vasild wrote earlier:

Is it not worth saving CKey::fCompressed to disk as well and fully support ser/unser of any CKey?

I added this before, but given the existence of CPrivKey I'm tempted to drop this. That makes it more clear that is only for keys that are intended to be used with Schnorr signatures, x-only ECDH, etc. I.e. keys for which there is no valid DER encoding.

I dropped support for uncompressed keys and added a comment to explain the difference with OpenSSL encoding. Also added test for invalid keys.

others copy-paste my example in stead of repeating my original mistake

I really don't expect serializing and unserializing keys to disk (outside of the existing wallet use case) to be a common pattern. As @ryanofsky mentioned, I think it's good to have a little friction here to make the implementer think "should I really be doing this?"

This seems an overkill way to avoid adding a standard serialization method.

I don't think it's fair to claim you are adding a standard method here. As I mentioned before:

I'd be more inclined if this PR was proposing a general refactor that got rid of existing 'needlessly complicated ways' of serializing/unserializing, rather than just adding yet another method. That way, we end up with a well documented and standard way of doing things.

But this PR is only adding yet another method, which is unnecessary considering your use case can be accomplished with the existing methods.

I don't think it's fair to claim you are adding a standard method here.

By standard, I mean the same way we serialise other objects.

I do agree it's not ideal to have multiple serialisation standards for the same object. A further refactor could address that, e.g. in a way similar to how we can serialize a CTransaction with or without witness (TX_WITH_WITNESS / TX_NO_WITNESS).

A further refactor could address that

I think a better approach would be to use the existing methods for your use case and then if we start seeing more use cases pop up where people absolutely need to serialize and unserialize raw CKeys to disk, we do a general refactor of CKey that proposes standard methods for serializing/unserializing and removes the old ways of doing it.

I think a better approach would be to use the existing methods for your use case

As discussed a further above (#29295 (comment)) there's quite a few other things I need to get merged for Stratum v2 before this PR becomes a potential blocker. It's only at that point that I'll try other approaches.

I'll mark this as draft for now.