wallet: Disallow abandon of conflicted txes #12282

MarcoFalke commented at 10:13 PM on January 27, 2018: member

Abandon transactions that are already conflicted is a noop, so don't try and return false/throw instead.

MarcoFalke added the label Wallet on Jan 27, 2018

in src/wallet/wallet.cpp:1117 in 99991a5a92 outdated

1109 | @@ -1110,9 +1110,9 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
1110 |          CWalletTx& wtx = it->second;
1111 |          int currentconfirm = wtx.GetDepthInMainChain();
1112 |          // If the orig tx was not in block, none of its spends can be
1113 | -        assert(currentconfirm <= 0);

luke-jr commented at 10:57 PM on January 27, 2018:

Its spends can very well be conflicted independently from orig tx being conflicted or not, no?

MarcoFalke commented at 11:14 PM on January 27, 2018:

Thanks, fixed.

MarcoFalke force-pushed on Jan 27, 2018

luke-jr commented at 11:47 PM on January 27, 2018: member

utACK.

fanquake commented at 12:52 PM on January 29, 2018: member

Needs rebase

MarcoFalke force-pushed on Jan 29, 2018

MarcoFalke commented at 8:43 PM on January 29, 2018: member

Rebased

in test/functional/wallet_bumpfee.py:239 in fa9e533924 outdated

 235 | @@ -236,8 +236,8 @@ def test_unconfirmed_not_spendable(rbf_node, rbf_node_address):
 236 |      # mempool. this makes it possible to check whether the rbf tx outputs are
 237 |      # spendable before the rbf tx is confirmed.
 238 |      block = submit_block_with_tx(rbf_node, rbftx)
 239 | -    rbf_node.abandontransaction(bumpid)

ryanofsky commented at 9:00 PM on January 29, 2018:

comment above is now out of date

MarcoFalke commented at 9:21 PM on January 29, 2018:

Thanks, going to fix...

ryanofsky commented at 9:10 PM on January 29, 2018: member

Bumpfee part of this change seems good, but I don't understand the motivation behind the setabandoned change, and also think maybe it be moved to a separate PR if it is going to be done. PR description claims reason for not allowing abandoning conflicted txs is that "Abandon transactions that are already conflicted is a noop," but this isn't true, and obviously not given the bumpfee test change. Does this have something to do with https://botbot.me/freenode/bitcoin-core-dev/msg/96238851/?

MarcoFalke commented at 9:19 PM on January 29, 2018: member

@ryanofsky I don't know if this has anything to do with the bug report, as it misses some details. It could be related, though.

However, to the best of my knowledge the abandon is indeed a noop on conflicted transactions. See the comment

// if (currentconfirm < 0) {Tx and spends are already conflicted, no need to abandon}

which is effectively code-wise: if (currentconfirm < 0) continue;

The test change was necessary because instead of making it a silent noop, we return false. (I.e. throw an exception on the rpc interface)

On another note, the line in the test did nothing, so I might as well remove it altogether. However, it seems safer to me to abandon the tx instead of noop to reduce the risk of races.

MarcoFalke force-pushed on Jan 29, 2018

in test/functional/wallet_bumpfee.py:235 in fa06ceaf8e outdated

 230 | @@ -231,13 +231,14 @@ def test_unconfirmed_not_spendable(rbf_node, rbf_node_address):
 231 |      assert_equal([t for t in rbf_node.listunspent(minconf=0, include_unsafe=False) if t["txid"] == bumpid], [])
 232 |  
 233 |      # submit a block with the rbf tx to clear the bump tx out of the mempool,
 234 | -    # then call abandon to make sure the wallet doesn't attempt to resubmit the
 235 | -    # bump tx, then invalidate the block so the rbf tx will be put back in the
 236 | -    # mempool. this makes it possible to check whether the rbf tx outputs are
 237 | +    # then invalidate the block so the rbf tx will be put back in the mempool.
 238 | +    # Also, call abandon to make sure the wallet doesn't attempt to resubmit

ryanofsky commented at 9:37 PM on January 29, 2018:

Isn't this comment misleading if the call to abandon doesn't actually do anything? Seems better to remove the comment and call entirely and maybe replace with a comment that says "this test assumes the wallet will not reinsert the bump transaction after the call to invalidateblock."

