CKey: add Serialize and Unserialize #29295

Sjors · 2024-01-23T13:50:53Z

In #28983 I need to read and write two private keys to/from disk that are used by Stratum v2 peers to (optionally) authenticate us.

For the write part, I initially just put the key data into a std::vector<unsigned char> and then used a modified version of WriteBinaryFile. But @vasild pointed out in #29229 that:

CKey stores sensitive data and takes care to wipe it from memory when freed. In #28983 Read/WriteBinaryData() is used in a way that defeats that - the sensitive data will be copied to a temporary variable (ordinary std::vector) for IO. Can we change Read/WriteBinaryData() to operate directly on CKey in such a way that data goes directly from CKey to the disk?

This PR tries a different approach that hopefully addresses that. See Sjors#32 for how it's used (in sv2_template_provider.cpp).

DrahtBot · 2024-01-23T13:50:56Z

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.

Type	Reviewers
Stale ACK	shaavan, vasild

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.

Sjors · 2024-01-23T13:51:11Z

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

Sjors · 2024-01-23T14:17:49Z

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

vasild

Concept ACK

src/key.h

vasild · 2024-01-23T16:04:31Z

src/key.h

+    template<typename Stream>
+    void Serialize(Stream &s) const {
+        if (!fCompressed) {
+            throw std::ios_base::failure("Uncompressed key");


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

Good point.

Sjors · 2024-01-25T14:14:51Z

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

Keeping this draft pending #29307.

Sjors · 2024-01-25T14:18:09Z

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

shaavan

Code Review ACK 8c067ef

Notes:

The Unserialise function first reads the compression flag from the stream and then updates the actual keydata.
The Serialise function first writes the compression flag, followed by the keydata
The test appropriately verifies the added code for both the compressed and uncompressed keys.

maflcko

How is this different from CPrivKey?

src/test/key_tests.cpp

maflcko · 2024-01-26T14:36:59Z

src/key.h

+    inline void Unserialize(Stream& s) {
+        s >> fCompressed;
+        MakeKeyData();
+        s >> MakeWritableByteSpan(*keydata);


Not sure. IIUC the assumption is that keydata should not hold invalid keys. Can you explain why this assumption should be broken?

Do CKey::Check() on keydata[0]?

josibake · 2024-01-26T17:56:45Z

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

Sjors · 2024-01-29T10:41:05Z

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:

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.

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.

vasild

ACK 683f988

src/test/key_tests.cpp

Sjors · 2024-01-29T11:13:32Z

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

maflcko · 2024-01-29T11:23:21Z

src/key.h

+    }
+
+    template <typename Stream>
+    inline void Unserialize(Stream& s) {


template implies inline, so it can be removed. Also, could run clang-format on new code?

Done. Would be nice to do this in a pre-commit hook or something, because it's unlikely I'm going to remember doing this every time.

maflcko · 2024-01-29T11:24:31Z

src/key.h

+            // Uncompressed keys should be OpenSSL serialized instead
+            throw std::ios_base::failure("uncompressed key");
+        }
+        ::Serialize(s, MakeUCharSpan(*this));


Any reason to cast a byte span to a u-char span? I think you can replace all Make*Span() by just Span{}.

s << Span{*this};

Done in both places.

josibake · 2024-03-07T09:43:19Z

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.

Sjors · 2024-03-07T09:54:09Z

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).

josibake · 2024-03-07T10:10:43Z

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.

Sjors · 2024-03-07T10:20:05Z

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.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

This was referenced Jan 23, 2024

Make (Read/Write)BinaryFile work with char vector, use AutoFile #29229

Closed

Stratum v2 Template Provider (take 2) #28983

Closed

Sjors force-pushed the 2024/01/ckey_unserialize branch from f2684ab to 98ab264 Compare January 23, 2024 14:17

Sjors changed the title ~~CKey: add Unserialize~~ CKey: add Serialize and Unserialize Jan 23, 2024

Sjors mentioned this pull request Jan 23, 2024

Use AutoFile Sjors/bitcoin#32

Closed

vasild reviewed Jan 23, 2024

View reviewed changes

willcl-ark added Wallet Utils/log/libs labels Jan 25, 2024

Sjors force-pushed the 2024/01/ckey_unserialize branch from 98ab264 to 84ac6c3 Compare January 25, 2024 14:14

Sjors force-pushed the 2024/01/ckey_unserialize branch from 84ac6c3 to 8c067ef Compare January 25, 2024 14:16

Sjors marked this pull request as ready for review January 25, 2024 14:17

shaavan approved these changes Jan 26, 2024

View reviewed changes

DrahtBot requested a review from vasild January 26, 2024 14:06

maflcko reviewed Jan 26, 2024

View reviewed changes

Sjors force-pushed the 2024/01/ckey_unserialize branch from 8c067ef to 683f988 Compare January 29, 2024 10:41

vasild approved these changes Jan 29, 2024

View reviewed changes

src/test/key_tests.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from shaavan and removed request for shaavan January 29, 2024 10:52

Sjors force-pushed the 2024/01/ckey_unserialize branch from 683f988 to 3f11c15 Compare January 29, 2024 11:13

Sjors force-pushed the 2024/01/ckey_unserialize branch from 3f11c15 to 0f5d0a6 Compare January 29, 2024 11:14

maflcko reviewed Jan 29, 2024

View reviewed changes

DrahtBot requested review from shaavan and removed request for shaavan March 7, 2024 09:25

DrahtBot requested review from shaavan and removed request for shaavan March 7, 2024 09:43

DrahtBot requested review from shaavan and removed request for shaavan March 7, 2024 09:54

DrahtBot requested review from shaavan and removed request for shaavan March 7, 2024 10:10

DrahtBot requested review from shaavan and removed request for shaavan March 7, 2024 10:20

Sjors marked this pull request as draft March 7, 2024 10:20

DrahtBot requested review from shaavan and removed request for shaavan March 7, 2024 10:20

DrahtBot mentioned this pull request Mar 19, 2024

wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675

Open

DrahtBot added the Needs rebase label May 20, 2024

Sjors force-pushed the 2024/01/ckey_unserialize branch from af25cfc to 57fbd6c Compare May 28, 2024 10:58

DrahtBot removed the Needs rebase label May 28, 2024

DrahtBot mentioned this pull request May 28, 2024

crypto, refactor: add new KeyPair class #30051

Merged

This was referenced Jul 17, 2024

Silent Payments: sending #28201

Draft

Silent Payments: Implement BIP352 #28122

Draft

Silent payment index (for light wallets and consistency check) #28241

Closed

DrahtBot added the Needs rebase label Aug 7, 2024

CKey: add Serialize and Unserialize

8d35a9b

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Sjors force-pushed the 2024/01/ckey_unserialize branch from 57fbd6c to 8d35a9b Compare August 8, 2024 16:26

DrahtBot removed the Needs rebase label Aug 8, 2024

Sjors closed this Oct 16, 2024

CKey: add Serialize and Unserialize #29295

CKey: add Serialize and Unserialize #29295

Uh oh!

Conversation

Sjors commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Sjors commented Jan 23, 2024

Uh oh!

Sjors commented Jan 23, 2024

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 25, 2024

Uh oh!

Sjors commented Jan 25, 2024

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake commented Jan 26, 2024

Uh oh!

Sjors commented Jan 29, 2024

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jan 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake commented Mar 7, 2024

Uh oh!

Sjors commented Mar 7, 2024

Uh oh!

josibake commented Mar 7, 2024

Uh oh!

Sjors commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jan 23, 2024 •

edited

Loading

DrahtBot commented Jan 23, 2024 •

edited

Loading

maflcko Jan 29, 2024 •

edited

Loading

Sjors commented Mar 7, 2024 •

edited

Loading