[New Feature] BIP 352 Silent Payments Address Support

[notTanveer]

Description

Implemented support for BIP-352 Silent Payment address generation (including label support), and also refactored the keyboard input screens.
This PR builds on top of Keith’s awesome work here: kdmukai/seedsigner#bip352_silent_payments_basic_support

Key changes:

• Added Silent Payment address generation
• Added label support (numeric-only) to help identify senders

Refactor

• Refactored the xpub details screen — now extendable to show scan and spend key details as well
• Removed BIP-85 numeric screen by directly instantiating the keyboard. (while creating, a integer-only label screen)

I included the refactor in this PR since the changes were minimal and related. For easier review, I recommend starting with the first commit, which focuses solely on SP address generation.

Screenshots

This pull request is categorized as a:

• New feature
• Bug fix
• Code refactor
• Documentation
• Other

Checklist

• I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

• No, I’m a fool
• Yes
• N/A

I have tested this PR on the following platforms/os:

• Raspberry Pi OS Manual Build
• SeedSigner OS on a Pi0/Pi0W board
• Other

[alvroble]

Thanks a lot @notTanveer for your work!! I haven't dived yet in the code but tested this PR on Raspberry Pi OS Manual Build:

• pytest passes fine
• It does not work when using a label. I got the following error when interacting with the keyboard:
  UnhandledExceptionView({'error': ['UnboundLocalError', 'seed_screens.py, 918, in _run', " local variable 'keyboard_swap' referenced before assignment"]})
• It works fine without a label.

Also, for "Export spend pubkey" and "Export scan privkey", I think it would improve the UX to first show the key before proceeding with the QR, similar to how the SP address is displayed first. Additionally, maybe it makes sense to show a WarningScreen before displaying the keys, depending on what others think.

[notTanveer]

• It does not work when using a label. I got the following error when interacting with the keyboard:
  UnhandledExceptionView({'error': ['UnboundLocalError', 'seed_screens.py, 918, in _run', " local variable 'keyboard_swap' referenced before assignment"]})

Ah, I’d already fixed that error but forgot to push, should be good now!
just needed to assign keyboard_swap at the start of each loop to avoid the UnboundLocalError.
Appreciate you pointing it out!

[alvroble]

Tested on Raspberry Pi OS manual build. Really neat UI. Functionally LGTM.

I'd suggest to add screenshots for all the new screens, including the warning screen before showing the scan key and both spend and public key screenshots.

[kdmukai]

I only briefly scanned through the BaseKeyboardEntryScreen refactor but it triggered a few thoughts:

As far as I know, BIP-352 says basically nothing about what can or cannot be in a label. It lists incrementing integers as the most foolproof, trivially recoverable approach (i.e. every SP wallet can just automatically scan out to label 100000 to see if any of those labels had received a payment; apparently that's not as resource intensive as it sounds(?)). But Josie said that people can use labels however they like (any arbitrary text string), but then future recoverability is completely up to them (e.g. if I use a complex passphrase as a label, I better have that recorded somewhere or I'll never be able to find that label -- nor their payments --- ever again).

So it seems overly restrictive to assume that only upper/lower/digit chars are valid. Josie's first example of a possible non-incrementing integer use case was a user account number. But what if the account numbers are specified like: f347-99337-b83x1? Or even street address + unit number + last name to track tenants: 123 Main, #34: Simmons.

IF we allow arbitrary text labels, it feels like we should allow common symbols as well.

Which would mean the keyboard would be the same as the existing bip39 passphrase keyboard. Which would mean there's no need to refactor (other than to rename that keyboard class to be more general).

THAT ALL BEING SAID... my sense so far is that the best practice is to assume the incrementing integer approach. There are so few wallet implementations that there's not enough data points to even suggest what an emerging consensus might be. But the incrementing integer approach was explained to me as the "don't shoot yourself in the foot" approach (because it's trivially easy for a wallet to recover those kinds of labels).

This all leads me to think that maybe this is a better approach:

• DISABLE label support entirely unless explicitly enabled; it's a niche add-on to an already niche feature. The vast majority of early adopters will just use the base SP address without wading into the more foot-gun-y waters of labels. Better to hide it away from the UX entirely unless the user specifically goes looking to enable it in Settings. Maybe to avoid having two separate settings to configure, one setting could have three options:
  • Enable SP (the option we hope that most people select)
  • Enable SP + labels (the more advanced option)
  • Disable SP (default)
• And limit our initial label implementation to JUST the integer use case.
• Leave a TODO note that we should revisit broadening the label support to any arbitrary char string at a later point.

And then if some of the keyboard cleanup offers real improvements (DRY, readability, flexibility) without going too far into unnecessary premature optimization, then consider pulling those changes out into their own isolated refactor PR. As it is, those changes make this PR much harder to review and a bigger "ask" to get merged.

