Skip to content

Commit

Permalink
wallet: (fix) bump_fee sometimes created invalid tx that spent orig out
Browse files Browse the repository at this point in the history
When replacing non-segwit tx, bump_fee in some circumstances created
a tx that tried to spend from the tx-to-be-replaced. There is
explicit logic to avoid this but it only worked for segwit txs.

The change in transaction.py is a no-op, just tried to make it clearer
that the scriptSigs, witnesses are being reset by from_tx().
  • Loading branch information
SomberNight committed Dec 18, 2020
1 parent 44e6bfb commit bb41ef3
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 2 deletions.
47 changes: 47 additions & 0 deletions electrum/tests/test_wallet_vertical.py
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,10 @@ def test_rbf(self, mock_save_db):
self._bump_fee_p2wpkh_when_there_is_a_change_address(
simulate_moving_txs=simulate_moving_txs,
config=config)
with self.subTest(msg="_bump_fee_p2pkh_when_there_are_two_ismine_outs_one_change_one_recv", simulate_moving_txs=simulate_moving_txs):
self._bump_fee_p2pkh_when_there_are_two_ismine_outs_one_change_one_recv(
simulate_moving_txs=simulate_moving_txs,
config=config)
with self.subTest(msg="_bump_fee_when_user_sends_max", simulate_moving_txs=simulate_moving_txs):
self._bump_fee_when_user_sends_max(
simulate_moving_txs=simulate_moving_txs,
Expand Down Expand Up @@ -990,6 +994,49 @@ def _bump_fee_p2pkh_when_there_is_a_change_address(self, *, simulate_moving_txs,
wallet.receive_tx_callback(tx.txid(), tx, TX_HEIGHT_UNCONFIRMED)
self.assertEqual((0, 7484320, 0), wallet.get_balance())

def _bump_fee_p2pkh_when_there_are_two_ismine_outs_one_change_one_recv(self, *, simulate_moving_txs, config):
"""This tests a regression where sometimes we created a replacement tx
that spent from the original (which is clearly invalid).
"""
wallet = self.create_standard_wallet_from_seed('amazing vapor slab rib chat cousin east float plug baby session weird',
config=config)

# bootstrap wallet
funding_tx = Transaction('02000000000101a3a9d94039c1051102e36b835764b89985602608a3e121c91cb63d67277355080100000000fdffffff0220a10700000000001976a9143decc30f4f7eec45c5775347050b85a43ac7ee0b88ac203c3500000000001600149d91f0053172fab394d277ae27e9fa5c5a4921090247304402207a2b4abe2c4128fe80db297d636b81487feda2ee3c51a95bc670b7b377b09ca402205147bc550dfdff72e9159554c19045111daf6d95f556a4f4dc370c90aa37a3e0012102cccad56b36e7bd1ae44c37d69019d006d8911b43071725d6dcbbdfcade05650313f71c00')
funding_txid = funding_tx.txid()
self.assertEqual('0d98d8615f7b711beff2efcd4cf6b9f7ecd3b16a53fb9374e6a81d852492674e', funding_txid)
wallet.receive_tx_callback(funding_txid, funding_tx, TX_HEIGHT_UNCONFIRMED)

orig_rbf_tx = Transaction('02000000014e679224851da8e67493fb536ab1d3ecf7b9f64ccdeff2ef1b717b5f61d8980d000000006a4730440220361b332f0488501e0605b9a5385edda762e761c00f95195f308e2baea5e12f9d0220051be1c834f0de69ecf084b0311abf541687436cb34311a002efa4f104a722a3012103d4ce4ba5be0b861d2ee7c715b84ab0e791ccd36530bd8652babae37eda693c39fdffffff02bc020000000000001976a914093107975170d4416bd2dad961414ac0a5c9b3de88ac389d0700000000001976a914ac55156f62fa9085c114fc6496aee5ab153cb22888ac13f71c00')
orig_rbf_txid = orig_rbf_tx.txid()
self.assertEqual('2bce74c17a2b4c1f57b454604c87006173716e92028de60463182c344f3e2180', orig_rbf_txid)
wallet.receive_tx_callback(orig_rbf_txid, orig_rbf_tx, TX_HEIGHT_UNCONFIRMED)

# bump tx
tx = wallet.bump_fee(tx=tx_from_any(orig_rbf_tx.serialize()), new_fee_rate=200)
self.assertTrue(not any([txin for txin in tx.inputs() if txin.prevout.txid.hex() == orig_rbf_txid]))
tx.locktime = 1898260
tx.version = 2
if simulate_moving_txs:
partial_tx = tx.serialize_as_bytes().hex()
self.assertEqual("70736274ff01005502000000014e679224851da8e67493fb536ab1d3ecf7b9f64ccdeff2ef1b717b5f61d8980d0000000000fdffffff01d4ed0600000000001976a914ac55156f62fa9085c114fc6496aee5ab153cb22888ac14f71c00000100e102000000000101a3a9d94039c1051102e36b835764b89985602608a3e121c91cb63d67277355080100000000fdffffff0220a10700000000001976a9143decc30f4f7eec45c5775347050b85a43ac7ee0b88ac203c3500000000001600149d91f0053172fab394d277ae27e9fa5c5a4921090247304402207a2b4abe2c4128fe80db297d636b81487feda2ee3c51a95bc670b7b377b09ca402205147bc550dfdff72e9159554c19045111daf6d95f556a4f4dc370c90aa37a3e0012102cccad56b36e7bd1ae44c37d69019d006d8911b43071725d6dcbbdfcade05650313f71c00220603d4ce4ba5be0b861d2ee7c715b84ab0e791ccd36530bd8652babae37eda693c390c11aad9ae000000000000000000220203feceda5212994b3552847c93288c47490404784d90f1966b7d02e009ba40680e0c11aad9ae000000000100000000",
partial_tx)
tx = tx_from_any(partial_tx) # simulates moving partial txn between cosigners
self.assertFalse(tx.is_complete())

wallet.sign_transaction(tx, password=None)
self.assertTrue(tx.is_complete())
self.assertFalse(tx.is_segwit())
tx_copy = tx_from_any(tx.serialize())
self.assertEqual('02000000014e679224851da8e67493fb536ab1d3ecf7b9f64ccdeff2ef1b717b5f61d8980d000000006a473044022024ce838fb02d482ca33872197175a15fed66cf139ebcdb4cc840e227273a15f70220466306a037f2b9f9702b4a331672efcadde32a4df25dc4e28abd019b68eb2761012103d4ce4ba5be0b861d2ee7c715b84ab0e791ccd36530bd8652babae37eda693c39fdffffff01d4ed0600000000001976a914ac55156f62fa9085c114fc6496aee5ab153cb22888ac14f71c00',
str(tx_copy))
self.assertEqual('198d85f7a2ab342f1d98c6838ef1d68fb79d10cb1c1842a5b152d4fffefbe483', tx_copy.txid())
self.assertEqual('198d85f7a2ab342f1d98c6838ef1d68fb79d10cb1c1842a5b152d4fffefbe483', tx_copy.wtxid())

wallet.receive_tx_callback(tx.txid(), tx, TX_HEIGHT_UNCONFIRMED)
self.assertEqual((0, 454100, 0), wallet.get_balance())


@mock.patch.object(wallet.Abstract_Wallet, 'save_db')
def test_cpfp_p2pkh(self, mock_save_db):
wallet = self.create_standard_wallet_from_seed('fold object utility erase deputy output stadium feed stereo usage modify bean')
Expand Down
3 changes: 2 additions & 1 deletion electrum/transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -1558,7 +1558,8 @@ def to_json(self) -> dict:
@classmethod
def from_tx(cls, tx: Transaction) -> 'PartialTransaction':
res = cls()
res._inputs = [PartialTxInput.from_txin(txin) for txin in tx.inputs()]
res._inputs = [PartialTxInput.from_txin(txin, strip_witness=True)
for txin in tx.inputs()]
res._outputs = [PartialTxOutput.from_txout(txout) for txout in tx.outputs()]
res.version = tx.version
res.locktime = tx.locktime
Expand Down
4 changes: 3 additions & 1 deletion electrum/wallet.py
Original file line number Diff line number Diff line change
Expand Up @@ -1401,6 +1401,8 @@ def bump_fee(self, *, tx: Transaction, new_fee_rate: Union[int, float, Decimal],

def _bump_fee_through_coinchooser(self, *, tx: Transaction, new_fee_rate: Union[int, Decimal],
coins: Sequence[PartialTxInput] = None) -> PartialTransaction:
old_txid = tx.txid()
assert old_txid
tx = PartialTransaction.from_tx(tx)
tx.add_info_from_wallet(self)
old_inputs = list(tx.inputs())
Expand Down Expand Up @@ -1428,7 +1430,7 @@ def _bump_fee_through_coinchooser(self, *, tx: Transaction, new_fee_rate: Union[
if coins is None:
coins = self.get_spendable_coins(None)
# make sure we don't try to spend output from the tx-to-be-replaced:
coins = [c for c in coins if c.prevout.txid.hex() != tx.txid()]
coins = [c for c in coins if c.prevout.txid.hex() != old_txid]
for item in coins:
self.add_input_info(item)
def fee_estimator(size):
Expand Down

0 comments on commit bb41ef3

Please sign in to comment.