refactor: Allow std::span construction from CKey

maflcko commented at 3:46 pm on December 22, 2023: member

Is is possible to construct a Span from a reference to a CKey. However, the same is not possible with std::span.

Fix that.

DrahtBot commented at 3:46 pm on December 22, 2023: contributor

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
ACK	shaavan, willcl-ark

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:

#28774 (Avoid returning references to mutex guarded members by vasild)
#28710 (Remove the legacy wallet and BDB dependency 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.

DrahtBot renamed this:
~~refactor: Allow std::span construction from CKey~~
refactor: Allow std::span construction from CKey
on Dec 22, 2023

DrahtBot added the label Refactoring on Dec 22, 2023

maflcko force-pushed on Dec 22, 2023

maflcko added the label DrahtBot Guix build requested on Dec 25, 2023

DrahtBot commented at 6:05 am on December 26, 2023: contributor

Guix builds (on x86_64)

File	commit 4b1196a9855dcd188a24f393aa2fa21e2d61f061(master)	commit b6d4aaa09e8976767c8e73f8d1362c39e5538b8f(master and this pull)
SHA256SUMS.part	`7aefd803a584a152...`	`ab079c58401ba0fb...`
*-aarch64-linux-gnu-debug.tar.gz	`735f38fb37b239f8...`	`4dbdbe5e93c73851...`
*-aarch64-linux-gnu.tar.gz	`83a257c74e184b96...`	`988aaff726a6daa2...`
*-arm-linux-gnueabihf-debug.tar.gz	`513acf435b18fab2...`	`b8b4ecdca2193722...`
*-arm-linux-gnueabihf.tar.gz	`581f911fcca5ae02...`	`76777352461e8bc6...`
*-arm64-apple-darwin-unsigned.tar.gz	`7a3f2b4e01f27896...`	`19d1adb684d05225...`
*-arm64-apple-darwin-unsigned.zip	`dff45b014236e990...`	`47db0212b16e9ed6...`
*-arm64-apple-darwin.tar.gz	`0d9696b7bd76b36f...`	`c43711dc9f00aa77...`
*-powerpc64-linux-gnu-debug.tar.gz	`dfe6cfdcd6ad3c6f...`	`419d862f3e56bbf8...`
*-powerpc64-linux-gnu.tar.gz	`54c013201daad1ff...`	`bbfd8c4b3727a09e...`
*-powerpc64le-linux-gnu-debug.tar.gz	`8e944b002273b678...`	`ce4db42977bd22a2...`
*-powerpc64le-linux-gnu.tar.gz	`1ba8199984d7c43a...`	`d8293f8d7c529a5c...`
*-riscv64-linux-gnu-debug.tar.gz	`e9abe48aeb7acc68...`	`f1f2c5ba6f83255d...`
*-riscv64-linux-gnu.tar.gz	`f4d1484dec65b967...`	`62afb94bd8008b15...`
*-x86_64-apple-darwin-unsigned.tar.gz	`915650aa35e6e322...`	`f0fcc5c9b634cdf5...`
*-x86_64-apple-darwin-unsigned.zip	`6a6139cb415de743...`	`0629e63a88039b69...`
*-x86_64-apple-darwin.tar.gz	`c479d099d1df9a85...`	`097d74e47b61b648...`
*-x86_64-linux-gnu-debug.tar.gz	`9e27161179f18a4a...`	`1c4818ca22c3cc3e...`
*-x86_64-linux-gnu.tar.gz	`f69632ca3dc98896...`	`937b9ff173077441...`
*.tar.gz	`c04997c9f9e51d80...`	`838da9ae9aa9e619...`
guix_build.log	`5528a9baa3a070f3...`	`86f09be4ba8fad36...`
guix_build.log.diff		`cf884818846014a3...`

DrahtBot removed the label DrahtBot Guix build requested on Dec 26, 2023

shaavan approved

shaavan commented at 1:27 pm on January 2, 2024: contributor

Code Review ACK

Commit-1: Since std::span requires a mutable pointer to the data for certain operations, this change makes XOnlyPubKey compatible with std::span.