ryanofsky commented at 9:49 PM on January 29, 2018: member

Thanks for the explanation.

I'd still suggest splitting abandon and bumpfee changes into separate PRs, because implementations are confusing, and it's possible we might want one change and not the other.
Would also suggest updating abandontransaction RPC documentation to say "it is an error to abandon a transaction that is conflicted" instead of "it has no effect on transactions which are already conflicted or abandoned."

MarcoFalke force-pushed on Jan 29, 2018

MarcoFalke renamed this:
~~wallet: Bump and Abandon only on non-conflicted txes~~
wallet: Abandon only non-conflicted txes
on Jan 29, 2018

MarcoFalke renamed this:
~~wallet: Abandon only non-conflicted txes~~
wallet: Disallow abandon of conflicted txes
on Jan 29, 2018

MarcoFalke force-pushed on Jan 29, 2018

in test/functional/wallet_abandonconflict.py:32 in fa120eb7f7 outdated

  28 | @@ -28,6 +29,11 @@ def run_test(self):
  29 |          sync_mempools(self.nodes)
  30 |          self.nodes[1].generate(1)
  31 |  
  32 | +        # Can not abanond non-wallet transaction

promag commented at 12:03 AM on January 30, 2018:

Typo.

in test/functional/wallet_abandonconflict.py:33 in fa120eb7f7 outdated

  28 | @@ -28,6 +29,11 @@ def run_test(self):
  29 |          sync_mempools(self.nodes)
  30 |          self.nodes[1].generate(1)
  31 |  
  32 | +        # Can not abanond non-wallet transaction
  33 | +        assert_raises_rpc_error(-5, 'Invalid or non-wallet transaction id', lambda: self.nodes[0].abandontransaction(txid='ff' * 32))

promag commented at 12:03 AM on January 30, 2018:

Another commit since this covers already existing functionality?

promag commented at 12:06 AM on January 30, 2018: member

utACK but some comments

Needs release note?

ryanofsky commented at 10:52 PM on January 30, 2018: member

utACK fa120eb7f7703b14babdf459418ebfb81ed5a7f2. Thanks for addressing my comments. Agree a release note for the RPC could be nice (and would make it clearer for reviewers why the tests are changing).

MarcoFalke commented at 11:10 PM on January 30, 2018: member

Will do the release notes in a separate pull, as it is not clear what is the first release that includes this fix.

promag commented at 11:21 PM on January 30, 2018: member

Will do the release notes in a separate pull

Maybe add tag for now?

MarcoFalke added the label Needs release notes on Jan 30, 2018

laanwj commented at 2:45 PM on January 31, 2018: member

Concept ACK.

laanwj commented at 1:06 PM on February 6, 2018: member

utACK (though would be nice to fix the comment typos that @promag mentions)

laanwj assigned laanwj on Feb 6, 2018

MarcoFalke force-pushed on Feb 6, 2018

wallet: Disallow abandon of conflicted txes fa795cf9c5

MarcoFalke force-pushed on Feb 6, 2018

MarcoFalke commented at 2:55 PM on February 6, 2018: member

Fixed typo. Should be trivial to re-review if you still have my old commit locally.

promag commented at 3:16 PM on February 6, 2018: member

utACK fa795cf. New tests LGTM.

laanwj merged this on Feb 8, 2018

laanwj closed this on Feb 8, 2018

laanwj referenced this in commit 663911ed58 on Feb 8, 2018

MarcoFalke deleted the branch on Feb 8, 2018

fanquake removed the label Needs release note on Mar 20, 2019

PastaPastaPasta referenced this in commit 765e5dba07 on Jun 13, 2020

PastaPastaPasta referenced this in commit 4e983cc72c on Jun 13, 2020

PastaPastaPasta referenced this in commit 026f75830e on Jun 14, 2020

PastaPastaPasta referenced this in commit 7c657b8771 on Jun 17, 2020

PastaPastaPasta referenced this in commit 7be921801e on Jun 17, 2020

PastaPastaPasta referenced this in commit fdf2c556eb on Jun 17, 2020

PastaPastaPasta referenced this in commit 4025a5b9af on Jun 17, 2020

DrahtBot locked this on Dec 16, 2021