wallet: add codex32 argument to addhdkey#32652
wallet: add codex32 argument to addhdkey#32652roconnor-blockstream wants to merge 10 commits intobitcoin:masterfrom
Conversation
unused() descriptors do not have scriptPubKeys. Instead, the wallet uses them to store keys without having any scripts to watch for.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32652. ReviewsSee 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. ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Concept ACK-ish This is certainly better than the previous approach. |
roconnor-blockstream
force-pushed
the
codex32
branch
from
46b0e70 to
3335c14
Compare
In the next commit we will implement a new checksum, codex32, which uses the same encoding and HRP rules as bech32 and bech32m, but has a substantially different checksum verification procedure. To minimize duplicated code, we expose the character conversion in a new bech32::internals module.
roconnor-blockstream
force-pushed
the
codex32
branch
from
3335c14 to
b0d9b94
Compare
roconnor-blockstream
force-pushed
the
codex32
branch
from
b0d9b94 to
59777ef
Compare
|
Can you elaborate? |
|
I suppose NACK is a bit too strong. While codex32 itself is interesting, it is not interesting enough that any contributors to this project is interested in reviewing PRs including it. |
|
I still haven't gotten around to trying the paper booklet instructions. If and when I do that, I'll probably review this. But no idea when that is. |
From BIP-0093:
To encode some codex32 strings to import without the paper booklet there are libraries in Rust, Ruby and Python. |
|
@achow101 if there is renewed interest in testing these PR's will you reopen it? |
@Sjors if I sent you some metal codedx32 (currently awaiting to laser engrave them), would that make it easier for you to test? Don't have to fluff about with cutting it out or piecing it together. |
There needs to be interest to review the PR, not just test it. Further, current contributors must also be interested. AFAICT there is no interest from current contributors in reviewing this PR. |
|
@achow101 I appreciate the clarification. Looking at the history: PR #27351 received Concept ACKs from @josibake, @S3RK, and you yourself gave a “Concept ACK-ish” on this PR (#32652), noting it was “better than the previous approach.” @BenWestgate is also an active contributor who’s engaged with codex32 work. What would it take specifically to move this forward? Is the blocker: If it’s review bandwidth, I’d be happy to help coordinate interest from the codex32 community. There are real users waiting for this. I'm building metal backup products specifically for codex32 and currently the only wallet support is Bails/Bitcoin Knots. |
| /** Accessor for the binary payload data (in base 256, not gf32) */ | ||
| std::vector<unsigned char> GetPayload() const { | ||
| assert(IsValid()); | ||
|
|
||
| std::vector<unsigned char> ret; | ||
| ret.reserve(((m_data.size() - 6) * 5) / 8); | ||
| // Note that `ConvertBits` returns a bool indicating whether or not nonzero bits | ||
| // were discarded. In BIP 93, we discard bits regardless of whether they are 0, | ||
| // so this is not an error and does not need to be checked. | ||
| ConvertBits<5, 8, false>([&](unsigned char c) { ret.push_back(c); }, m_data.begin() + 6, m_data.end()); | ||
| return ret; | ||
| }; |
There was a problem hiding this comment.
We've noticed that accessing the binary payload for non-"s" strings is 1) useless, 2) can't round trip back to a share without passing the padding, 3) isn't enough information to recover the secret unambiguously.
We don't define how to decode shares to bytes in BIP93 so unless we do, perhaps gate this for "s" only.
|
My understanding is that they are currently unable to dedicate time to the project in general. Furthermore, just because something has Concept ACKs does not mean that it has reviewer buy in. If those Concept ACKs don't convert into actual code review after some time, that indicates that while the Concept ACKer was interested, they weren't interested enough to put in the work to actually review the PR.
Which was effectively rescinded by my closure of this PR. I also refer to my previous point - I thought this was interesting, but it is not interesting enough to put review effort into it.
They are still new to the Bitcoin Core project and are not yet a regular contributor.
Primarily this. Actual code review may lead to concerns with the approach, but no one has actually done that yet.
We generally are not interested in a PR that is reviewed solely by people new to Bitcoin Core. Such reviews are not very valuable as there are many intricacies of this codebase which are difficult to understand. New reviewers will have not yet demonstrated competency or knowledge of this codebase, and as such their reviews are given less weight than a regular contributor. |
8 participants
Rebase of #27351 onto #29136.
WIP
This PR introduces the ability to import BIP 93/codex32 master seeds with the
addhdkeycommand. It currently expects seeds to be provided in either as a single seed or as a list of shares which can be assembled via Shamir Secret Sharing.