wallet: Only fee-bump non-conflicted/non-confirmed txes #12296

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:Mf1801-walletFeeBumpNoConf changing 2 files +27 −20

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

This only affects the gui.

Fee-bumping of transactions that are already confirmed or are already conflicted by other transactions should not be offered by the gui.
move more bumpfee prechecks to feebumper::PreconditionChecks 718f05cab5
MarcoFalke added the label Wallet on Jan 29, 2018
MarcoFalke added the label GUI on Jan 29, 2018
MarcoFalke renamed this:
~~wallet: Only fee-bump and non-conflicted/non-confirmed txes~~
wallet: Only fee-bump non-conflicted/non-confirmed txes
on Jan 29, 2018
promag commented at 10:11 PM on January 29, 2018: member

ACK fa4b00f.
MarcoFalke force-pushed on Jan 29, 2018
MarcoFalke commented at 10:13 PM on January 29, 2018: member

Sorry, fixed up typo in commit message.
promag commented at 10:17 PM on January 29, 2018: member

re-ACK fa2457e.

It can be a bit heavy, but the check wallet->IsAllFromMe() makes a lot of sense, there is no way to bump received transactions.
instagibbs commented at 10:24 PM on January 29, 2018: member

Food for thought: Move more checks into PreconditionChecks and use that instead: https://github.com/instagibbs/bitcoin/commit/718f05cab5fe632c5dc4e3c689d5e4cd51331089

This makes reasoning about what is supposed to be called easier in the future.
promag commented at 11:04 PM on January 29, 2018: member

Restarted travis job due to AssertionError: Mempool sync failed in wallet_backup.py.
MarcoFalke force-pushed on Jan 29, 2018
MarcoFalke commented at 11:23 PM on January 29, 2018: member

@instagibbs Thx. Your commit seems like a proper fix.
instagibbs commented at 11:26 PM on January 29, 2018: member

https://github.com/bitcoin/bitcoin/pull/12296/commits/fabefab1e202cac84ac0b2ba837672feb2662119 utACK. We can debate what checks to roll into that later.
feebumper: Use PreconditionChecks to determine bump eligibility faca18dcf4
MarcoFalke force-pushed on Jan 29, 2018
promag commented at 11:38 PM on January 29, 2018: member

This should be tested with a big wallet now that IsMine is called when the context menu opens.

in src/wallet/feebumper.cpp:237 in faca18dcf4

 233 | @@ -228,6 +234,7 @@ Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoin
 234 |          }
 235 |      }
 236 |  
 237 | +

promag commented at 11:38 PM on January 29, 2018:

Nit, remove.

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

utACK faca18dcf499e36069ce5fcd3e02a5ee86639436

This should be tested with a big wallet now that IsMine is called when the context menu opens.

I could be misunderstanding the concern, but I'd expect the size of the wallet not to make a big difference if IsMine is only called for the one transaction that's clicked.
MarcoFalke commented at 11:13 PM on January 30, 2018: member

@promag Thanks for raising the issue. Though, I confirmed that it is indeed only called when the context menue opens, so it shouldn't be a problem.
promag commented at 11:33 PM on January 30, 2018: member

Just saying that the UI will block because of IO and all, and that may be noticeable on big wallets (lots of keys) - reason AllFromMe must fetch all spent scriptPubKey and call IsMine for each one.

in src/wallet/feebumper.cpp:75 in faca18dcf4

  70 | +        errors.push_back("Transaction is not BIP 125 replaceable");
  71 | +        return feebumper::Result::WALLET_ERROR;
  72 | +    }
  73 | +
  74 | +    if (wtx.mapValue.count("replaced_by_txid")) {
  75 | +        errors.push_back(strprintf("Cannot bump transaction %s which was already bumped by transaction %s", wtx.GetHash().ToString(), wtx.mapValue.at("replaced_by_txid")));

promag commented at 2:46 PM on February 6, 2018:

Could avoid 2nd lookup, but looks more clean this way.

promag commented at 2:52 PM on February 6, 2018: member

utACK faca18d.
jonasschnelli commented at 10:25 AM on February 12, 2018: contributor

utACK faca18dcf499e36069ce5fcd3e02a5ee86639436
jonasschnelli merged this on Feb 12, 2018
jonasschnelli closed this on Feb 12, 2018
jonasschnelli referenced this in commit 8e6f9f4ebc on Feb 12, 2018
MarcoFalke deleted the branch on Feb 12, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12297 Improve CWallet::IsAllFromMe for false results