Add warnings for non-active addresses in receive tab and address book #723
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
pinheadmz
force-pushed
the
used-addr-ui-gui
branch
5 times, most recently
from
2750aff
to
e4e7466
Compare
pinheadmz
changed the title
There was a problem hiding this comment.
Quick GitHub review, will swing back and build/test.
A couple thoughts:
GetWarningsis already the name of a method insrc/warnings; perhaps use a slightly different name.- Watch out for missing brackets in conditionals as mentioned in the developer notes; we require brackets with conditionals over one line due to CVE-2014-1266 (the Apple "goto fail" vulnerability, see also https://dwheeler.com/essays/apple-goto-fail).
- Consider notating pure/getter-like or need-to-check-returned-value methods with
[[nodiscard]]. As written recently here in the repo by martinus, a [[nodiscard]] attribute is most useful in two cases:- if a function has no effects beyond returning a certain result, i.e. is pure. If the result is not used, the call is certainly useless.
- if the return value must be checked, e.g. for a C-like interface that returns error codes instead of throwing.
pinheadmz
force-pushed
the
used-addr-ui-gui
branch
2 times, most recently
from
b53d9be
to
8823c42
Compare
|
Thanks for all the style notes @jonatack I addressed all your comments including adding a new struct for |
|
I also moved the warnings column to the right, after date. I think we could just remove the title "warnings" and that column would narrow down to 1 character ...?
|
pinheadmz
force-pushed
the
used-addr-ui-gui
branch
from
8823c42
to
9b5d7aa
Compare
pinheadmz
force-pushed
the
used-addr-ui-gui
branch
from
9b5d7aa
to
58fba35
Compare
Thanks D-bot! rebased. |
pinheadmz
force-pushed
the
used-addr-ui-gui
branch
from
58fba35
to
568fe6c
Compare
This new method returns true if the given CScript key is derived from the SPKM. For Legacy that means checking the hd_seed_id in the key's metadata. Also patches PKDescriptor to return associated public keys in MakeScripts()
This connects SPKM.IsKeyActive() up to the wallet level.
|
Concept ACK |
Tool tip and dialog field can be expanded in the future. For now the first warning we show is if the address was derived from a non-active seed or descriptor. See bitcoin/bitcoin#3314
Tool tip can be expanded in the future. For now the first warning we show is if the address was derived from a non-active seed or descriptor. See bitcoin/bitcoin#3314
pinheadmz
force-pushed
the
used-addr-ui-gui
branch
from
568fe6c
to
d6519a8
Compare
Thanks Randy! I just rebased on master to fix merge conflict. If you'd like to see this move forward, you can help my reviewing the parent Bitcoin Core PR: bitcoin/bitcoin#27216 🙏 ❤️ |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Concept NACK, there's no reason such an address shouldn't be used.
Except those categories are entirely unrelated to whether it's "active" or not. |
While the base PR is still under reviewing, maybe convert this one to a draft? |
There was a problem hiding this comment.
Concept ACK
I'd prefer your suggestion of removing the title of the column or even removing the column completely and put the warning icon next to the date-time in the Date column as proposed by @jonasschnelli.
|
closing for now, #27216 needs better approach |
Closes bitcoin/bitcoin#3314
Built off of bitcoin/bitcoin#27216 (== first two commits of this PR, required core updates)
This PR adds a "warnings" column in the receive coins dialog (recent requests table) and the address book receive page. Hovering over the⚠️ if present shows a tooltip with a list of warnings. We also add a "warnings" field in the receive request dialog (shown after double-clicking an address in the first table) with the same message.
The warnings shown can be anything implemented in new methods
GetWarnings()but for now the only message we show there is:Example screenshots:
1. Create new unencrypted wallet, get new receive address
2. Encrypt wallet
Notice how the first address is immediately flagged.
3. Get new receive address after encrypting wallet
New address is not flagged, it is active and safe to use.