Add RPC call abandontransaction #7312

pull morcos wants to merge 4 commits into bitcoin:master from morcos:forgetstuck changing 7 files +285 −17
  1. morcos commented at 5:33 pm on January 7, 2016: member

    Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined. abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction. All dependent transactions in the wallet will also be marked as abandoned. @laanwj also for 0.12

    This is the basic functionality.
    There are more things to add though. I have an RPC test in progress, but if anyone else wants to work on the remaining items, please do:

    • Return abandoned status in listtransactions
    • Return abandoned status in GUI
    • Fix any issues with how abandoned txs should sort
    • Add a way to abandon transactions from GUI

    I built this off of #7306 to make sure the tests would work correctly.

  2. luke-jr commented at 5:35 pm on January 7, 2016: member
    This is way too late for 0.12…
  3. morcos commented at 5:45 pm on January 7, 2016: member

    @luke-jr I don’t think we can release 0.12 without this. With mempool eviction combined with the new behaviour that unconfirmed txs still tie up the coins they spend even if the txs are not in the mempool, users may end up with permanently unspendable coins frequently.

    We discussed making this change in conjunction with the other changes back in November, but it appears to have slipped through the cracks.

  4. luke-jr commented at 5:52 pm on January 7, 2016: member
    This situation isn’t really any worse than the status quo from older versions…
  5. morcos force-pushed on Jan 7, 2016
  6. laanwj added the label Wallet on Jan 7, 2016
  7. in src/wallet/wallet.cpp: in 9726a520ee outdated 
    456@@ -455,8 +457,10 @@ bool CWallet::IsSpent(const uint256& hash, unsigned int n) const