[notTanveer]

I agree completely, if users don’t store or record their labels, there’s a high risk they’ll forget them over time. In fact, I initially wanted to add a warning screen when labels are chosen. I started out considering only numeric labels, but ultimately decided to allow any choice. To steer users toward numeric labels, though, I set the keyboard default to digits, and that required refactoring the keyboard screen to accommodate only numbers and letters.

For symbols, I followed the Bitcoin design convention, which only includes alphanumeric characters, so that’s the approach I took. The BIP itself doesn’t explicitly limit label formats, but it does emphasize integers: “Let Bm = Bspend + hash(bscan || m)·G where m is an incrementable integer starting from 1.” It made sense to stick with integers, which again led to needing a keyboard refactor to avoid redundant code.

All that said, I think I’ll move forward with the approach you described. As a first step, I’ll keep the initial pull request small—just basic Silent Payment address generation.
Should I add label support in a subsequent PR?

[kdmukai]

I still think integer-only labels have a place here (though, again, users should be encouraged to leave labels disabled).

But I'd like to hear what others think.

[notTanveer]

updated the labels to support only integers for now. also did a small refactor on the details and labels screens to clean up some redundancy.

now that i think about it — should we consider setting a limit on the label value? like 100K or maybe u32?

EDIT: now that labels have a separate enable button, should i get rid of the 'Add Label' screen?

[kdmukai]

Type hint on label would be nice.

    ..., label: int | str | bytes, ...

The convention seems to be to have spaces between each "|" which I hate, but, alas, a lot of these conventions are annoying (e.g. I would prefer to have no whitespace around = when setting default vals for input args).

[kdmukai]

That being said, I'm not sure having a separate function for labels is necessary. It's probably cleaner to just have an optional label arg in encode_silent_payment_address that defaults to None.

[notTanveer]

..., label: int | str | bytes, ...

since KeyboardScreen(used in LabelEntryView) always gives back a string, wondering if this should just be str across the board?

[kdmukai]

TRANSLATOR_NOTE - this one could be misinterpreted as a command ("Scan your private key now" instead of talking about the "scan private thing" as a noun). Have to make sure the translators aren't confused about the intention here.

[kdmukai]

I would phrase this as something like "arbitrary (non-integer) labels"

[kdmukai]

This PR definitely needs to add a flow test to be considered complete!

I'd suggest:

• Flow with labels disabled.
• Flow with a custom label or two.

[kdmukai]

Super minor nit: "with labels no label" is confusing. Might be clearer in all of these test names to use "labels_enabled" / "labels_disabled" and then maybe something like "skip_label" / "specify_label"? Or "with_label" / "without_label"? You get the idea...

Also elsewhere we use double underscore to help provide extra clarity / separation in names when needed. I don't know if any flow tests use it, but I find it useful. e.g. labels_enabled__skip_label...

[kdmukai]

I probably would have just directly connected these tests into one sequence.

The strict separation here is slightly more rigorous, of course, just a tiny bit unnecessary.

[kdmukai]

I would leverage the pytest harness to do this in something like setup_class (I can't recall the exact method name) where it's done once for the entire class before any of its child tests are run and is then present for all tests.

There's another method that's run before each individual test. I can't recall which scope would be required for this sort of pre-test config.

[kdmukai]

Can you regenerate the screenshots and update them in the PR description? I know at least the "DANGER" screen has changed.

[kdmukai]

What if we just added an option here for "SP address + label" (or something like that) in order to avoid the label Yes/No screen entirely?

[kdmukai]

Also worth thinking about if "Generate SP address" could offer a tiny bit more guidance: "Shareable SP address"?

Or just: "Silent payment address"? Or "Export SP address" to match our xpub approach?

Basically, "Generate" is taking up a lot space but isn't really useful.

[notTanveer]

I believe i've addressed all the feedback.

Changes:

• Removed **LabelPromptScreen** and added dedicated SP labeled address button
• Added Flow Tests with pytest setup_method to instantiate test seed and reduce redundancies
• Enforced consistent integer-only SP address label generation
• Updated translator notes
• Renamed test flow methods following better naming conventions

Questions:

• Given our focus on UX and foolproof labels, should we add integer limits for now? (BIP doesn't specify any constraints)
• currently type-casting string input from label screen to integers since KeyboardScreen only accepts strings, but our labels support int/str/bytes types. Should we standardize on integers / strings throughout instead of this conversion? (i don't want to modify KeyboardScreen )
• should I update the messages.pot file?

[newtonick]

cACK 1d984f8

I haven't had the time to dive into this code, but did run the PR and did some functionality testing. Looks really good. Looking forward to getting SP functionality merged into SeedSigner. Thanks for this work!

[macgyver13]

Linking sp() output descriptor bip proposal for visibility

Great to see the progress made on Label UI/UX