feat(dip-18): switch to bech32m encoding with reference implementation #173
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces Base58Check platform addresses with bech32m (BIP‑350): adds HRP-based network IDs, a 1‑byte type field + 20‑byte HASH160 payload, bech32m encoding/decoding and validation, wallet/device UX changes, updated test vectors, and a new Python implementation with BIP‑39/BIP‑32 utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Mnemonic as "Mnemonic (BIP‑39)"
participant Seed as "Seed (PBKDF2‑HMAC‑SHA512)"
participant BIP32 as "BIP‑32 Derivation"
participant KeyOps as "Priv -> Compressed Pub"
participant Hash as "HASH160 (SHA256→RIPEMD160)"
participant Bech32m as "Bech32m Encoder (BIP‑350)"
participant Address as "Platform Address (HRP + type + hash)"
User->>Mnemonic: mnemonic_to_seed(mnemonic, passphrase)
Mnemonic->>Seed: PBKDF2‑HMAC‑SHA512
Seed->>BIP32: bip32_master(seed) / derive_path(...)
BIP32->>KeyOps: derive priv_key
KeyOps->>Hash: priv_to_compressed_pub() -> hash160()
Hash->>Bech32m: build payload (type_byte || hash160)
Bech32m->>Bech32m: convertbits(8→5), HRP expand, bech32m checksum
Bech32m->>Address: emit bech32m platform address
Address->>User: return encoded address
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dip-0018/bech32.py (2)
260-277: Consider renaming variableIfor clarity.The variable name
Imatches BIP-32 specification terminology but is flagged by linters as ambiguous (could be confused withlor1). Consider renaming tohmac_resultorderivedfor better readability.def bip32_master(seed: bytes) -> tuple[bytes, bytes]: """BIP-32: Derive master private key and chain code from seed.""" - I = hmac.new(b"Bitcoin seed", seed, "sha512").digest() - return I[:32], I[32:] # private_key, chain_code + hmac_result = hmac.new(b"Bitcoin seed", seed, "sha512").digest() + return hmac_result[:32], hmac_result[32:] # private_key, chain_code def bip32_derive_child(priv_key: bytes, chain_code: bytes, index: int) -> tuple[bytes, bytes]: """BIP-32: Derive child key at given index (hardened if index >= 0x80000000).""" if index >= 0x80000000: # hardened data = b'\x00' + priv_key + index.to_bytes(4, 'big') else: # normal sk = SigningKey.from_string(priv_key, curve=SECP256k1) pubkey = sk.get_verifying_key().to_string("compressed") data = pubkey + index.to_bytes(4, 'big') - I = hmac.new(chain_code, data, "sha512").digest() - child_key_int = (int.from_bytes(I[:32], 'big') + int.from_bytes(priv_key, 'big')) % SECP256k1.order - return child_key_int.to_bytes(32, 'big'), I[32:] + hmac_result = hmac.new(chain_code, data, "sha512").digest() + child_key_int = (int.from_bytes(hmac_result[:32], 'big') + int.from_bytes(priv_key, 'big')) % SECP256k1.order + return child_key_int.to_bytes(32, 'big'), hmac_result[32:]
432-433: Consider adding assertion messages for easier debugging.The assertions lack error messages, which would make debugging failures harder.
- assert dec_net_m == "mainnet" and dec_type_m == "p2pkh" and dec_hash_m.hex() == expected_hash - assert dec_net_t == "testnet" and dec_type_t == "p2pkh" and dec_hash_t.hex() == expected_hash + assert dec_net_m == "mainnet" and dec_type_m == "p2pkh" and dec_hash_m.hex() == expected_hash, \ + f"Mainnet round-trip failed for vector {i}" + assert dec_net_t == "testnet" and dec_type_t == "p2pkh" and dec_hash_t.hex() == expected_hash, \ + f"Testnet round-trip failed for vector {i}"dip-0018.md (1)
104-110: Consider referencing BIP-173 for the character set instead of Bitcoin Cash.The bech32 character set originates from BIP-173. While the Bitcoin Cash reference is accurate, referencing BIP-173 directly would be more canonical for a BIP-350-based specification.
-The bech32m encoding used in this specification requires a fixed, 32-character base-32 alphabet. The data portion of all encoded addresses MUST use the same character set as Bitcoin and [Bitcoin Cash](https://reference.cash/protocol/blockchain/encoding/cashaddr): +The bech32m encoding used in this specification requires a fixed, 32-character base-32 alphabet. The data portion of all encoded addresses MUST use the character set defined in [BIP-173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dip-0018.md(3 hunks)dip-0018/bech32.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
dip-0018.md
[style] ~144-~144: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s intended to receive Platform funds. * Wallets MUST treat HRP as the network selector....
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.14.7)
dip-0018/bech32.py
1-1: Shebang is present but file is not executable
(EXE001)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
96-96: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
196-196: Avoid specifying long messages outside the exception class
(TRY003)
199-199: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
231-231: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
262-262: Ambiguous variable name: I
(E741)
275-275: Ambiguous variable name: I
(E741)
🔇 Additional comments (8)
dip-0018/bech32.py (4)
24-39: LGTM! Bech32m constants are correctly defined per BIP-350.The charset, generator constants, and bech32m constant (0x2bc830a3) match the BIP-350 specification.
70-125: LGTM! Bech32m encode/decode implementation is correct.The encoding enforces lowercase output, and decoding properly validates character set, rejects mixed case, verifies checksum, and strips the 6-character checksum suffix.
184-211: LGTM! Platform address encoding follows DIP-18 specification.The function correctly validates inputs, constructs the 21-byte payload (type_byte || hash160), converts to 5-bit groups, and produces the bech32m-encoded address.
1-1: Make file executable or remove shebang.The shebang
#!/usr/bin/env python3is present but the file may not have executable permissions. Either make the file executable (chmod +x bech32.py) or remove the shebang if it's only intended to be run viapython bech32.py.dip-0018.md (4)
34-46: LGTM! Clear abstract and proper references to prior work.The abstract concisely explains the purpose, and the prior work section correctly references the foundational BIPs.
57-103: LGTM! Encoding algorithm is well-specified with clear steps.The 5-step encoding algorithm and decoder requirements are clearly documented with proper references to BIP-173 and BIP-350.
163-218: LGTM! Reference implementation pseudocode matches the Python implementation.The pseudocode accurately describes the encoding and decoding logic consistent with
bech32.py.
238-251: LGTM! Test vectors are consistent with the Python reference implementation.The P2PKH and P2SH test vectors match exactly between the specification and
bech32.py, ensuring implementations can be validated against the same data.
Replace Base58Check encoding with bech32m (BIP-350) for Platform addresses. Add Python reference implementation with full DIP-17 test vector validation.
thephez
force-pushed
the
feat/dip17-bech32m
branch
from
ccc7e0e to
d803fc0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dip-0018/bech32.py (2)
262-262: Consider renaming variableIfor clarity.Static analysis flags the variable name
I(lines 262, 275) as ambiguous per PEP 8. WhileIis conventional in BIP-32 literature for the HMAC-SHA512 output, consider using a more descriptive name likehmac_outputorderived_datato improve readability.Apply this diff to improve variable naming:
def bip32_master(seed: bytes) -> tuple[bytes, bytes]: """BIP-32: Derive master private key and chain code from seed.""" - I = hmac.new(b"Bitcoin seed", seed, "sha512").digest() - return I[:32], I[32:] # private_key, chain_code + hmac_output = hmac.new(b"Bitcoin seed", seed, "sha512").digest() + return hmac_output[:32], hmac_output[32:] # private_key, chain_codedef bip32_derive_child(priv_key: bytes, chain_code: bytes, index: int) -> tuple[bytes, bytes]: """BIP-32: Derive child key at given index (hardened if index >= 0x80000000).""" if index >= 0x80000000: # hardened data = b'\x00' + priv_key + index.to_bytes(4, 'big') else: # normal sk = SigningKey.from_string(priv_key, curve=SECP256k1) pubkey = sk.get_verifying_key().to_string("compressed") data = pubkey + index.to_bytes(4, 'big') - I = hmac.new(chain_code, data, "sha512").digest() - child_key_int = (int.from_bytes(I[:32], 'big') + int.from_bytes(priv_key, 'big')) % SECP256k1.order - return child_key_int.to_bytes(32, 'big'), I[32:] + hmac_output = hmac.new(chain_code, data, "sha512").digest() + child_key_int = (int.from_bytes(hmac_output[:32], 'big') + int.from_bytes(priv_key, 'big')) % SECP256k1.order + return child_key_int.to_bytes(32, 'big'), hmac_output[32:]Also applies to: 275-275
432-433: Consider replacing assertions with conditional error handling.The self-test uses
assertstatements (lines 432-433, 462-463) for round-trip validation. While assertions work for testing, they can be disabled withpython -O, potentially masking validation failures. Consider using explicitifchecks that setall_passed = Falseinstead.Apply this diff to replace assertions with explicit checks:
# Verify round-trip decoding dec_net_m, dec_type_m, dec_hash_m = decode_platform_address(mainnet_addr) dec_net_t, dec_type_t, dec_hash_t = decode_platform_address(testnet_addr) - assert dec_net_m == "mainnet" and dec_type_m == "p2pkh" and dec_hash_m.hex() == expected_hash - assert dec_net_t == "testnet" and dec_type_t == "p2pkh" and dec_hash_t.hex() == expected_hash + roundtrip_ok = ( + dec_net_m == "mainnet" and dec_type_m == "p2pkh" and dec_hash_m.hex() == expected_hash and + dec_net_t == "testnet" and dec_type_t == "p2pkh" and dec_hash_t.hex() == expected_hash + ) + if not roundtrip_ok: + print(" ✗ Round-trip decoding failed") + all_passed = False if not (main_ok and test_ok): all_passed = FalseSimilar change for P2SH validation:
# Verify round-trip decoding dec_net_m, dec_type_m, dec_hash_m = decode_platform_address(mainnet_addr) dec_net_t, dec_type_t, dec_hash_t = decode_platform_address(testnet_addr) - assert dec_net_m == "mainnet" and dec_type_m == "p2sh" and dec_hash_m.hex() == p2sh_hash - assert dec_net_t == "testnet" and dec_type_t == "p2sh" and dec_hash_t.hex() == p2sh_hash + roundtrip_ok = ( + dec_net_m == "mainnet" and dec_type_m == "p2sh" and dec_hash_m.hex() == p2sh_hash and + dec_net_t == "testnet" and dec_type_t == "p2sh" and dec_hash_t.hex() == p2sh_hash + ) + if not roundtrip_ok: + print(" ✗ Round-trip decoding failed") + all_passed = False if not (main_ok and test_ok): all_passed = FalseAlso applies to: 462-463
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)dip-0018.md(2 hunks)dip-0018/bech32.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dip-0018/bech32.py (1)
dip-0027/dip-0027-request-id-calc.py (1)
sha256(6-7)
🪛 GitHub Actions: .github/workflows/markdownlint.yml
README.md
[error] 5-5: markdownlint MD059/descriptive-link-text Link text should be descriptive [Context: "[here]"]
🪛 LanguageTool
dip-0018.md
[style] ~144-~144: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s intended to receive Platform funds. * Wallets MUST treat HRP as the network selector....
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.14.8)
dip-0018/bech32.py
76-76: Avoid specifying long messages outside the exception class
(TRY003)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
96-96: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
113-113: Avoid specifying long messages outside the exception class
(TRY003)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
196-196: Avoid specifying long messages outside the exception class
(TRY003)
199-199: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
231-231: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
262-262: Ambiguous variable name: I
(E741)
275-275: Ambiguous variable name: I
(E741)
🔇 Additional comments (13)
README.md (1)
37-37: LGTM! Author metadata correctly updated.The addition of thephez as co-author aligns with the PR objectives and contributions.
dip-0018/bech32.py (6)
128-162: LGTM! Base conversion implementation is correct.The
convertbitsfunction correctly handles 8-bit to 5-bit conversion (and vice versa) with appropriate padding validation. The logic matches the BIP-173 specification.
164-242: LGTM! Platform address encoding/decoding correctly implements DIP-18.The implementation properly:
- Maps network identifiers to HRPs (
dashevo,tdashevo)- Maps address types to type bytes (P2PKH=0x00, P2SH=0x01)
- Constructs 21-byte payloads (type_byte || hash160)
- Validates all inputs and payload structure
- Uses bech32m encoding/decoding with appropriate padding rules
288-290: LGTM! HASH160 implementation is correct.The
hash160function correctly implements RIPEMD160(SHA256(data)), which is the standard HASH160 operation used in Bitcoin and Dash.
310-348: LGTM! Test vectors are comprehensive.The test vectors cover:
- Three DIP-17 P2PKH derivation paths with full key derivation chain
- Both mainnet and testnet address encodings
- P2SH address encoding example
These align with the DIP-17 and DIP-18 specifications.
351-475: LGTM! Self-test provides comprehensive validation.The self-test main block:
- Validates DIP-17 key derivation against test vectors
- Validates DIP-18 P2PKH and P2SH address encoding
- Includes round-trip decode verification
- Provides clear output with checkmarks for pass/fail
- Exits with non-zero status on failure
This provides good validation coverage for the reference implementation.
24-126: The bech32m implementation is correct and fully compliant with BIP-350.All cryptographic constants match the official specification: BECH32M_CONST is 0x2bc830a3, the GENERATOR array contains the correct values [0x3b6a57b2, 0x26508e6d, 0x1ea119fa, 0x3d4233dd, 0x2a1462b3], and the Bech32 character set is correct. The polymod algorithm, HRP expansion, and checksum creation/verification logic all conform to BIP-173/350 standards. Validation logic properly enforces character restrictions, rejects mixed case, validates length constraints (8-90 characters), checks separator positioning, and verifies checksums. No issues found.
dip-0018.md (6)
4-4: LGTM! Author metadata correctly updated.Consistent with the README.md update and PR contributions.
34-40: LGTM! Abstract and motivation are clear.The abstract effectively summarizes the bech32m encoding format for Dash Platform addresses, and the motivation clearly explains the benefits over alternatives.
48-147: LGTM! Specification is comprehensive and consistent with implementation.The specification section clearly defines:
- Address types (P2PKH, P2SH)
- Bech32m encoding algorithm with step-by-step details
- Network parameters (HRPs and type bytes)
- Validation requirements
- Wallet and hardware wallet behavior
All details align with the Python reference implementation in
dip-0018/bech32.py.
149-157: LGTM! Rationale clearly articulates bech32m benefits.The rationale section effectively explains why bech32m was chosen over Base58Check, highlighting improved checksum strength, QR efficiency, network separation, and extensibility.
163-220: LGTM! Reference implementation pseudocode matches Python code.The pseudocode for
encode_platform_addressanddecode_platform_addressaccurately reflects the logic indip-0018/bech32.py, including:
- Input validation
- Type and network mappings
- Payload construction/extraction
- Bech32m encoding/decoding calls
- Error handling
233-253: Test vectors are correct and match the reference implementation.The P2PKH vectors (lines 242–246) and P2SH vector (lines 248–253) in the specification match exactly with those hardcoded in
dip-0018/bech32.py. All 3 P2PKH test vectors and the P2SH example have been verified to be identical in both the markdown and the Python reference implementation, including the HASH160 payloads and corresponding mainnet/testnet addresses.
Normalize mnemonic and passphrase with NFKD before UTF-8 encoding per BIP-39 spec.
Replace custom Bech32m implementation with exact code from sipa's reference at github.com/sipa/bech32. Adds MIT license header and proper attribution.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Replace Base58Check encoding with bech32m (BIP-350) for Platform addresses.
Add Python reference implementation with full DIP-17 test vector validation.
Summary by CodeRabbit
New Features
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.