Commit-2: Since the return type of begin() and end() much match the data type of the std::span of the container, this change makes CKey compatible with std::span, and allows std::span<std::byte> construction for it.

Commit-3: Tests that the above changes made CKey compatible with std::span.

I am curious about one thing, though.

If we now have std::span since C++20, are we thinking of making every container in the codebase std::span compatible and moving away from needing to use our ### custom Span class?

maflcko commented at 1:31 pm on January 2, 2024: member

If we now have std::span since C++20, are we thinking of making every container in the codebase std::span compatible and moving away from needing to use our ### custom Span class?

Yes, I think it makes sense to allow new code to use std::span directly. Also, I think it makes sense to (longer term) remove Span (or make it an alias of std::span) to avoid confusion which to use in new code, and to get the additional compile-time checks from std::span.

DrahtBot added the label CI failed on Jan 15, 2024

Add missing XOnlyPubKey::data() to get mutable data

This is needed for consistency, and also to allow std::span construction
from XOnlyPubKey.

999962d68d

maflcko force-pushed on Jan 16, 2024

refactor: Allow std::span construction from CKey fa96d93711

maflcko force-pushed on Jan 16, 2024

maflcko commented at 2:29 pm on January 16, 2024: member

I dropped the last commit, to not introduce new std::span usage for now. To verify the changes in this pull, the commit’s diff can be used. (Fails to compile before, compiles after):

 0diff --git a/src/key.cpp b/src/key.cpp
 1index 2bd6396298..300c95e35a 100644
 2--- a/src/key.cpp
 3+++ b/src/key.cpp
 4@@ -385,7 +385,7 @@ bool CExtKey::Derive(CExtKey &out, unsigned int _nChild) const {
 5     return key.Derive(out.key, out.chaincode, _nChild, chaincode);
 6 }
 7 
 8-void CExtKey::SetSeed(Span<const std::byte> seed)
 9+void CExtKey::SetSeed(std::span<const std::byte> seed)
10 {
11     static const unsigned char hashkey[] = {'B','i','t','c','o','i','n',' ','s','e','e','d'};
12     std::vector<unsigned char, secure_allocator<unsigned char>> vout(64);
13diff --git a/src/key.h b/src/key.h
14index d6b26f891d..d63ec6c1cc 100644
15--- a/src/key.h
16+++ b/src/key.h
17@@ -12,10 +12,10 @@
18 #include <support/allocators/secure.h>
19 #include <uint256.h>
20 
21+#include <span>
22 #include <stdexcept>
23 #include <vector>
24 
25-
26 /**
27  * CPrivKey is a serialized private key, with all parameters included
28  * (SIZE bytes)
29@@ -227,7 +227,7 @@ struct CExtKey {
30     void Decode(const unsigned char code[BIP32_EXTKEY_SIZE]);
31     [[nodiscard]] bool Derive(CExtKey& out, unsigned int nChild) const;
32     CExtPubKey Neuter() const;
33-    void SetSeed(Span<const std::byte> seed);
34+    void SetSeed(std::span<const std::byte> seed);
35 };
36 
37 /** Initialize the elliptic curve support. May not be called twice without calling ECC_Stop first. */

DrahtBot removed the label CI failed on Jan 16, 2024

shaavan approved

shaavan commented at 7:49 am on January 17, 2024: contributor

ReACK fa96d937116682f32613d31a3ae7d6f652e8146d

Changes since my last review

Removed the last commit, which was temporarily introduced to test the workings of changes in this commit.
Tested that the above patch leads to failure in master, while successful compilation with this PR.

fanquake requested review from willcl-ark on Jan 17, 2024

willcl-ark approved

willcl-ark commented at 3:05 pm on January 17, 2024: contributor

ACK fa96d937116682f32613d31a3ae7d6f652e8146d

A nice simple change to enable using std::span with CKey.

The PR description could be updated now that the verification commit has been removed.

fanquake merged this on Jan 17, 2024

fanquake closed this on Jan 17, 2024

maflcko deleted the branch on Jan 18, 2024

refactor: Allow std::span construction from CKey #29133

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)