[Feature] Add BIP-353 proof validation #798
Conversation
There was a problem hiding this comment.
Hi @Bicaru20 and thank you for your work! I left some comments but in general I feel that this PR is going on the right direction. Currently it will not pass the test because it depends on the PR to embit but I tested it using Raspberry Pi OS Manual Build env along Sparrow Wallet and it works!
|
|
||
|
|
||
|
|
||
| class PSBTDnsNameView(View): |
There was a problem hiding this comment.
This name may be too generic. It'd make sense to use something more related to BIP-353. Maybe PSBTHrnView or PSBTDnsPaymentIntructionsView
| class PSBTDnsNameView(View): | ||
| def run(self): | ||
| from seedsigner.gui.screens.screen import LargeIconStatusScreen | ||
| from embit.bip353 import verify_dns_proof |
There was a problem hiding this comment.
This line will raise an error until diybitcoinhardware/embit#102 gets merged
| title = _("DNS name verification"), | ||
| button_data = [ButtonOption("Review math")], | ||
| text = _("Verification complete. Review the DNS name."), | ||
| status_headline = dns_name, |
There was a problem hiding this comment.
This will print out pay.user._bitcoin-payment.satsto.me for HRN pay@satsto.me.
SHOULD display ₿pay@satsto.me. Currently ₿ displays as an unknown character in SeedSigner, so maybe it makes sense to leave it as pay@satsto.me for now.
|
|
||
|
|
||
|
|
||
| class PSBTDnsNameErrorView(View): |
There was a problem hiding this comment.
Same about the naming. As it is an error view, should use a name that identifies the reason of error which is an invalid DNSSEC proof for the provided HRN. Maybe PSBTDnssecProofError or PSBTDnsPaymentIntructionsError
| \xaer\x05\x03\x11\t\x8c\xd4\xcf\xaa\x07\x96\x9b\x05craig\x04user\x10_bitcoin-payment\rsparrowwallet\x03com\x00\x00\x10\x00\x01\x00\x00\x0e\x0f\x0032bitcoin:bc1qwthe43xeuasklclq4kvhreluv3hu92rzej42js\x05craig\x04user\x10_bitcoin-payment\rsparrowwallet\x03com\x00\x00.\x00\x01\x00\x00\x0e\x0f\x00e\x00\x10\r\x05\x00\x00\x0e\x10h\x91\xef\xe0h~)`\xbb&\rsparrowwallet \ | ||
| \x03com\x00\xe7\xae\x93\xd2;tw7ULMR\xdd\x1e\xc0\xf5\x8cA\x1cjGM\xa4l<$\xd0\xdb\x97\r\x86\xe9\x1b\xf9\x1b^\xab\xeb\x1e\xd5\x91!g\x8e\xf54\xa2Zou\xce\x05\x88\xe6RD9\xc1\x1d \x8f0\x1dF') | ||
|
|
||
| def PSBTaddwrongDNSSECproof(): |
There was a problem hiding this comment.
Invalid DNSSEC proof test vector should use one of the proposed in here that are presumably going to be in the BIP soon.
There was a problem hiding this comment.
I've never messed around much with PSBTs, but is a good practice to do all this flow testinghere? My guts tell me to just display here both new views and move this flow testing to test_flows_psbt.py + test_psbt_parser.py
There was a problem hiding this comment.
I don't know if it is the right place, but my intention was to create the screenshots of the two new screens. I still have to add the tests to test_flows_psbt.py + test_psbt_parser.py
| self.destination_amounts.append(self.psbt.tx.vout[i].value) | ||
| self.spend_amount += self.psbt.tx.vout[i].value | ||
|
|
||
| if out.dnssec_proof: |
There was a problem hiding this comment.
This line will raise an error in all PSBT parser test cases until diybitcoinhardware/embit#102 gets merged
There was a problem hiding this comment.
How should I handle this? Put a try and except?
There was a problem hiding this comment.
Since this functionality depends on the embit PR, this PR should only be merged into SeedSigner once those changes are merged into embit. So there’s no immediate need to add temporary error handling here.
| dns_name = dnssec_data[0].decode('utf-8').replace('@', '.user._bitcoin-payment.') | ||
| proof = dnssec_data[1] | ||
|
|
||
| verified = verify_dns_proof(dns_name+'.', proof) |
There was a problem hiding this comment.
More checking should happen here. What if the dns_name is not the same as the verified by the proof? I'm not sure if we should display the hrn from from the PSBT field or from the proof.
There was a problem hiding this comment.
If I understood it correctly and as explained in #768 , the dns_name should not be extracted from the proof as it may be different of the dns_name of the PSBT. When validating, we look if the proof allows that name. The way I am doing this is to first look if the verification was successful and then check if the proof allows for that name. I do that in the next line when checking if verified['verified_rrs'] have any records, as that would mean the proof is valid for that name. Do you think my approach is correct?
There was a problem hiding this comment.
Ah, I see now, you are right. dns_name should always come from the PSBT, and the proof just gets validated against that value. We don’t extract or compare a name from the proof itself. So my earlier note isn’t needed here. The key check is simply whether the proof validates for the PSBT’s dns_name, which is done entirely by dnssec-prover.
2 participants
Description
This pull request adds support for verifying DNSSEC proofs in Partially Signed Bitcoin Transactions (PSBTs) within SeedSigner, implementing BIP-353 (DNS-based Human-Readable Names for Bitcoin Wallets). When a user scans a PSBT, SeedSigner now checks for a DNS name (e.g., craig@sparrowwallet.com) and its associated DNSSEC proof. If both are present, SeedSigner verifies the proof against the provided DNS name. Based on the result, one of two new screens will appear:
Shows an error message if the DNSSEC proof is invalid or not found, then navigates back to the main screen.
Dependencies
This feature relies on a specific version of the embit library, available at diybitcoinhardware/embit#102. This version includes BIP-353 support for offline DNSSEC proof verification. Without it, the feature will not work.
This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before sumbitting the PR (with the embit version previously mentioned)If you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os:
TO DO: