Add extra checks for RBF transactions (#2416)

* Add extra checks for RBF transactions

* Move to util function, don't allow CPFP of confirmed txs

core/src/main/scala/org/bitcoins/core/protocol/transaction/TxUtil.scala

@@ -24,6 +24,11 @@ import scala.util.{Failure, Success, Try}
 
 object TxUtil extends BitcoinSLogger {
 
+  def isRBFEnabled(transaction: Transaction): Boolean = {
+    transaction.inputs.exists(
+      _.sequence < TransactionConstants.disableRBFSequence)
+  }
+
   private def computeNextLockTime(
       currentLockTimeOpt: Option[UInt32],
       locktime: Long): Try[UInt32] = {

wallet-test/src/test/scala/org/bitcoins/wallet/WalletIntegrationTest.scala

@@ -198,6 +198,40 @@ class WalletIntegrationTest extends BitcoinSWalletTest {
     }
   }
 
+  it should "fail to RBF a confirmed transaction" in { walletWithBitcoind =>
+    val WalletWithBitcoindRpc(wallet, bitcoind) = walletWithBitcoind
+
+    for {
+      // Fund wallet
+      addr <- wallet.getNewAddress()
+      txId <- bitcoind.sendToAddress(addr, valueFromBitcoind)
+      _ <- bitcoind.getNewAddress.flatMap(bitcoind.generateToAddress(6, _))
+      rawTx <- bitcoind.getRawTransaction(txId)
+      _ <- wallet.processTransaction(rawTx.hex, rawTx.blockhash)
+
+      // Verify we funded the wallet
+      balance <- wallet.getBalance()
+      _ = assert(balance == valueFromBitcoind)
+
+      // Create rbf tx
+      rbf <- bitcoind.getNewAddress.flatMap {
+        wallet.sendToAddress(_, valueToBitcoind, SatoshisPerVirtualByte.one)
+      }
+      _ <- bitcoind.sendRawTransaction(rbf)
+
+      // Confirm transaction
+      _ <- bitcoind.getNewAddress.flatMap(bitcoind.generateToAddress(1, _))
+      rawTx1 <- bitcoind.getRawTransaction(rbf.txIdBE)
+      _ = require(rawTx1.blockhash.isDefined)
+      _ <- wallet.processTransaction(rbf, rawTx1.blockhash)
+
+      // fail to RBF confirmed tx
+      res <- recoverToSucceededIf[IllegalArgumentException] {
+        wallet.bumpFeeRBF(rbf.txIdBE, SatoshisPerVirtualByte.fromLong(20))
+      }
+    } yield res
+  }
+
   it should "correctly bump fees with CPFP" in { walletWithBitcoind =>
     val WalletWithBitcoindRpc(wallet, bitcoind) = walletWithBitcoind
 

wallet-test/src/test/scala/org/bitcoins/wallet/WalletSendingTest.scala

@@ -2,16 +2,22 @@ package org.bitcoins.wallet
 
 import org.bitcoins.core.api.wallet.{CoinSelectionAlgo, CoinSelector}
 import org.bitcoins.core.currency._
+import org.bitcoins.core.number.{Int32, UInt32}
 import org.bitcoins.core.protocol.BitcoinAddress
+import org.bitcoins.core.protocol.script.EmptyScriptSignature
 import org.bitcoins.core.protocol.transaction.{
+  BaseTransaction,
   EmptyTransaction,
+  TransactionConstants,
+  TransactionInput,
   TransactionOutput
 }
+import org.bitcoins.core.psbt.PSBT
 import org.bitcoins.core.script.constant.{BytesToPushOntoStack, ScriptConstant}
 import org.bitcoins.core.script.control.OP_RETURN
 import org.bitcoins.core.wallet.fee._
 import org.bitcoins.core.wallet.utxo.TxoState
-import org.bitcoins.crypto.CryptoUtil
+import org.bitcoins.crypto.{CryptoUtil, DoubleSha256DigestBE}
 import org.bitcoins.testkit.Implicits.GeneratorOps
 import org.bitcoins.testkit.core.gen.FeeUnitGen
 import org.bitcoins.testkit.wallet.BitcoinSWalletTest
@@ -288,6 +294,50 @@ class WalletSendingTest extends BitcoinSWalletTest {
     }
   }
 
+  it should "fail to RBF a confirmed transaction" in { fundedWallet =>
+    val wallet = fundedWallet.wallet
+
+    val feeRate = FeeUnitGen.satsPerByte.sampleSome
+    val newFeeRate = SatoshisPerByte(feeRate.currencyUnit + Satoshis.one)
+
+    for {
+      tx <- wallet.sendToAddress(testAddress, amountToSend, feeRate)
+      _ <- wallet.processTransaction(tx, Some(DoubleSha256DigestBE.empty))
+
+      res <- recoverToSucceededIf[IllegalArgumentException] {
+        wallet.bumpFeeRBF(tx.txIdBE, newFeeRate)
+      }
+    } yield res
+  }
+
+  it should "fail to RBF a non-signaling transaction" in { fundedWallet =>
+    val wallet = fundedWallet.wallet
+
+    for {
+      addr <- wallet.getNewAddress()
+      utxo <- wallet.listUtxos().map(_.head)
+
+      // Create tx not signaling RBF
+      input = TransactionInput(utxo.outPoint,
+                               EmptyScriptSignature,
+                               TransactionConstants.disableRBFSequence)
+      output =
+        TransactionOutput(utxo.output.value - Satoshis(500), addr.scriptPubKey)
+      tx =
+        BaseTransaction(Int32.two, Vector(input), Vector(output), UInt32.zero)
+      psbt = PSBT.fromUnsignedTx(tx)
+
+      // Have wallet sign and process transaction
+      signedPSBT <- wallet.signPSBT(psbt)
+      signedTx = signedPSBT.finalizePSBT.get.extractTransactionAndValidate.get
+      _ <- wallet.processTransaction(signedTx, None)
+
+      res <- recoverToSucceededIf[IllegalArgumentException] {
+        wallet.bumpFeeRBF(signedTx.txIdBE, SatoshisPerVirtualByte.fromLong(100))
+      }
+    } yield res
+  }
+
   it should "correctly CPFP a transaction" in { fundedWallet =>
     val wallet = fundedWallet.wallet
     for {
@@ -324,6 +374,21 @@ class WalletSendingTest extends BitcoinSWalletTest {
     }
   }
 
+  it should "fail to CPFP a confirmed transaction" in { fundedWallet =>
+    val wallet = fundedWallet.wallet
+
+    val feeRate = FeeUnitGen.satsPerByte.sampleSome
+
+    for {
+      tx <- wallet.sendToAddress(testAddress, amountToSend, feeRate)
+      _ <- wallet.processTransaction(tx, Some(DoubleSha256DigestBE.empty))
+
+      res <- recoverToSucceededIf[IllegalArgumentException] {
+        wallet.bumpFeeCPFP(tx.txIdBE, feeRate)
+      }
+    } yield res
+  }
+
   it should "fail to CPFP a transaction we don't own" in { fundedWallet =>
     val wallet = fundedWallet.wallet
 

wallet/src/main/scala/org/bitcoins/wallet/Wallet.scala

@@ -506,6 +506,8 @@ abstract class Wallet
             new RuntimeException(s"Unable to find transaction ${txId.hex}"))
       }
 
+      _ = require(TxUtil.isRBFEnabled(tx), "Transaction is not signaling RBF")
+
       outPoints = tx.inputs.map(_.previousOutput).toVector
       spks = tx.outputs.map(_.scriptPubKey).toVector
 
@@ -513,6 +515,13 @@ abstract class Wallet
       _ = require(utxos.nonEmpty, "Can only bump fee for our own transaction")
       _ = require(utxos.size == tx.inputs.size,
                   "Can only bump fee for a transaction we own all the inputs")
+
+      oldOutputs <- spendingInfoDAO.findDbsForTx(txId)
+      blockHashes = oldOutputs.flatMap(_.blockHash).distinct
+      _ = require(
+        blockHashes.isEmpty,
+        s"Cannot replace a confirmed transaction, ${blockHashes.map(_.hex)}")
+
       spendingInfos <- FutureUtil.sequentially(utxos) { utxo =>
         transactionDAO
           .findByOutPoint(utxo.outPoint)
@@ -558,9 +567,8 @@ abstract class Wallet
         }
 
       // Mark old outputs as replaced
-      oldUtxos <- spendingInfoDAO.findDbsForTx(txId)
       _ <- spendingInfoDAO.updateAll(
-        oldUtxos.map(_.copyWithState(TxoState.DoesNotExist)))
+        oldOutputs.map(_.copyWithState(TxoState.DoesNotExist)))
 
       sequence = tx.inputs.head.sequence + UInt32.one
       outputs = tx.outputs.filterNot(_.scriptPubKey == changeSpk)
@@ -721,6 +729,12 @@ abstract class Wallet
       _ = require(spendingInfos.nonEmpty,
                   s"Transaction ${txId.hex} must have an output we own")
 
+      oldOutputs <- spendingInfoDAO.findDbsForTx(txId)
+      blockHashes = oldOutputs.flatMap(_.blockHash).distinct
+      _ = require(
+        blockHashes.isEmpty,
+        s"No need to fee bump a confirmed transaction, ${blockHashes.map(_.hex)}")
+
       changeSpendingInfos = spendingInfos.flatMap { db =>
         if (db.privKeyPath.chain.chainType == HDChainType.Change) {
           Some(db)