horizontalsystems / unstoppable-wallet-android

A powerful non-custodial multi-wallet for Bitcoin, Ethereum, Binance Smart Chain, Avalanche, Solana and other blockchains. Non-custodial crypto and NFT storage, onchain decentralized exchange, institutional grade analytics for cryptcurrency and NFT markets, extensive privacy controls and human oriented design. Implemented on Kotlin.
https://unstoppable.money
MIT License
829 stars 356 forks source link

Issues trying to use P2SH Addresses #2373

Closed Cyrois closed 4 years ago

Cyrois commented 4 years ago

Right now we have BECH32 and P2PKH addresses working, we are trying to add support for P2SH.

We generate the address using the addressConverter: addressConverter.convert(publicKey.publicKeyHash, ScriptType.P2SH).string

We are able to receive money but when we try to spend it, we get the error. The error us caused by the transactionOutput redeemScript not being set. Here are the error logs:

    java.lang.IllegalArgumentException: Found irregular outputs
        at io.horizontalsystems.bitcoincore.transactions.TransactionProcessor.processOutgoing(TransactionProcessor.kt:41)
        at io.horizontalsystems.bitcoincore.transactions.TransactionCreator.create(TransactionCreator.kt:34)
        at io.horizontalsystems.bitcoincore.transactions.TransactionCreator.create(TransactionCreator.kt:17)
        at io.horizontalsystems.bitcoincore.BitcoinCore.send(BitcoinCore.kt:402)
        at io.horizontalsystems.bitcoincore.AbstractKit.send(AbstractKit.kt:55)
        at io.horizontalsystems.bitcoincore.AbstractKit.send$default(AbstractKit.kt:54)
        at com.skyglobal.skywalletcore.wallet.WalletBitcoinManager.sendTransaction(WalletBitcoinManager.kt:101)
        at com.skyglobal.skywalletcore.utils.CryptoRequest.sendTransaction(CryptoRequest.kt:105)
        at com.skyglobal.cryptowallet.ui.fragments.ConfirmTxnSentFragment.onViewCreated(ConfirmTxnSentFragment.kt:40)
        at androidx.fragment.app.FragmentManagerImpl.moveToState(FragmentManagerImpl.java:892)
        at androidx.fragment.app.FragmentManagerImpl.addAddedFragments(FragmentManagerImpl.java:2100)
        at androidx.fragment.app.FragmentManagerImpl.executeOpsTogether(FragmentManagerImpl.java:1874)
        at 

So to fix this, I am wondering why the redeemScript is empty. I am looking for the place that writes the value of the redeemScript. Which seems to be the HolderPlugin class. This class is called from the TransactionExtractor.

Right now I am confused about the following code in the TransactionExtractor:

fun extractOutputs(transaction: FullTransaction) {
    var nullDataOutput : TransactionOutput? = null
    for (output in transaction.outputs) {
        val payload: ByteArray
        val scriptType: ScriptType

        val lockingScript = output.lockingScript

        if (isP2PKH(lockingScript)) {
            payload = lockingScript.copyOfRange(3, 23)
            scriptType = ScriptType.P2PKH
...
        } else if (isP2SH(lockingScript)) {
            payload = lockingScript.copyOfRange(2, lockingScript.size - 1)
            scriptType = ScriptType.P2SH
        }
...
        } else if (isNullData(lockingScript)) {
            payload = lockingScript
            scriptType = ScriptType.NULL_DATA
            nullDataOutput = output
        } else continue

        output.scriptType = scriptType
        output.keyHash = payload

        // Set public key if exist
        getPublicKey(output)?.let {
            transaction.header.isMine = true
            output.setPublicKey(it)
        }
    }

    nullDataOutput?.let {
        pluginManager.processTransactionWithNullData(transaction, it)
    }

The processTransactionWithNullData method will extract the redeemScript. However this method only gets trigged if the lockingScript is null, which for P2SH is NOT the case.

Am I missing something here? Any ideas are appreciated. 🙏

Cyrois commented 4 years ago

The error while spending is due to the Input Serializer:

Caused by: java.lang.Exception: no previous output script
        at io.horizontalsystems.bitcoincore.serializers.InputSerializer.serializeForSignature(InputSerializer.kt:47)
        at io.horizontalsystems.bitcoincore.serializers.TransactionSerializer.serializeForSignature(TransactionSerializer.kt:124)
        at io.horizontalsystems.bitcoincore.transactions.builder.InputSigner.sigScriptData(InputSigner.kt:23)
        at io.horizontalsystems.bitcoincore.transactions.builder.TransactionSigner.sign(TransactionSigner.kt:15)
        at io.horizontalsystems.bitcoincore.transactions.builder.TransactionBuilder.buildTransaction(TransactionBuilder.kt:25)
        at io.horizontalsystems.bitcoincore.transactions.TransactionCreator$create$1.invoke(TransactionCreator.kt:18)
        at io.horizontalsystems.bitcoincore.transactions.TransactionCreator$create$1.invoke(TransactionCreator.kt:10)
        at io.horizontalsystems.bitcoincore.transactions.TransactionCreator.create(TransactionCreator.kt:32)
        at io.horizontalsystems.bitcoincore.transactions.TransactionCreator.create(TransactionCreator.kt:17)
        at io.horizontalsystems.bitcoincore.BitcoinCore.send(BitcoinCore.kt:402)
        at io.horizontalsystems.bitcoincore.AbstractKit.send(AbstractKit.kt:55)
        at io.horizontalsystems.bitcoincore.AbstractKit.send$default(AbstractKit.kt:54)
        at com.skyglobal.skywalletcore.wallet.WalletBitcoinManager.sendTransaction(WalletBitcoinManager.kt:97)
        at com.skyglobal.skywalletcore.utils.CryptoRequest.sendTransaction(CryptoRequest.kt:134)
        at com.skyglobal.cryptowallet.ui.fragments.ConfirmTxnSentFragment.onViewCreated(ConfirmTxnSentFragment.kt:40)

