-
Notifications
You must be signed in to change notification settings - Fork 40
[New Feature] Add BIP-353 support #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
* Generated with uniffi with cross-compilation from macOS
|
|
||
| # Extract the Bitcoin URI from the verified TXT DNS records only | ||
| verified_rrs = result.get("verified_rrs", []) | ||
| txt_records = [rr for rr in verified_rrs if rr.get("type") == "txt"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtering verified_rrs for TXT records is redundant since verify_dns_proof already filters to only TXT records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, didn't notice... thanks for the heads up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe it does, though an HRN proof in a PSBT generally shouldn't be proving other records :)
src/embit/bip353.py
Outdated
|
|
||
| # Validate that at least one payment method is available | ||
| has_regular_address = payment_info["address"] is not None | ||
| has_silent_payment = payment_info["silent_payment_address"] is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this belongs here, given that sending silent payments isn’t supported yet. should we just leave a TODO for now?
| except (InvalidOperation, ValueError, OverflowError) as e: | ||
| raise BIP21Error(f"'{value}' is not a valid amount: {e}") | ||
|
|
||
| elif name == self.FIELD_SILENT_PAYMENT and value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
embit still lacks support for sending silent payments and doesn't handle decoding of large payloads. i'm wondering what happens if a dns instruction contains only a silent payment address.
also, this code ain't decoding the sp addresses properly. i think we should think this through carefully before adding this here. incomplete validation might be worse than no validation at all. maybe we can leave a TODO for now(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I partially agree with you. I should remove those decoding conditions I set completely arbitrarily. But would it hurt to enable DNS record validation for SP? I would say no, at least we should add minimal support. If we don’t add any support and we receive a DNS record with an SP address, should we raise an error? I think that may be overkill, as it’s not directly related to DNSSEC validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as validation goes, i don’t think it’s harmful—but the point i’m trying to make is that if we try to send to a silent payment address, it’s going to fail anyway since there’s no support for it yet.
that said, if you’d like to keep the validation, i’d suggest adding a test vector in test_bip21.py to go along with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see what others devs think. What we have now is mostly a hybrid BIP-21/BIP-321 implementation for the URI without all the Lightning stuff. Maybe it makes sense to also ignore the SP stuff as you propose.
* enabled multiple bc and sp queries as per BIP321 draft * internally manage "bitcoin:<address>" with no explicit field
|
BIP21 replacement by BIP321 is being proposed in bitcoin/bips#1911. Maybe it makes sense to go full BIP321 compatible? |
|
I think this is BIP 321 compatible already? |
|
Yes, if we leave aside the Lightning elements |
|
|
||
| # Extract the Bitcoin URI from the verified TXT DNS records only | ||
| verified_rrs = result.get("verified_rrs", []) | ||
| txt_records = [rr for rr in verified_rrs if rr.get("type") == "txt"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe it does, though an HRN proof in a PSBT generally shouldn't be proving other records :)
|
BIP 321 doesn't require you to implement lightning, if its confusing anywhere and implies that let me know. |
|
It would help my end-to-end testing if someone could create or point me to a DNS entry that resolves to a testnet addr. |
*Allow multiple TXT records in DNSSEC proof per BIP353 specification *Only one TXT record can start with "bitcoin:" (case insensitive) *Add validation from bitcoin/bips#1912
|
@kdmukai try with test@alvroble.com |
src/embit/bip353.py
Outdated
| except Exception as e: | ||
| return {"error": f"DNSSEC proof verification failed for HRN '{hrn}': {e}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the exception convention in embit, but my first instinct here is to define a specific Exception (e.g. something like DNSSECProofValidationFailureException).
Also is there any diversity of exception types that might get thrown by the library call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From dnssec_prover.py, I think only InternalError applies, which is more related to the Rust FFI that to the actual functionality. No other specific error applies from the generated bindings file perspective.
|
The main README should be updated to explain the new binaries. I think there should also be some documentation (maybe in a separate doc) about how the binaries were made and how to reproduce them, links to the rust repo, etc. |
- Add comprehensive build instructions for reproducing dnssec-prover binaries - Include reproducible build hashes for verification - Added binaries that can be reproduced and hash-attested - Update main README to reference dnssec-prover binaries
|
Added reproducible build instructions for dnssec-prover and updated README. Uploaded hash-attested binaries. Please test to confirm everything builds correctly. |
This PR implements BIP-353 "DNS Payment Instructions" for embit, enabling Bitcoin payments to human-readable names (HRNs) with offline DNSSEC validation capabilities.
Resolves #85
Related SeedSigner/seedsigner#768
Overview
BIP-353 allows Bitcoin payments to be directed to human-readable names instead of cryptographic addresses, using DNSSEC proofs for secure verification. This implementation provides complete infrastructure for BIP-353 compliance, including offline validation support for hardware wallet integration.
Related Work
This implementation leverages @TheBlueMatt's dnssec-prover Rust tool for offline DNSSEC validation. The tool has been cross-compiled and integrated with Python bindings for embit integration.
Implementation Details
PSBT field
Added
PSBT_OUT_DNSSEC_PROOFfield to embed DNSSEC proofs in PSBT outputs. This enables wallets to include DNSSEC validation for the payment destination, allowing offline verification of the transaction.Noticed there's an overlap here with #101. The implementation approach is quite similar. I had committed this change a few weeks ago but only just got around to publishing it. To avoid conflicts, we might want to consolidate both efforts or drop one of the overlapping changes. Open to suggestions on how best to proceed.
dnssec-proverintegrationIntegrated
dnssec-provertool with Python bindings for cross-platform support. The prebuilt binaries and bindings were generated by usinguniffion the maindnssec-proverbranch.Added for the same set of platforms as secp256k1. Tested on macOS and Raspberry Pi 3B. Additional testing is needed for more SeedSigner platforms and MicroPython environments.
BIP-353 implementation
The core implementation provides two main functions:
verify_dns_proof()- Validates DNSSEC proofs and extracts TXT recordsget_payment_info_from_hrn()- Extracts complete payment information from HRNsThe implementation filters to only process TXT records (as required by BIP-353) and supports both regular Bitcoin addresses and silent payment addresses.
Added test vectors from Sparrow Wallet's implementation.
BIP-21 URI support
Added complete BIP-21 URI scheme implementation to parse Bitcoin URIs from DNS TXT records. This handles all standard Bitcoin URI parameters including amounts, labels, messages, and silent payment addresses.
Considerations
dnssec-prover. Main benefit would be not having to rely on prebuilt binaries. On the other hand, that library is recent and not as tested asdnssec-prover.References