Sjors
commented at 1:50 pm on January 23, 2024:
member
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 #28983Read/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 https://github.com/Sjors/bitcoin/pull/32 for how it’s used (in sv2_template_provider.cpp).
DrahtBot
commented at 1:50 pm on January 23, 2024:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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
commented at 1:51 pm on January 23, 2024:
member
Opened as draft because I suspect this can be done less verbosely.
Sjors force-pushed
on Jan 23, 2024
Sjors
commented at 2:17 pm on January 23, 2024:
member
Added Serialize for completeness. This only saves me a MakeUCharSpan(key).
Sjors renamed this:
CKey: add Unserialize
CKey: add Serialize and Unserialize
on Jan 23, 2024
Use secure_allocator for the vector because it also ensures the data is not swapped out to disk. Then you do not need the memory_cleanse() call at the end:
Wait, I forgot that this is inside a CKey method and we have access to the private keydata member. Should be possible to write directly to it, without an intermediate vector. Something like:
216+
217+ /** Can only be used for compressed keys, because Unserialize assumes that. */
218+ template<typename Stream>
219+ void Serialize(Stream &s) const {
220+ if (!fCompressed) {
221+ throw std::ios_base::failure("Uncompressed key");
maflcko
commented at 2:38 pm on January 26, 2024:
member
How is this different from CPrivKey?
josibake
commented at 5:56 pm on January 26, 2024:
member
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
commented at 10:41 am on January 29, 2024:
member
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.
Sjors force-pushed
on Jan 29, 2024
in
src/test/key_tests.cpp:410
in
683f988f90outdated
josibake
commented at 11:42 am on January 29, 2024:
member
Why would you not use a CKey to hold a private key?
My point was not “CKey shouldn’t be used for private keys.” My point is CKey (or at least my understanding of it) is meant for a specific kind(s) of “private key” i.e. a private key that represents a private/public key pair and is intended to be used in that context. It also seems we want to know where the key came from and ensure that it is valid, hence the only ways to load a secret key are the DecodeSecret method, which decodes a BIP32 ExtPrivKey string, and the Load method which takes an openssl key and verifies the corresponding public key (I suspect fSkipCheck is only there for performance reasons and not to indicate “use skipCheck if you want to load generic secret data without a public key”).
Again, I don’t fully understand your usecase, but adding generic methods to de-serialize bytes from anywhere into a CKey and methods to serialize a CKey into 32 bytes without any context about where the key came from or what the key represents seems like a bad idea for CKey.
It’s more similar to how we handle Tor v3 and I2P private keys.
I did a quick grep for CKey and <key.h> and didn’t see anything related to Tor or I2P. Can you provide an example of what you mean here, because I’m not sure I fully understand your point.
Sjors
commented at 11:46 am on January 29, 2024:
member
It’s more similar to how we handle Tor v3 and I2P private keys.
I did a quick grep for CKey and <key.h> and didn’t see anything related to Tor or I2P. Can you provide an example of what you mean here, because I’m not sure I fully understand your point.
See #29229: they don’t use CKey, but instead are stored as plain text. They’re similar in the sense that they’re used as an identity key on the network and that we store them in the data dir. But we can’t use CKey for them because afaik they’re on a different curve.
without any context about where the key came from or what the key represents seems like a bad idea for CKey.
I could make a subclass CNotTerriblyImportKey if that’s a huge concern.
josibake
commented at 11:59 am on January 29, 2024:
member
they don’t use CKey, but instead are stored as plain text. They’re similar in the sense that they’re used as an identity key on the network and that we store them in the data dir.
Why can’t we do the same for StratumV2 identity?
Sjors
commented at 12:12 pm on January 29, 2024:
member
Why a convert a key to a plain text hex string if you can just store the bytes? CKey also takes care to not leave key material dangling in memory in temporary variables.
josibake
commented at 12:21 pm on January 29, 2024:
member
My question was why not handle StratumV2 keys the same way we are handling Tor and I2P identity keys and not use CKey at all. This avoids needing to add generic Serialize/Deserialize methods to CKey just for this specific use case, and also avoids needing to go write a specific SV2 key serialization format and corresponding DecodeSV2Secret method for CKey.
Sjors
commented at 2:08 pm on January 29, 2024:
member
The Tor and I2P keys are not generated by us. We get them via a JSON RPC and pass them back that way. We don’t use them ourselves for decryption and encryption. The passing around as plain text is suboptimal, because we’re not clearing the key data from memory.
With Stratum v2 we do use the key. We need its pubkey, perform (Elligator-Swift) ECDH with it, etc. That’s what a CKey is for.
josibake
commented at 2:55 pm on January 29, 2024:
member
With Stratum v2 we do use the key. We need its pubkey, perform (Elligator-Swift) ECDH with it, etc. That’s what a CKey is for.
Your 1st objection to using CPrivKey was that CPrivKey is always meant to be used in conjunction with a public key (implying that this SV2 key is not), but it sounds like we will also always be using a public key with the SV2 key, so I don’t understand your 1st objection to using CPrivKey or why we would need a “dummy public key.”
Your second point makes sense to me in that if this is meant to be an XOnlyPubKey, then passing it around in DER encoding is incorrect / confusing. But by the same argument, what you are doing now is also incorrect and confusing in that you are serializing and deserializing 32 bytes with no indication it is meant for an XOnlyPublicKey.
Again, I really think we should avoid adding generic Serialize/Deserialize methods on CKey and so far your objections to using existing methods doesn’t seem convincing enough to warrant adding two new methods to CKey.
If your usecase is “read 32 bytes and create a CKey,” why not just read the 32 bytes and use the CKey.Set method? Your Unserialize code seems to duplicate what Set is already doing. For de-serializing, why not just get the byte data and write it out, and then destroy the key?
Sjors
commented at 3:41 pm on January 29, 2024:
member
it sounds like we will also always be using a public key with the SV2 key,
Yes, but we just derive the public key from the private key. There’s no need to store it and check it. If it got corrupted, you just make a new one.
what you are doing now is also incorrect and confusing in that you are serializing and deserializing 32 bytes with no indication it is meant for an XOnlyPublicKey
We don’t make that distinction for Taproot wallet keys either. Any compressed key can be added to a Taproot descriptor or to any ECDSA based descriptor. We could prevent that by adding bool xonly to CKey, but that would (probably) require a wallet refactor.
Another approach would be XOnlyKey subclass, similar to what I suggested above. The wallet could later make use of that too.
why not just get the byte data and write it out, and then destroy the key?
I could, but then the next person implementing something similar forgets to destroy the key (or any of the other checks). It’s safer to have the serialisation code take care of this.
josibake
commented at 6:15 pm on January 29, 2024:
member
Yes, but we just derive the public key from the private key. There’s no need to store it and check it. If it got corrupted, you just make a new one.
If bitcoin core is not generating this key, then it seems reasonable to have the pubkey along with it so that it can be verified. If bitcoin core is generating this key, I don’t see why we are writing it to disk unencrypted. Seems like a bad idea. Also it seems like these keys can be ephemeral if we can just make a new one, so writing it to disk makes even less sense to me. There’s probably some reason why its done this way in the StratumV2 spec and that’s fine, but that doesn’t seem like a compelling enough reason to add these general methods to CKey when you can write your StratumV2 code without needing these methods on CKey.
We don’t make that distinction for Taproot wallet keys either. Any compressed key can be added to a Taproot descriptor or to any ECDSA based descriptor. We could prevent that by adding bool xonly to CKey, but that would (probably) require a wallet refactor.
The descriptor is the encoding information: you import a private key wrapped in a descriptor which tells you exactly what the key is meant for. It’s expected you would back up that single key with the descriptor info.
I could, but then the next person implementing something similar forgets to destroy the key (or any of the other checks). It’s safer to have the serialisation code take care of this.
What you are doing here seems very usecase specific (hence why I think you should do it without adding new methods to CKey), and I certainly hope we don’t have a lot of future use cases where we need to write out unencrypted private keys to disk.
FWIW, BIP324Cipher is doing it in the way I suggested, i.e. use the key and then set mykey = CKey()
Sjors
commented at 10:16 am on January 30, 2024:
member
If bitcoin core is not generating this key, then it seems reasonable to have the pubkey along with it so that it can be verified. If bitcoin core is generating this key, I don’t see why we are writing it to disk unencrypted.
We generate the key for Stratum v2. We don’t generate the key for Tor and I2P. We also don’t encrypt those.
I don’t see how encryption could work for a server application like a Template Provider. Putting the password in a config file adds complexity and no security unless the system admin really knows what they’re doing. An even more complicated approach would have the user unlock the server key via an RPC call. This is why a certificate can be signed on a different machine, and users can simply rotate the server (static) key when it’s compromised. We’re trying to make it easy to run a Template Provider, while providing a way for advanced users to increase security (the certificate).
Also it seems like these keys can be ephemeral if we can just make a new one, so writing it to disk makes even less sense to me.
It would help if you read the Stratum v2 spec on this. I also wrote some context in #29346 (opened yesterday). There’s three keys: ephemeral to start the handshake (like in BIP324), a static key and an authority key. That last key can be on a separate machine it’s the one a pool might publish on their website. It signs the static key. We need to store that static key. It should be rotated if the server is compromised. Otherwise you’d have to make sign and copy a new certificate on every restart.
BIP324Cipher is doing it in the way I suggested
Yes, BIP324 only uses ephemeral keys, there’s no identity by design.
The descriptor is the encoding information: you import a private key wrapped in a descriptor which tells you exactly what the key is meant for. It’s expected you would back up that single key with the descriptor info.
That makes sense for the wallet. But a file name like sv2_static_key is plenty of context.
josibake
commented at 10:50 am on January 30, 2024:
member
It would help if you read the Stratum v2 spec on this
This PR is adding generic Serialize/Deserialize commands to CKey which allows you to read and write 32 bytes without any context. I don’t think this is a good change for Bitcoin Core.
After talking through your specific usecase for SV2, I better understand why you want to read and write a key to disk and how you intend to provide context for the key, but I don’t think it’s a compelling enough reason to motivate a general change to CKey, especially when you can accomplish exactly what you want with the existing methods on CKey.
Sjors force-pushed
on Feb 1, 2024
Sjors
commented at 1:23 pm on February 1, 2024:
member
I looked into the possibility of subclassing CKey or making a similar class from scratch, but that got too complicated too quickly.
So I’m keeping the approach as-is, and will work around it in some more verbose way if this PR doesn’t make it.
I did however bring back the compressed boolean. That way the serialisation maps 1-to-1 to CKey.
I think that if the wallet was designed from scratch it would use 32 byte encoding for private keys. The DER encoding adds no value, we don’t even use it for export and import and the conversion is expensive (according to the GetPrivKey()’s code comment). It’s not worth overhauling the wallet, but it seems reasonable to discourage DER encoding use for any other application.
vasild approved
vasild
commented at 1:01 pm on February 4, 2024:
contributor
ACKaf25cfcbabe96283625f3a32f493940658a5d1e5
I think the newly added generic ser/unser methods to CKey are harmless. “…especially when you can accomplish exactly what you want with the existing methods on CKey” that is doing the same thing, conceptually, but shifting the complexity to the caller in a way that is less efficient (more copying) and more prone to errors (the caller forgets to cleanse the intermediate buffer).
DrahtBot removed review request from shaavan
on Feb 4, 2024
DrahtBot requested review from shaavan
on Feb 4, 2024
josibake
commented at 1:52 pm on February 5, 2024:
member
I think the newly added generic ser/unser methods to CKey are harmless.
Its adding a footgun to CKey. The class is making sure to keep the secret data secure in memory and to free it when the key is gone, so allowing the key to be created directly from (potentially insecure) data on disk and allowing the key to be written out unencrypted to disk without destroying the key seems like an anti-pattern. You could argue that the existing methods (e.g. GetPrivKey) are also footguns, but that is not a good argument for adding more footguns to the class. It also seems strange to me that all of this care is being taken to keep the secret data secure in memory when you are reading writing it unencrypted to and from disk.
Even if harmless, this still seems unnecessary since your usecase can be accomplished without it. If your requirements for SV2 were to change and not need core to be generating this secret, or not need the secret written to disk, then we would want to remove these unused methods from CKey, hence better to continue without adding usecase specific methods to CKey.
in a way that is less efficient (more copying) and more prone to errors
This seems specific to the current implementation? I imagine there is a way to do it efficiently with the class as is. Maybe once the StratumV2 spec is finalized and the implementation finished, you can reopen this if it still makes sense as a refactor?
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
Sjors
commented at 2:43 pm on February 5, 2024:
member
It also seems strange to me that all of this care is being taken to keep the secret data secure in memory when you are reading writing it unencrypted to and from disk.
That’s literally what the wallet does by default. CKey provides extra security for free, so it’s better to use it. The optional wallet encryption feature makes use of this security feature, by ensuring that once a wallet is (re)locked the key data is erased from memory.
It would take additional code for Sv2 to handle the key material insecurely. Namely merging #29229 and convering the CKey to and from Span<std::byte> before saving it.
You could argue that the existing methods (e.g. GetPrivKey) are also footguns, but that is not a good argument for adding more footguns to the class.
They’re not footguns. They’re merely needlessly complicated ways to serialize / unserialize key data.
There’s no way for CKey to protect callers other than by cleaning up after itself. You also can’t accidentally store a key on disk by somehow inadvertently calling the serialise function, it requires opening a file.
If your requirements for SV2 were to change and not need core to be generating this secret
That’s an argument for holding off merging this PR until #28983 / #29346 have more concept ACK’s. I’m not expecting the Sv2 protocol to be overhauled so that it’s no longer necessary to generate and persist keys. It’s conceivable that we don’t implement support for it, specifically we could decide to only support plain text connections and require users to install a proxy (not my preference though).
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
josibake
commented at 3:49 pm on February 5, 2024:
member
It also seems strange to me that all of this care is being taken to keep the secret data secure in memory when you are reading writing it unencrypted to and from disk.
That’s literally what the wallet does by default. CKey provides extra security for free, so it’s better to use it. The optional wallet encryption feature makes use of this security feature, by ensuring that once a wallet is (re)locked the key data is erased from memory.
You could argue that the existing methods (e.g. GetPrivKey) are also footguns, but that is not a good argument for adding more footguns to the class.
They’re not footguns. They’re merely needlessly complicated ways to serialize / unserialize key data.
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.
If your requirements for SV2 were to change and not need core to be generating this secret
That’s an argument for holding off merging this PR until #28983 / #29346 have more concept ACK’s. I’m not expecting the Sv2 protocol to be overhauled so that it’s no longer necessary to generate and persist keys. It’s conceivable that we don’t implement support for it, specifically we could decide to only support plain text connections and require users to install a proxy (not my preference though).
So lets hold off? You’re not blocked on this PR being merged, and it seems we wont need this change if the other PRs don’t progress. I imagine there are also more possibilities, such as doing this optional identity management outside of Bitcoin Core and using the keys similar to how we do I2P/Tor, where the secret is read as Bytes.
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
Sjors
commented at 4:21 pm on February 5, 2024:
member
In your case, loss of a passphrase would simply mean regenerating a new key.
There is no passphrase for sv2 keys. It would make running a template provider much too complicated. It could be added as an advanced feature later of course. (I would just enable disk encryption at rest for a server if this was a concern)
’d be more inclined if this PR was proposing a general refactor
I just need to store a key a on disk, I’m not interested in a general refactor.
You’re not blocked on this PR being merged, and it seems we wont need this change if the other PRs don’t progress.
Correct. As I said above, I’m not in a rush to get this merged, but I’m also not going to make it (significantly) more complicated.
doing this optional identity management outside of Bitcoin Core and using the keys similar to how we do I2P/Tor
This implies running the template provider outside of Bitcoin Core. As long as RPC and ZMQ are the only way for external tools to interact with Bitcoin Core, that approach adds too much overhead and delay. We’re trying to push new block templates out as quickly as possible, preferable as soon as a new transaction enters the mempool that increases the total block template fees by enough to justify the bandwidth. See earlier discussion about this: #27854 (comment)
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
DrahtBot removed review request from shaavan
on Feb 5, 2024
DrahtBot requested review from shaavan
on Feb 5, 2024
vasild
commented at 7:38 am on February 8, 2024:
contributor
@josibake, I see your point even though I see things a bit differently. I guess the important takeaway here is that this PR is not a blocker for other stuff but a convenience of doing ser/deser in the class instead of in the caller. Thanks!
DrahtBot removed review request from shaavan
on Feb 8, 2024
DrahtBot requested review from shaavan
on Feb 8, 2024
DrahtBot removed review request from shaavan
on Feb 8, 2024
DrahtBot requested review from shaavan
on Feb 8, 2024
ryanofsky
commented at 8:08 pm on February 26, 2024:
contributor
If I can summarize the disagreement here, it seems like there are currently three ways to serialize CKey data:
Call CKey::GetPrivKey() to save in in an openssl format and use CKey::Load() to load it.
Call EncodeSecret() and DecodeSecret() and to store and load it as base58 strings.
Call CKey::IsCompressed() to get the fCompressed bit and call CKey::data() or treat it as a span to get the raw bytes. Then call CKey::Set() to load the compression bit and key bytes.
This PR is adding a adding a fourth way:
Adding CKey::Serialize() and CKey::Unserialize() methods to serialize the compressed bit and key data into a stream object, and also calling CKey::Check() method during deserialization. The original implementation of this did not include the compressed bit. But then it was changed to add it, before dropping it again, and then adding it again.
Josibake is arguing that new serialization method is redundant, and might make it easier to make mistakes like leaking keys by not zeroing after freeing, storing keys without encryption when encryption should be used, storing the raw bytes without format information when key format should be recorded, and forgetting to check assumptions such as whether uncompressed keys are allowed.
On the other hand though, a lot of these problems already exist currently and the new method is not markedly worse than existing ones, and in some cases it may be better. For example if serializing with DataStream, a secure allocator will be used unlike with EncodeSecret and std::string.
Overall I think I agree with Josie that adding this new CKey serialization method seems to make a confusing situation a little worse and doesn’t seem to offer major benefits over CKey::Set. But I don’t think the amount of harm would be great.
I’d also note that this change would allow dropping CKey serialization code from #10102:
But I’m not sure if this is a good thing. It seems reasonable to me that there maybe should be a little extra friction around serializing private keys, so they don’t get serialized automatically without thinking about the best way to do it, and whether it is necessary.
DrahtBot removed review request from shaavan
on Feb 26, 2024
DrahtBot requested review from shaavan
on Feb 26, 2024
DrahtBot removed review request from shaavan
on Feb 26, 2024
DrahtBot requested review from shaavan
on Feb 26, 2024
DrahtBot removed review request from shaavan
on Feb 26, 2024
DrahtBot requested review from shaavan
on Feb 26, 2024
fanquake
commented at 2:28 pm on March 6, 2024:
member
Overall I think I agree with Josie that adding this new CKey serialization method seems to make a confusing situation a little worse and doesn’t seem to offer major benefits over CKey::Set.
I agree. Adding more code, with the argument that “it’s only as bad as all the existing code” does not seem good, makes an already less than ideal situation worse, and it’s already being argued against.
DrahtBot removed review request from shaavan
on Mar 6, 2024
DrahtBot requested review from shaavan
on Mar 6, 2024
Sjors
commented at 9:24 am on March 7, 2024:
member
and might make it easier to make mistakes like leaking keys by not zeroing after freeing
It’s the exact opposite. The existing methods makes this mistake easier. In fact, I actually made that mistake, as cited in the PR description, which is why opened this in the first place.
I could try again to implement it safely using method 3, just to see if I don’t screw it up again. And then we can hope that others copy-paste my example in stead of repeating my original mistake. This seems an overkill way to avoid adding a standard serialization method.
DrahtBot removed review request from shaavan
on Mar 7, 2024
DrahtBot requested review from shaavan
on Mar 7, 2024
DrahtBot removed review request from shaavan
on Mar 7, 2024
DrahtBot requested review from shaavan
on Mar 7, 2024
josibake
commented at 9:43 am on March 7, 2024:
member
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.
DrahtBot removed review request from shaavan
on Mar 7, 2024
DrahtBot requested review from shaavan
on Mar 7, 2024
Sjors
commented at 9:54 am on March 7, 2024:
member
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).
DrahtBot removed review request from shaavan
on Mar 7, 2024
DrahtBot requested review from shaavan
on Mar 7, 2024
josibake
commented at 10:10 am on March 7, 2024:
member
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.
DrahtBot removed review request from shaavan
on Mar 7, 2024
DrahtBot requested review from shaavan
on Mar 7, 2024
Sjors
commented at 10:20 am on March 7, 2024:
member
I think a better approach would be to use the existing methods for your use case
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