457     {
458         const uint256& wtxid = it->second;
459         std::map<uint256, CWalletTx>::const_iterator mit = mapWallet.find(wtxid);
460-        if (mit != mapWallet.end() && mit->second.GetDepthInMainChain() >= 0)
461-            return true; // Spent
462+        if (mit != mapWallet.end()) {
463+            if (mit->second.GetDepthInMainChain() > 0  || (mit->second.GetDepthInMainChain() == 0 && !mit->second.isAbandoned()))
    jonasschnelli commented at 8:05 pm on January 7, 2016:
    CMerkleTx::GetDepthInMainChain() is slightly expansive (and IsSpent() gets called a lot). Would caching mit->second.GetDepthInMainChain() make sense? Or do we expect the compiler does it internally (I don’t think so)?
    morcos commented at 9:17 pm on January 7, 2016:
    sure can fix
  8. in src/wallet/wallet.cpp: in 9726a520ee outdated 
    47@@ -48,6 +48,8 @@ bool fSendFreeTransactions = DEFAULT_SEND_FREE_TRANSACTIONS;
48  */
49 CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE);
50 
51+const uint256 CMerkleTx::abandonHash(uint256S("0000000000000000000000000000000000000000000000000000000000000001"));
    jonasschnelli commented at 8:14 pm on January 7, 2016:
    nit: I probably would do it also that way,… but, maybe a cleaner way would be to use CWalletTx::mapValue (a flexible kv store) which also would not require a db-scheme update.
    morcos commented at 9:38 pm on January 7, 2016:
    I debated doing that and maybe it would have been better, but I liked the way updating the hashBlock automatically made the transaction no longer abandoned with this approach.
    jonasschnelli commented at 8:04 am on January 8, 2016:

    I debated doing that and maybe it would have been better, but I liked the way updating the hashBlock automatically made the transaction no longer abandoned with this approach.

    fair enough.

  9. jonasschnelli commented at 8:17 pm on January 7, 2016: contributor

    Concept ACK. I agree it late for 0.12,… but I don’t take a position on this (if it’s saver to go with or without a such feature).

    Would be nice to see some tests (especially when we tag this for 0.12).

  10. Make wallet descendant searching more efficient 9e69717254
  11. morcos force-pushed on Jan 7, 2016
  12. morcos commented at 9:42 pm on January 7, 2016: member
    Addressed @jonasschnelli’s comment (thanks for quick review!) and reordered commits for simpler reading.
  13. morcos commented at 3:47 am on January 8, 2016: member
    oops, i didn’t mean to push that RPC test yet, it was still in progress, but might as well leave it there for now…
  14. in src/wallet/rpcwallet.cpp: in 6d74a63154 outdated 
    1771+
1772+    if (fHelp || params.size() != 1)
1773+        throw runtime_error(
1774+            "abandontransaction \"txid\"\n"
1775+            "\nMark in-wallet transaction <txid> as abandoned\n"
1776+            "This will mark this tx and all its in-wallet descendants as abandoned which will allow\n"
    kanzure commented at 12:58 pm on January 8, 2016:
    consistency nit: all the other fHelp messages use “transaction” and “transactions” instead of “tx” and “txs”.
  15. kanzure commented at 1:01 pm on January 8, 2016: contributor
    I think a name like “abandonlocaltransaction” or “abandonunconfirmedtransaction” could help to minimize ambiguity. But I would consider this a very low priority suggestion, because this RPC call is only available when the wallet is enabled.
  16. MarcoFalke commented at 1:05 pm on January 8, 2016: member

    abandonunconfirmedtransaction

    How would you type that without any typos? I’d rather make the rpc short (“abandontx”) and the documentation/GUI verbose.

  17. MarcoFalke commented at 1:06 pm on January 8, 2016: member
    Concept ACK 6d74a63154cca756d698d6022c694b21e7f43ac5
  18. laanwj added this to the milestone 0.12.0 on Jan 8, 2016
  19. morcos force-pushed on Jan 8, 2016
  20. morcos commented at 4:46 pm on January 8, 2016: member
    Thanks for the review. Addressed nits in RPC help and finalized the RPC test
  21. in qa/pull-tester/rpc-tests.py: in 21702ff8cd outdated 
    104@@ -105,6 +105,7 @@
105     'prioritise_transaction.py',
106     'invalidblockrequest.py',
107     'invalidtxrequest.py',
108+    'abandonconflicts.py',
    MarcoFalke commented at 5:09 pm on January 8, 2016:
    abandonconflicts.py: No such file or directory
  22. in qa/rpc-tests/abandonconflict.py: in 21702ff8cd outdated 
    13+
14+class AbandonConflictTest(BitcoinTestFramework):
15+
16+    def setup_network(self):
17+        self.nodes = []
18+        self.nodes.append(start_node(0, self.options.tmpdir, ["-debug","-logtimemicros","-minrelaytxfee=0.00001"], binary="bitcoind"))
    MarcoFalke commented at 5:10 pm on January 8, 2016:

    Nit: Is the "-debug","-logtimemicros" part required?

    Also the binary="bitcoind" part?

    sdaftuar commented at 5:22 pm on January 8, 2016:
    I think we should drop the binary="bitcoind" part – by default start_node can pick up the binary from the BITCOIND environment variable which is preferred when set.
  23. MarcoFalke commented at 5:52 pm on January 8, 2016: member
    Reviewed-tests ACK 21702ff @morcos Please fix the travis issue (file not found)
  24. morcos force-pushed on Jan 8, 2016
  25. morcos commented at 7:05 pm on January 8, 2016: member
    oops sorry. fixed typo and removed bitcoind argument from rpc test.
  26. in qa/rpc-tests/abandonconflict.py: in d7168667b3 outdated 
    105+        newbalance = self.nodes[0].getbalance()
106+        assert(newbalance == balance - Decimal("20") + Decimal("14.99998"))
107+        balance = newbalance
108+
109+        # Send child tx again so its unabandoned
110+        self.nodes[0].sendrawtransaction(signed2["hex"])
    MarcoFalke commented at 7:43 pm on January 8, 2016:
    Shouldn’t resendwallettransactions do the same? I get the empty array on resendwallettransactions.
    morcos commented at 1:44 pm on January 13, 2016:
    I thought it made sense that abandoned transactions would not be automatically resent. That way when you restart a bitcoind with transactions that you’ve tried to abandon, it wouldn’t automatically try to send them all again
  27. MarcoFalke commented at 7:44 pm on January 8, 2016: member

    QT-tested ACK d716866

    screenshot from 2016-01-08 20-41-15 Nit: Qt balance != getwalletinfo

  28. in src/wallet/wallet.cpp: in d7168667b3 outdated 
    812+            // If the orig tx was not in block/mempool, none of its spends can be in mempool
813+            assert(!wtx.InMempool());
814+            wtx.nIndex = -1;
815+            wtx.setAbandoned();
816+            wtx.MarkDirty();
817+            wtx.WriteToDisk(&walletdb);
    jonasschnelli commented at 9:54 am on January 11, 2016:
    I think here we also need a NotifyTransactionChanged(this, wtx.GetHash(), CT_UPDATED); Otherwise the signal listeners (UI) can’t recalculate the balance.
  29. in src/wallet/wallet.cpp: in d7168667b3 outdated 
    826+            // If a transaction changes 'conflicted' state, that changes the balance
827+            // available of the outputs it spends. So force those to be recomputed
828+            BOOST_FOREACH(const CTxIn& txin, wtx.vin)
829+            {
830+                if (mapWallet.count(txin.prevout.hash))
831+                    mapWallet[txin.prevout.hash].MarkDirty();
    jonasschnelli commented at 9:55 am on January 11, 2016:
    Probably also here: NotifyTransactionChanged(this, txin.prevout.hash, CT_UPDATED);
  30. jonasschnelli commented at 10:02 am on January 11, 2016: contributor

    Tested ACK (also ack on a 0.12 bp, because it’s a wallet only change) Nits:

    • missing update signal (see above)
    • missing flag in listtransactions.
  31. jonasschnelli commented at 10:21 am on January 11, 2016: contributor
    Addressed the nits and a cosmetic change for GUI in: https://github.com/morcos/bitcoin/pull/6
  32. morcos commented at 7:27 pm on January 11, 2016: member
    @jonasschnelli , thanks! I added your notify commit. I left the other changes for a separate pull.
  33. morcos force-pushed on Jan 11, 2016
  34. morcos commented at 9:47 pm on January 11, 2016: member
    Ok updated after speaking to @jonasschnelli I think this is good now
  35. PRabahy commented at 4:39 am on January 12, 2016: contributor
    Does https://github.com/bitcoin/bitcoin/blob/master/contrib/bitcoind.bash-completion need to get updated with this change?
  36. MarcoFalke commented at 7:29 am on January 12, 2016: member
    @PRabahy sure, but I don’t think this is part of this pull.
  37. Add new rpc call: abandontransaction 
    Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined.  abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction.  All dependent transactions in the wallet will also be marked as abandoned.
    01e06d1fa3
  38. Add RPC test for abandoned and conflicted transactions. df0e2226d9
  39. [Wallet] Call notification signal when a transaction is abandoned d11fc1695c
  40. morcos force-pushed on Jan 13, 2016
  41. morcos commented at 1:46 pm on January 13, 2016: member
    Addressed @laanwj’s concern about clearly identifying constant
  42. laanwj merged this on Jan 13, 2016
  43. laanwj closed this on Jan 13, 2016
  44. laanwj referenced this in commit be6d5a617d on Jan 13, 2016
  45. laanwj referenced this in commit fd4bd5009e on Jan 13, 2016
  46. morcos referenced this in commit 25926ed30f on Jan 13, 2016
  47. random-zebra referenced this in commit 0f1764a3db on Oct 9, 2019
  48. DrahtBot locked this on Sep 8, 2021


morcos luke-jr jonasschnelli kanzure MarcoFalke sdaftuar PRabahy

Labels
Wallet

Milestone
0.12.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me