The question is, why is the redeemScript empty?

Cyrois commented 4 years ago

I have tried to manually extract the scripthash like so:

 } else if (isP2SH(lockingScript)) {
                payload = lockingScript.copyOfRange(2, lockingScript.size - 1)
                scriptType = ScriptType.P2SH

                val script = Script(output.lockingScript)
                val scriptChucks = script.chunks
                val pubkeyHash = checkNotNull(scriptChucks[1].data)

                val redeemScript = byteArrayOf(OP_CHECKSEQUENCEVERIFY.toByte(), OP_DROP.toByte()) + OpCodes.p2pkhStart + OpCodes.push(pubkeyHash) + OpCodes.p2pkhEnd
                storage.getPublicKeyByKeyOrKeyHash(pubkeyHash)?.let { pubkey ->
                    output.redeemScript = redeemScript
                    output.setPublicKey(pubkey)
                    transaction.header.isMine = true
                }
            } else if (isP2WPKH(lockingScript)) {

Now I have removed the Exception but I get the following error when I try to broadcast the Transaction:

{"jsonrpc": "2.0", "error": {"code": 1, "message": "the transaction was rejected by network rules.\n\nmandatory-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element) (code 16)\n[0200000001ab9191e19421ac70e44ae55940a392ab4e1e67419b99ba2010e3aef36a9aa94b010000008647304402200f8c1ca033d9a7457c33c135fd1333a4a3cc6d80b8194652b03bbe1c9371ea0402201f3ef503a31525009e2ab3424e39c67b0961de27e2b6a6e326dd09e5b08af92e012103a096239bb5473d788fdbc7f5fcd084caec4879e12c3e1cbc9365ac37a81aa6e11bb27576a914fbf7de49549cd17a1694d77ce52b9b629f28dacb88acfeffffff01715c0000000000001600147d4231bc0fc3581dfa53d33ff4528d7ebc40858cce111b00]"}
abdrasulov commented 4 years ago

To spend P2SH output you need to provide the redeem script. As for P2WPKHSH output, bitcoin kit can parse it since the redeem script in this case is standardized. You can check bips 49, 84, 141 for the details.

Cyrois commented 4 years ago

Hi @abdrasulov, I looked into BIP49, since that is what we are looking for in this case. Looks like BIP49 uses a different derivation path so I switched the BIP that we are passing into the BitcoinKit object to BIP49 and removed all my changes.

Trying to spend the p2sh output resulted in the same error as before, missing the previous output script.

Caused by: java.lang.Exception: no previous output script

I added my changes in, and got the same error while broadcasting the transaction.

abdrasulov commented 4 years ago

Hi @Cyrois,

It seems that the receive address wasn't generated properly. Could you check it with a new receive address generated using BIP49?

Cyrois commented 4 years ago

hi @abdrasulov, I have tried switching to BIP49.

I changed the default bip being passed into BitcoinKit. We do not overwrite this value.

constructor(
        context: Context,
        seed: ByteArray,
        walletId: String,
        ...
        confirmationsThreshold: Int = 6,
        bip: Bip = Bip.BIP49
    ) {

While debugging the code, I can see that the derivation path is indeed using BIP49.

    m/49'/1'/0'/0/0  <--external
    m/49'/1'/0'/1/14  <--internal

We are using a 24 word seed with a passphrase. The words and passphrase were (this is Testnet):

words -> accident alcohol pride awake recall lazy another myself clump regret beach phone super bean skirt crack section knock throw tell allow outside kind legal

passphrase -> "PASSPHRASE"

I set a breakpoint on the privateKey method in HDWallet that shows that i have the derivation path of m/49'/1'/0'/0/14 and I got 2N89sktKs3bBTnHfaFj2uiFik2QeuaKvjnv as my P2SH address.

I wonder if it's the fact that we changed our private key, but that does not make sense since it's just passed in as a byteArray into HDWallet...

abdrasulov commented 4 years ago

Hi @Cyrois,

We don't support custom passphrase as of now. So I checked both receiving and sending BTC on testnet using your mnemonic words without passphrase. The first receive address is 2N9QeNaNXYeFARnTD8uAYr2WneLhTMb8d8F. And the second one is 2MwRAmZoi7Eve16Vc3YYbEPKthtX3QPr5y3. I could successfully receive and spend BTC without any issues. You can see here https://blockstream.info/testnet/address/2N9QeNaNXYeFARnTD8uAYr2WneLhTMb8d8F. I've received coin via testnet faucet to the first address. Then I've sent it to the second address.

I don't think that the issue is related to the custom passphrase you've used. At the end, the system uses seed (byte array) for master key that is generated with mnemonic words and passphrase. Where passphare can be empty string.

I'd recommend to check the code where P2SH output is saved in local database as P2WPKHSH output. It is the method TransactionExtractor::getPublicKey(). As for the other questions, check the bitcoin documentation at https://developer.bitcoin.org/ and also bips I've mentioned before.

Since the issue is not reproducable in bitcoin-kit, I am closing it.