feat: support sending BTC to P2SH/P2PKH destination addresses

Confirmed live via logcat: the real "QR can't be decoded" error was neither a QR format issue (the earlier BIP21 fix, while correct, wasn't the cause here) nor a scan failure - the raw QR content was a bare P2SH address (3...), which this wallet's native-SegWit-only address validation rejected outright. A real exchange (OKX) withdrawal address turned out to be P2SH-only with no way to request native SegWit instead. Adds Base58Check decoding and BitcoinDestinationAddress.kt (P2WPKH/P2SH/ P2PKH, each producing the correct scriptPubKey shape - these differ from each other, not just cosmetically). This wallet's OWN address/derivation stays native-SegWit-only and untouched (bitcoinAddressToAccountId is unaffected) - only the SEND path now recognizes wider destination types. BitcoinTransactionService/RealBitcoinTransactionService now take the raw recipient address string instead of a pre-resolved AccountId - converting to AccountId this early would have discarded which script shape the destination actually needs, silently building a P2WPKH output for a P2SH recipient (a real fund-misdirection risk, not just a validation gap). Test vectors: Satoshi's genesis P2PKH address, a well-known P2SH vanity address, and the real OKX withdrawal address from this session's live QR scan, all cross-verified against an independent Python implementation before writing the Kotlin equivalent. Reverts the temporary QrDiag logging added to find this - no longer needed.
2026-07-22 13:45:48 +00:00 · 2026-07-13 05:54:37 -07:00
parent ff19c7c4ae
commit d4faa64c19
8 changed files with 243 additions and 23 deletions
@@ -0,0 +1,53 @@
+package io.novafoundation.nova.common.utils
+
+import java.math.BigInteger
+
+private const val BASE58_ALPHABET = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"
+
+/**
+ * Base58Check - the legacy encoding used by Bitcoin's P2PKH (`1...`) and P2SH (`3...`) addresses, as opposed to
+ * native SegWit's bech32. Needed only to recognize/decode EXTERNAL destination addresses for sending (many
+ * exchanges, including at least one confirmed live, only offer a P2SH withdrawal address with no way to pick a
+ * native SegWit one) - this app's OWN address (derivation, receiving, `bitcoinAddressToAccountId()`) remains
+ * native SegWit only, unaffected by this.
+ */
+object Base58Check {
+
+    class Decoded(val version: Int, val payload: ByteArray)
+
+    fun decode(address: String): Decoded {
+        val full = base58Decode(address)
+        require(full.size == 25) { "Base58Check payload must be 25 bytes, got ${full.size}" }
+
+        val versionAndPayload = full.copyOfRange(0, 21)
+        val checksum = full.copyOfRange(21, 25)
+        val expectedChecksum = versionAndPayload.sha256d().copyOfRange(0, 4)
+
+        require(checksum.contentEquals(expectedChecksum)) { "Base58Check checksum mismatch" }
+
+        return Decoded(version = versionAndPayload[0].toInt() and 0xff, payload = versionAndPayload.copyOfRange(1, 21))
+    }
+
+    private fun base58Decode(input: String): ByteArray {
+        require(input.isNotEmpty()) { "Base58 input must not be empty" }
+
+        var value = BigInteger.ZERO
+        val base = BigInteger.valueOf(58)
+
+        for (c in input) {
+            val digit = BASE58_ALPHABET.indexOf(c)
+            require(digit >= 0) { "Invalid base58 character: $c" }
+            value = value.multiply(base).add(BigInteger.valueOf(digit.toLong()))
+        }
+
+        val bytes = value.toByteArray().let {
+            // BigInteger.toByteArray() may prepend a 0x00 sign-disambiguation byte - strip it, it is not part of the data.
+            if (it.size > 1 && it[0] == 0.toByte()) it.copyOfRange(1, it.size) else it
+        }
+
+        // Each leading '1' character encodes a leading zero byte that BigInteger's conversion would otherwise drop.
+        val leadingZeros = input.takeWhile { it == '1' }.length
+
+        return ByteArray(leadingZeros) + bytes
+    }
+}
@@ -0,0 +1,60 @@
+package io.novafoundation.nova.common.utils
+
+private const val P2PKH_VERSION = 0x00
+private const val P2SH_VERSION = 0x05
+
+/**
+ * Any Bitcoin address this wallet can SEND to - a strict superset of what it can derive/receive as its own
+ * address (native SegWit only, see `BitcoinAddress.kt`). Needed because a real exchange withdrawal address was
+ * confirmed live to be P2SH (`3...`), which this app's original native-SegWit-only `isValidBitcoinAddress()`
+ * rejected outright, blocking the send with a misleading "QR can't be decoded" error (the QR was fine - a bare
+ * P2SH address - the address TYPE just wasn't recognized as a valid destination at all).
+ *
+ * Deliberately kept separate from [bitcoinAddressToAccountId]/[AccountId] - that function's 20-byte output feeds
+ * generic multi-chain code (`Chain.accountIdOf`) that assumes every account id is THIS wallet's own P2WPKH
+ * shape. Silently reusing it for a P2SH/P2PKH recipient would produce a 20-byte hash with no type tag, and
+ * downstream code would wrap it in the wrong (P2WPKH) scriptPubKey - sending funds to an address that doesn't
+ * match what was actually asked for. [BitcoinDestination] carries its type through to [toScriptPubKey] instead.
+ */
+sealed class BitcoinDestination {
+
+    data class NativeSegwit(val witnessProgram: ByteArray) : BitcoinDestination()
+
+    data class P2sh(val scriptHash: ByteArray) : BitcoinDestination()
+
+    data class P2pkh(val pubKeyHash: ByteArray) : BitcoinDestination()
+}
+
+fun String.decodeBitcoinDestination(): BitcoinDestination {
+    runCatching {
+        val decoded = SegwitAddress.decode("bc", this)
+        if (decoded.witnessVersion == 0 && decoded.witnessProgram.size == 20) {
+            return BitcoinDestination.NativeSegwit(decoded.witnessProgram)
+        }
+    }
+
+    val decoded = Base58Check.decode(this)
+
+    return when (decoded.version) {
+        P2PKH_VERSION -> BitcoinDestination.P2pkh(decoded.payload)
+        P2SH_VERSION -> BitcoinDestination.P2sh(decoded.payload)
+        else -> error("Unsupported Bitcoin address version byte: ${decoded.version}")
+    }
+}
+
+fun String.isValidBitcoinDestinationAddress(): Boolean = runCatching { decodeBitcoinDestination() }.isSuccess
+
+/**
+ * @return the scriptPubKey a transaction output must use to actually pay this destination - P2SH/P2PKH have
+ * different script shapes from this wallet's own P2WPKH (see [toP2wpkhScriptPubKey]), despite all three being a
+ * "20-byte hash wrapped in a short script."
+ */
+fun BitcoinDestination.toScriptPubKey(): ByteArray = when (this) {
+    is BitcoinDestination.NativeSegwit -> byteArrayOf(0x00, 0x14) + witnessProgram
+
+    // OP_HASH160 <20-byte-push> OP_EQUAL
+    is BitcoinDestination.P2sh -> byteArrayOf(0xa9.toByte(), 0x14) + scriptHash + byteArrayOf(0x87.toByte())
+
+    // OP_DUP OP_HASH160 <20-byte-push> OP_EQUALVERIFY OP_CHECKSIG
+    is BitcoinDestination.P2pkh -> byteArrayOf(0x76, 0xa9.toByte(), 0x14) + pubKeyHash + byteArrayOf(0x88.toByte(), 0xac.toByte())
+}
@@ -0,0 +1,99 @@
+package io.novafoundation.nova.common.utils
+
+import io.novasama.substrate_sdk_android.extensions.toHexString
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * Test vectors:
+ * - [genesisP2pkhAddress] is Satoshi's genesis coinbase address - real, independently-verifiable, its hash160
+ *   cross-checked via Python's hashlib/base58 at implementation time.
+ * - [okxWithdrawalAddress] is a real address confirmed live: an OKX BTC withdrawal QR, whose bare (non-BIP21)
+ *   content this app's original native-SegWit-only address validation rejected outright, producing a misleading
+ *   "QR can't be decoded" error - the actual bug this file's code fixes.
+ */
+class BitcoinDestinationAddressTest {
+
+    private val genesisP2pkhAddress = "1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa"
+    private val genesisP2pkhHash160 = "62e907b15cbf27d5425399ebf6f0fb50ebb88f18"
+
+    private val piVanityP2shAddress = "3P14159f73E4gFr7JterCCQh9QjiTjiZrG"
+
+    private val okxWithdrawalAddress = "3QBsCZAv5hsZSrDpTcQYEqd82TdA8Qr3g9"
+    private val okxWithdrawalHash160 = "f6c78c3a049bfa34ed05b22ca9515414cdcb46d1"
+
+    private val nativeSegwitAddress = "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4"
+    private val nativeSegwitAccountId = "751e76e8199196d454941c45d1b3a323f1433bd6"
+
+    @Test
+    fun `should decode a known P2PKH address to the correct hash160`() {
+        val destination = genesisP2pkhAddress.decodeBitcoinDestination()
+
+        assertTrue(destination is BitcoinDestination.P2pkh)
+        assertEquals(genesisP2pkhHash160, (destination as BitcoinDestination.P2pkh).pubKeyHash.toHexString(withPrefix = false))
+    }
+
+    @Test
+    fun `should decode a known P2SH address to a P2sh destination`() {
+        val destination = piVanityP2shAddress.decodeBitcoinDestination()
+
+        assertTrue(destination is BitcoinDestination.P2sh)
+    }
+
+    @Test
+    fun `should decode the real OKX withdrawal address to the correct P2SH hash160`() {
+        val destination = okxWithdrawalAddress.decodeBitcoinDestination()
+
+        assertTrue(destination is BitcoinDestination.P2sh)
+        assertEquals(okxWithdrawalHash160, (destination as BitcoinDestination.P2sh).scriptHash.toHexString(withPrefix = false))
+    }
+
+    @Test
+    fun `should still decode a native segwit address as NativeSegwit`() {
+        val destination = nativeSegwitAddress.decodeBitcoinDestination()
+
+        assertTrue(destination is BitcoinDestination.NativeSegwit)
+        assertEquals(nativeSegwitAccountId, (destination as BitcoinDestination.NativeSegwit).witnessProgram.toHexString(withPrefix = false))
+    }
+
+    @Test
+    fun `isValidBitcoinDestinationAddress should accept P2PKH, P2SH and native segwit`() {
+        assertTrue(genesisP2pkhAddress.isValidBitcoinDestinationAddress())
+        assertTrue(piVanityP2shAddress.isValidBitcoinDestinationAddress())
+        assertTrue(okxWithdrawalAddress.isValidBitcoinDestinationAddress())
+        assertTrue(nativeSegwitAddress.isValidBitcoinDestinationAddress())
+    }
+
+    @Test
+    fun `isValidBitcoinDestinationAddress should reject a corrupted checksum`() {
+        assertFalse("1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNb".isValidBitcoinDestinationAddress())
+    }
+
+    @Test
+    fun `isValidBitcoinDestinationAddress should reject a Tron address`() {
+        assertFalse("TR7NHqjeKQxGTCi8q8ZY4pL8otSzgjLj6t".isValidBitcoinDestinationAddress())
+    }
+
+    @Test
+    fun `P2SH destination should produce OP_HASH160 push-20 OP_EQUAL scriptPubKey`() {
+        val destination = okxWithdrawalAddress.decodeBitcoinDestination()
+
+        assertEquals("a914${okxWithdrawalHash160}87", destination.toScriptPubKey().toHexString(withPrefix = false))
+    }
+
+    @Test
+    fun `P2PKH destination should produce OP_DUP OP_HASH160 push-20 OP_EQUALVERIFY OP_CHECKSIG scriptPubKey`() {
+        val destination = genesisP2pkhAddress.decodeBitcoinDestination()
+
+        assertEquals("76a914${genesisP2pkhHash160}88ac", destination.toScriptPubKey().toHexString(withPrefix = false))
+    }
+
+    @Test
+    fun `NativeSegwit destination should produce OP_0 push-20 scriptPubKey`() {
+        val destination = nativeSegwitAddress.decodeBitcoinDestination()
+
+        assertEquals("0014$nativeSegwitAccountId", destination.toScriptPubKey().toHexString(withPrefix = false))
+    }
+}
@@ -178,13 +178,9 @@ class AddressInputMixinProvider(
            systemCallExecutor.executeSystemCall(ScanQrCodeCall()).mapCatching {
                val spec = specProvider.spec.first()

-                android.util.Log.e("QrDiag", "raw QR content: $it")
-
                qrSharingFactory.create(spec::isValidAddress).decode(it).address
            }.onSuccess { address ->
                inputFlow.value = address
-            }.onFailure {
-                android.util.Log.e("QrDiag", "decode failed", it)
            }.onSystemCallFailure {
                errorDisplayer(resourceManager.getString(R.string.invoice_scan_error_no_info))
            }
@@ -5,7 +5,6 @@ import io.novafoundation.nova.feature_account_api.data.extrinsic.ExtrinsicSubmis
 import io.novafoundation.nova.feature_account_api.data.model.Fee
 import io.novafoundation.nova.feature_wallet_api.data.network.blockhain.assets.tranfers.TransactionExecution
 import io.novafoundation.nova.runtime.multiNetwork.chain.model.Chain
-import io.novasama.substrate_sdk_android.runtime.AccountId
 import java.math.BigInteger

 /**
@@ -13,15 +12,21 @@ import java.math.BigInteger
 * shape (calculateFee/transact/transactAndAwaitExecution over a sending origin), but for Bitcoin. See
 * `RealBitcoinTransactionService` for the UTXO-model construction/signing details, which differ substantially
 * from Tron's account-model approach.
+ *
+ * [recipientAddress] is the raw destination address string, not an [io.novasama.substrate_sdk_android.runtime.AccountId] -
+ * unlike every other chain's transfer path, a Bitcoin destination can legitimately be a P2SH/P2PKH address (a
+ * real exchange withdrawal address was confirmed to be P2SH-only, with no way to request native SegWit instead),
+ * which needs its own distinct scriptPubKey shape. Converting to a generic 20-byte AccountId this early would
+ * discard which shape it needs to be - see `BitcoinDestinationAddress.kt`.
 */
 interface BitcoinTransactionService {

-    suspend fun calculateFee(chain: Chain, origin: TransactionOrigin, recipient: AccountId, amountSat: BigInteger): Fee
+    suspend fun calculateFee(chain: Chain, origin: TransactionOrigin, recipientAddress: String, amountSat: BigInteger): Fee

    suspend fun transact(
        chain: Chain,
        origin: TransactionOrigin,
-        recipient: AccountId,
+        recipientAddress: String,
        presetFee: Fee?,
        amountSat: BigInteger
    ): Result<ExtrinsicSubmission>
@@ -29,7 +34,7 @@ interface BitcoinTransactionService {
    suspend fun transactAndAwaitExecution(
        chain: Chain,
        origin: TransactionOrigin,
-        recipient: AccountId,
+        recipientAddress: String,
        presetFee: Fee?,
        amountSat: BigInteger
    ): Result<TransactionExecution>
@@ -4,9 +4,11 @@ import io.novafoundation.nova.common.utils.BitcoinInput
 import io.novafoundation.nova.common.utils.BitcoinOutput
 import io.novafoundation.nova.common.utils.BitcoinTransaction
 import io.novafoundation.nova.common.utils.DerSignature
+import io.novafoundation.nova.common.utils.decodeBitcoinDestination
 import io.novafoundation.nova.common.utils.toBitcoinAddress
 import io.novafoundation.nova.common.utils.toEcdsaSignatureData
 import io.novafoundation.nova.common.utils.toP2wpkhScriptPubKey
+import io.novafoundation.nova.common.utils.toScriptPubKey
 import io.novafoundation.nova.feature_account_api.data.ethereum.transaction.TransactionOrigin
 import io.novafoundation.nova.feature_account_api.data.extrinsic.ExtrinsicSubmission
 import io.novafoundation.nova.feature_account_api.data.extrinsic.SubmissionOrigin
@@ -43,10 +45,12 @@ private data class UtxoSelection(
 )

 /**
- * Builds, signs and broadcasts native SegWit (P2WPKH) Bitcoin transactions - UTXO selection and raw construction
- * happen entirely client-side (no server-assisted "createtransaction" the way TronGrid offers - mempool.space
- * only exposes UTXOs/fee-rate/broadcast, not transaction construction), using the hand-rolled protocol
- * primitives in [BitcoinTransaction]/[DerSignature]/`BitcoinAddress.kt` verified against BIP143/BIP173.
+ * Builds, signs and broadcasts Bitcoin transactions from this wallet's own native SegWit (P2WPKH) address - the
+ * recipient output, however, can be P2WPKH, P2SH or P2PKH (see `BitcoinDestinationAddress.kt`; a real exchange
+ * withdrawal address was confirmed live to be P2SH-only). UTXO selection and raw construction happen entirely
+ * client-side (no server-assisted "createtransaction" the way TronGrid offers - mempool.space only exposes
+ * UTXOs/fee-rate/broadcast, not transaction construction), using the hand-rolled protocol primitives in
+ * [BitcoinTransaction]/[DerSignature]/`BitcoinAddress.kt` verified against BIP143/BIP173.
 *
 * ## UTXO selection
 * Greedy largest-first over CONFIRMED UTXOs only (unconfirmed outputs are skipped - spending them risks the
@@ -74,7 +78,7 @@ class RealBitcoinTransactionService(
    private val bitcoinApi: BitcoinApi,
 ) : BitcoinTransactionService {

-    override suspend fun calculateFee(chain: Chain, origin: TransactionOrigin, recipient: AccountId, amountSat: BigInteger): Fee {
+    override suspend fun calculateFee(chain: Chain, origin: TransactionOrigin, recipientAddress: String, amountSat: BigInteger): Fee {
        val submittingMetaAccount = accountRepository.requireMetaAccountFor(origin, chain.id)
        val ownerAccountId = submittingMetaAccount.requireAccountIdIn(chain)
        val baseUrl = chain.requireMempoolSpaceBaseUrl()
@@ -91,7 +95,7 @@ class RealBitcoinTransactionService(
    override suspend fun transact(
        chain: Chain,
        origin: TransactionOrigin,
-        recipient: AccountId,
+        recipientAddress: String,
        presetFee: Fee?,
        amountSat: BigInteger
    ): Result<ExtrinsicSubmission> = runCatching {
@@ -102,6 +106,7 @@ class RealBitcoinTransactionService(
        }
        val baseUrl = chain.requireMempoolSpaceBaseUrl()
        val amountSatLong = amountSat.toLong()
+        val recipientScriptPubKey = recipientAddress.decodeBitcoinDestination().toScriptPubKey()

        val utxos = confirmedUtxos(baseUrl, ownerAccountId)
        val feeRate = bitcoinApi.fetchRecommendedFeeRateSatPerVbyte(baseUrl)
@@ -113,7 +118,7 @@ class RealBitcoinTransactionService(
        }

        val outputs = buildList {
-            add(BitcoinOutput(valueSat = amountSatLong, scriptPubKey = recipient.toP2wpkhScriptPubKey()))
+            add(BitcoinOutput(valueSat = amountSatLong, scriptPubKey = recipientScriptPubKey))
            if (selection.changeSat > 0) {
                add(BitcoinOutput(valueSat = selection.changeSat, scriptPubKey = ownerAccountId.toP2wpkhScriptPubKey()))
            }
@@ -143,13 +148,13 @@ class RealBitcoinTransactionService(
    override suspend fun transactAndAwaitExecution(
        chain: Chain,
        origin: TransactionOrigin,
-        recipient: AccountId,
+        recipientAddress: String,
        presetFee: Fee?,
        amountSat: BigInteger
    ): Result<TransactionExecution> {
        // Broadcast acceptance is already a strong signal (same posture as Tron/EVM) - this sits outside the
        // primary send flow's critical path, which only ever calls transact().
-        return transact(chain, origin, recipient, presetFee, amountSat).map { TransactionExecution.Bitcoin(it.hash) }
+        return transact(chain, origin, recipientAddress, presetFee, amountSat).map { TransactionExecution.Bitcoin(it.hash) }
    }

    private suspend fun confirmedUtxos(baseUrl: String, ownerAccountId: AccountId): List<BitcoinUtxo> {
@@ -19,7 +19,6 @@ import io.novafoundation.nova.feature_wallet_impl.data.network.blockchain.assets
 import io.novafoundation.nova.feature_wallet_impl.data.network.blockchain.assets.transfers.validations.sufficientTransferableBalanceToPayOriginFee
 import io.novafoundation.nova.feature_wallet_impl.data.network.blockchain.assets.transfers.validations.validAddress
 import io.novafoundation.nova.feature_wallet_impl.domain.validaiton.recipientCanAcceptTransfer
-import io.novafoundation.nova.runtime.ext.accountIdOrDefault
 import io.novafoundation.nova.runtime.multiNetwork.chain.model.Chain
 import io.novasama.substrate_sdk_android.runtime.definitions.types.generics.GenericCall
 import kotlinx.coroutines.CoroutineScope
@@ -52,7 +51,7 @@ class BitcoinNativeAssetTransfers(
        return bitcoinTransactionService.calculateFee(
            chain = transfer.originChain,
            origin = transfer.sender.intoOrigin(),
-            recipient = transfer.originChain.accountIdOrDefault(transfer.recipient),
+            recipientAddress = transfer.recipient,
            amountSat = transfer.amountInPlanks
        )
    }
@@ -61,7 +60,7 @@ class BitcoinNativeAssetTransfers(
        return bitcoinTransactionService.transact(
            chain = transfer.originChain,
            origin = transfer.sender.intoOrigin(),
-            recipient = transfer.originChain.accountIdOrDefault(transfer.recipient),
+            recipientAddress = transfer.recipient,
            presetFee = transfer.fee.submissionFee,
            amountSat = transfer.amountInPlanks
        )
@@ -71,7 +70,7 @@ class BitcoinNativeAssetTransfers(
        return bitcoinTransactionService.transactAndAwaitExecution(
            chain = transfer.originChain,
            origin = transfer.sender.intoOrigin(),
-            recipient = transfer.originChain.accountIdOrDefault(transfer.recipient),
+            recipientAddress = transfer.recipient,
            presetFee = transfer.fee.submissionFee,
            amountSat = transfer.amountInPlanks
        )
@@ -16,7 +16,7 @@ import io.novafoundation.nova.common.utils.emptyEthereumAccountId
 import io.novafoundation.nova.common.utils.emptySubstrateAccountId
 import io.novafoundation.nova.common.utils.findIsInstanceOrNull
 import io.novafoundation.nova.common.utils.formatNamed
-import io.novafoundation.nova.common.utils.isValidBitcoinAddress
+import io.novafoundation.nova.common.utils.isValidBitcoinDestinationAddress
 import io.novafoundation.nova.common.utils.removeHexPrefix
 import io.novafoundation.nova.common.utils.emptyTronAccountId
 import io.novafoundation.nova.common.utils.isValidTronAddress
@@ -372,7 +372,10 @@ fun Chain.multiAddressOf(accountId: ByteArray): MultiAddress {
 fun Chain.isValidAddress(address: String): Boolean {
    return runCatching {
        when {
-            isBitcoinBased -> address.isValidBitcoinAddress()
+            // Wider than isValidBitcoinAddress() (native SegWit only, used for this wallet's OWN address/accountId):
+            // a valid SEND destination can legitimately be P2SH/P2PKH too - confirmed live via a real exchange
+            // withdrawal address - see BitcoinDestinationAddress.kt.
+            isBitcoinBased -> address.isValidBitcoinDestinationAddress()

            // Tron addresses are Base58Check(0x41 ++ accountId), not SS58 or plain 0x-hex - neither of the two
            // branches below would ever accept them, so this needs its own dedicated check.