commit 6da7d511bf5afe9241caa6d16a16595fe91348c6
parent 31e492a1d5445339c632473f1900d178f50289c8
Author: Jared Tobin <jared@jtobin.io>
Date: Sun, 25 Jan 2026 14:37:46 +0400
meta: docs
Diffstat:
| A | plans/ARCH2.md | | | 176 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | plans/IMPL2a.md | | | 128 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | plans/IMPL2b.md | | | 116 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
3 files changed, 420 insertions(+), 0 deletions(-)
diff --git a/plans/ARCH2.md b/plans/ARCH2.md
@@ -0,0 +1,176 @@
+# ARCH2: Refactoring for Abstraction and Type Safety
+
+## Overview
+
+This document describes refactoring opportunities identified in the ppad-bolt3
+library to reduce code duplication and improve type-level invariant
+enforcement.
+
+## Goals
+
+1. **Reduce duplication** in HTLC transaction builders and key derivation
+2. **Enforce size invariants** on ByteString-based newtypes at construction
+3. **Enforce range invariants** on numeric types (48-bit CommitmentNumber)
+
+## Non-Goals
+
+- Changing the public API signatures (beyond adding smart constructors)
+- Adding runtime overhead to hot paths
+- Breaking existing tests
+
+---
+
+## 1. Size-Constrained ByteString Types
+
+### Current State
+
+Several newtypes wrap `ByteString` without enforcing expected sizes:
+
+| Type | Expected Size |
+|---------------------|---------------|
+| `Pubkey` | 33 bytes |
+| `Point` | 33 bytes |
+| `Seckey` | 32 bytes |
+| `TxId` | 32 bytes |
+| `PaymentHash` | 32 bytes |
+| `PaymentPreimage` | 32 bytes |
+| `PerCommitmentSecret` | 32 bytes |
+
+### Proposed Change
+
+Add smart constructors that validate size, returning `Maybe`:
+
+```haskell
+-- | Parse a 33-byte compressed public key.
+pubkey :: BS.ByteString -> Maybe Pubkey
+pubkey bs
+ | BS.length bs == 33 = Just (Pubkey bs)
+ | otherwise = Nothing
+
+-- | Parse a 32-byte transaction ID.
+txid :: BS.ByteString -> Maybe TxId
+txid bs
+ | BS.length bs == 32 = Just (TxId bs)
+ | otherwise = Nothing
+```
+
+The raw constructors remain exported for internal use and for cases where
+the caller has already validated the input (e.g., from cryptographic
+operations that guarantee output size).
+
+### Impact
+
+- Callers parsing external data get early validation
+- No runtime overhead for internal construction paths
+- Tests may need updates for smart constructor usage
+
+---
+
+## 2. CommitmentNumber Range Constraint
+
+### Current State
+
+`CommitmentNumber` wraps `Word64` but semantically represents a 48-bit value
+(max 2^48 - 1 = 281474976710655).
+
+### Proposed Change
+
+Add a smart constructor that validates the range:
+
+```haskell
+-- | Create a commitment number (must be < 2^48).
+commitment_number :: Word64 -> Maybe CommitmentNumber
+commitment_number n
+ | n <= 0xFFFFFFFFFFFF = Just (CommitmentNumber n)
+ | otherwise = Nothing
+```
+
+### Impact
+
+- Prevents invalid commitment numbers from propagating
+- Minimal - most internal uses are already within range
+
+---
+
+## 3. HTLC Transaction Builder Abstraction
+
+### Current State
+
+`build_htlc_timeout_tx` and `build_htlc_success_tx` in `Tx.hs` share ~80%
+of their implementation:
+
+- Same output script construction
+- Same input/output structure
+- Differ only in: locktime source, fee function
+
+### Proposed Change
+
+Factor out a common helper:
+
+```haskell
+-- Internal helper for HTLC transaction construction
+build_htlc_tx_common
+ :: HTLCContext
+ -> Locktime -- ^ Locktime for the transaction
+ -> Satoshi -- ^ Fee to subtract
+ -> HTLCTx
+
+build_htlc_timeout_tx :: HTLCContext -> HTLCTx
+build_htlc_timeout_tx ctx =
+ let !fee = htlc_timeout_fee (hc_feerate ctx) (hc_features ctx)
+ !locktime = Locktime (unCltvExpiry $ htlc_cltv_expiry $ hc_htlc ctx)
+ in build_htlc_tx_common ctx locktime fee
+
+build_htlc_success_tx :: HTLCContext -> HTLCTx
+build_htlc_success_tx ctx =
+ let !fee = htlc_success_fee (hc_feerate ctx) (hc_features ctx)
+ in build_htlc_tx_common ctx (Locktime 0) fee
+```
+
+### Impact
+
+- Reduces code duplication
+- Makes the difference between timeout/success explicit
+- No API change
+
+---
+
+## 4. Key Derivation Abstraction
+
+### Current State
+
+In `Keys.hs`, several derivation functions are structurally identical:
+
+- `derive_local_htlcpubkey` / `derive_remote_htlcpubkey`
+- `derive_local_delayedpubkey` / `derive_remote_delayedpubkey`
+
+Each just unwraps a basepoint, calls `derive_pubkey`, and wraps in a
+different newtype.
+
+### Proposed Change
+
+Keep the explicit functions (they provide good type safety and
+documentation) but have them delegate to a single internal helper pattern.
+The current implementation already does this via `derive_pubkey`, so this
+is more about recognizing the pattern is intentional rather than changing
+it.
+
+Alternatively, could add a type class but that adds complexity for minimal
+gain.
+
+### Decision
+
+**Keep as-is.** The "duplication" is intentional type safety - each function
+has a distinct type signature that prevents mixing up local/remote keys.
+The actual derivation logic (`derive_pubkey`) is already shared.
+
+---
+
+## Summary of Changes
+
+| Area | Change | Priority |
+|------|--------|----------|
+| Types.hs | Smart constructors for sized ByteStrings | High |
+| Types.hs | Smart constructor for CommitmentNumber | Medium |
+| Tx.hs | Factor HTLC tx builder common code | Medium |
+| Keys.hs | Keep as-is (already well-factored) | N/A |
diff --git a/plans/IMPL2a.md b/plans/IMPL2a.md
@@ -0,0 +1,128 @@
+# IMPL2a: Smart Constructors for Type Safety
+
+Implements the type-level invariant enforcement from ARCH2.md.
+
+## Scope
+
+- Add smart constructors for size-constrained ByteString newtypes
+- Add smart constructor for CommitmentNumber (48-bit range)
+- Update exports in Types.hs and BOLT3.hs
+
+## Tasks
+
+### 1. Add smart constructors to Types.hs
+
+Add the following smart constructors in `lib/Lightning/Protocol/BOLT3/Types.hs`:
+
+```haskell
+-- 33-byte types
+pubkey :: BS.ByteString -> Maybe Pubkey
+point :: BS.ByteString -> Maybe Point
+
+-- 32-byte types
+seckey :: BS.ByteString -> Maybe Seckey
+txid :: BS.ByteString -> Maybe TxId
+payment_hash :: BS.ByteString -> Maybe PaymentHash
+payment_preimage :: BS.ByteString -> Maybe PaymentPreimage
+per_commitment_secret :: BS.ByteString -> Maybe PerCommitmentSecret
+
+-- 48-bit range
+commitment_number :: Word64 -> Maybe CommitmentNumber
+```
+
+Implementation pattern:
+
+```haskell
+-- | Parse a 33-byte compressed public key.
+--
+-- Returns Nothing if the input is not exactly 33 bytes.
+--
+-- >>> pubkey (BS.replicate 33 0x02)
+-- Just (Pubkey ...)
+-- >>> pubkey (BS.replicate 32 0x02)
+-- Nothing
+pubkey :: BS.ByteString -> Maybe Pubkey
+pubkey bs
+ | BS.length bs == 33 = Just (Pubkey bs)
+ | otherwise = Nothing
+{-# INLINE pubkey #-}
+```
+
+### 2. Update Types.hs exports
+
+Add the smart constructors to the export list, grouped with their
+corresponding types:
+
+```haskell
+ -- * Keys and points
+ , Pubkey(..)
+ , pubkey
+ , Seckey(..)
+ , seckey
+ , Point(..)
+ , point
+ ...
+```
+
+### 3. Update BOLT3.hs re-exports
+
+Add the new smart constructors to the main module's export list:
+
+```haskell
+ -- ** Keys and points
+ , Pubkey(..)
+ , pubkey
+ , Seckey(..)
+ , seckey
+ , Point(..)
+ , point
+ ...
+```
+
+### 4. Add tests for smart constructors
+
+Add test cases to `test/Main.hs`:
+
+```haskell
+smartConstructorTests :: TestTree
+smartConstructorTests = testGroup "Smart constructors" [
+ testCase "pubkey accepts 33 bytes" $ do
+ let bs = BS.replicate 33 0x02
+ isJust (pubkey bs) @?= True
+ , testCase "pubkey rejects 32 bytes" $ do
+ let bs = BS.replicate 32 0x02
+ isNothing (pubkey bs) @?= True
+ , testCase "commitment_number accepts 2^48-1" $ do
+ isJust (commitment_number 281474976710655) @?= True
+ , testCase "commitment_number rejects 2^48" $ do
+ isNothing (commitment_number 281474976710656) @?= True
+ ...
+ ]
+```
+
+### 5. Verify build and tests pass
+
+```bash
+nix develop -c cabal build all
+nix develop -c cabal test
+```
+
+## Files Modified
+
+- `lib/Lightning/Protocol/BOLT3/Types.hs`
+- `lib/Lightning/Protocol/BOLT3.hs`
+- `test/Main.hs`
+
+## Commit Message
+
+```
+Add smart constructors for type-safe parsing
+
+- pubkey/point: validate 33-byte compressed EC points
+- seckey/txid/payment_hash/payment_preimage: validate 32-byte values
+- per_commitment_secret: validate 32-byte secrets
+- commitment_number: validate 48-bit range
+
+Raw constructors remain exported for internal use where size is
+already guaranteed by construction.
+```
diff --git a/plans/IMPL2b.md b/plans/IMPL2b.md
@@ -0,0 +1,116 @@
+# IMPL2b: HTLC Transaction Builder Abstraction
+
+Implements the HTLC transaction builder refactoring from ARCH2.md.
+
+## Scope
+
+- Factor common code from `build_htlc_timeout_tx` and `build_htlc_success_tx`
+- No public API changes
+
+## Tasks
+
+### 1. Add internal helper function to Tx.hs
+
+Add a common builder function in `lib/Lightning/Protocol/BOLT3/Tx.hs`
+(not exported):
+
+```haskell
+-- | Internal helper for HTLC transaction construction.
+--
+-- Both HTLC-timeout and HTLC-success transactions share the same
+-- structure, differing only in locktime and fee calculation.
+build_htlc_tx_common
+ :: HTLCContext
+ -> Locktime -- ^ Transaction locktime
+ -> Satoshi -- ^ Fee to subtract from output
+ -> HTLCTx
+build_htlc_tx_common ctx locktime fee =
+ let !amountSat = msat_to_sat (htlc_amount_msat $ hc_htlc ctx)
+ !outputValue = if unSatoshi amountSat >= unSatoshi fee
+ then Satoshi (unSatoshi amountSat - unSatoshi fee)
+ else Satoshi 0
+ !inputSeq = if has_anchors (hc_features ctx)
+ then Sequence 1
+ else Sequence 0
+ !outpoint = Outpoint (hc_commitment_txid ctx) (hc_output_index ctx)
+ !outputScript = to_p2wsh $ htlc_output_script
+ (hc_revocation_pubkey ctx)
+ (hc_to_self_delay ctx)
+ (hc_local_delayed ctx)
+ in HTLCTx
+ { htx_version = 2
+ , htx_locktime = locktime
+ , htx_input_outpoint = outpoint
+ , htx_input_sequence = inputSeq
+ , htx_output_value = outputValue
+ , htx_output_script = outputScript
+ }
+{-# INLINE build_htlc_tx_common #-}
+```
+
+### 2. Refactor build_htlc_timeout_tx
+
+Replace the current implementation with:
+
+```haskell
+-- | Build an HTLC-timeout transaction.
+--
+-- * locktime: cltv_expiry
+-- * sequence: 0 (or 1 with option_anchors)
+-- * output: to_local style script with revocation and delayed paths
+build_htlc_timeout_tx :: HTLCContext -> HTLCTx
+build_htlc_timeout_tx ctx =
+ let !fee = htlc_timeout_fee (hc_feerate ctx) (hc_features ctx)
+ !locktime = Locktime (unCltvExpiry $ htlc_cltv_expiry $ hc_htlc ctx)
+ in build_htlc_tx_common ctx locktime fee
+{-# INLINE build_htlc_timeout_tx #-}
+```
+
+### 3. Refactor build_htlc_success_tx
+
+Replace the current implementation with:
+
+```haskell
+-- | Build an HTLC-success transaction.
+--
+-- * locktime: 0
+-- * sequence: 0 (or 1 with option_anchors)
+-- * output: to_local style script with revocation and delayed paths
+build_htlc_success_tx :: HTLCContext -> HTLCTx
+build_htlc_success_tx ctx =
+ let !fee = htlc_success_fee (hc_feerate ctx) (hc_features ctx)
+ in build_htlc_tx_common ctx (Locktime 0) fee
+{-# INLINE build_htlc_success_tx #-}
+```
+
+### 4. Verify build and tests pass
+
+```bash
+nix develop -c cabal build all
+nix develop -c cabal test
+```
+
+Existing tests should continue to pass since the public API is unchanged.
+
+### 5. Run benchmarks to verify no regression
+
+```bash
+nix develop -c cabal bench bolt3-bench
+```
+
+## Files Modified
+
+- `lib/Lightning/Protocol/BOLT3/Tx.hs`
+
+## Commit Message
+
+```
+Refactor HTLC transaction builders to share common code
+
+Factor out build_htlc_tx_common helper that handles the shared
+transaction structure. build_htlc_timeout_tx and build_htlc_success_tx
+now differ only in their locktime and fee parameters, making the
+distinction between them explicit.
+
+No public API changes.
+```
