Return txid even if ATMP fails for new transaction #9302

1. 
   sipa commented at 7:49 pm on December 8, 2016: member
2. jonasschnelli added the label Wallet on Dec 8, 2016
3. 
   jonasschnelli commented at 7:54 pm on December 8, 2016: contributor

   utACK a0728ccebb155b09cc537c88275662e33c1ab675 Would be nice if someone could write a test for this.
4. Return txid even if ATMP fails for new transaction b3a74100b8
5. sipa force-pushed on Dec 8, 2016
6. sipa added the label Bug on Dec 8, 2016
7. sipa added the label Needs backport on Dec 8, 2016
8. 
   morcos commented at 9:01 pm on December 8, 2016: member

   utACK b3a7410

   I think this needs release notes documenting the change in behavior.

   For 0.14.0 (not backport) we should also consider how to pass on the information on whether the tx was accepted to the mempool and if not why not?

9. 
   gmaxwell commented at 10:50 pm on December 8, 2016: contributor

   utACK. Will test. @morcos the listtransaction output tells you if the transaction is currently in the mempool, and we log the rejection. Is that covering a lot of your thinking?
10. MarcoFalke added this to the milestone 0.13.2 on Dec 8, 2016
11. MarcoFalke added the label Needs release notes on Dec 8, 2016
12. 
    luke-jr commented at 0:42 am on December 9, 2016: member

    utACK. It may make sense to move broadcasting outside of CommitTransaction entirely, but this is good enough for now (and backport).
13. 
    sdaftuar commented at 1:46 pm on December 9, 2016: member

    ACK b3a74100b86423c553ac327f3ea6fdbc2c50890a, wrote up a quick test to exercise the logic (EDIT: note that this test is no good to merge, as #9262 may require it to be rewritten):

     0diff --git a/qa/rpc-tests/wallet.py b/qa/rpc-tests/wallet.py
     1index 3c0dc0f..6435f00 100755
     2--- a/qa/rpc-tests/wallet.py
     3+++ b/qa/rpc-tests/wallet.py
     4@@ -346,5 +346,18 @@ class WalletTest (BitcoinTestFramework):
     5         assert_equal(coinbase_tx_1["transactions"][0]["blockhash"], blocks[1])
     6         assert_equal(len(self.nodes[0].listsinceblock(blocks[1])["transactions"]), 0)
     7 
     8+        # Test that spending coins doesn't fail as long as balance is high enough.
     9+        to_send = 0.05
    10+        txids = []
    11+        while self.nodes[0].getbalance() > to_send:
    12+            txids.append(self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), to_send))
    13+
    14+        mempool = self.nodes[0].getrawmempool()
    15+        sent_tx_not_in_mempool = False
    16+        for txid in txids:
    17+            if (txid not in mempool):
    18+                sent_tx_not_in_mempool = True
    19+        assert(sent_tx_not_in_mempool) # otherwise test needs to be updated to cover this case
    20+
    21 if __name__ == '__main__':
    22     WalletTest().main()
    

14. 
    morcos commented at 3:35 pm on December 9, 2016: member

    @gmaxwell No, I mean more than that. I think it would be good to inform the user that for some reason his transaction has not be accepted to the mempool. Perhaps there is no good way to do this without breaking the RPC API, but at the very least we could do it from the GUI.

    But to be clear, I do not think that should be a blocker for this. I’ll agree that this is an improvement, I just think we should continue to improve afterwards…

15. 
    gmaxwell commented at 5:13 pm on December 9, 2016: contributor

    ACK
16. 
    CodeShark commented at 8:57 am on December 11, 2016: contributor

    ACK
17. 
    MarcoFalke commented at 9:28 am on December 11, 2016: member

    utACK b3a74100b86423c553ac327f3ea6fdbc2c50890a
18. 
    MarcoFalke commented at 9:34 am on December 11, 2016: member

    I’d prefer not to merge any test case for this, as a passing test would assert that there is a bug in the code base. Tests are usually written to assert that there are no bugs…
19. 
    morcos commented at 1:37 pm on December 11, 2016: member

    @marcofalke what do you mean by that? There are certainly ways for transactions to not be accepted into the memory pool which aren’t bugs and I think it could make sense to verify that a txid is still returned.
20. 
    MarcoFalke commented at 2:11 pm on December 11, 2016: member

    @morcos What I mean is that the wallet logic should take care that a transaction is accepted to the mempool. When it is not possible, return early in the wallet logic and don’t commit the transaction to the wallet, otherwise it is a bug.

    I see the return-txid patch as a temporary workaround and not a solution to the problem. So sure, you could create a unit test to check that a transaction fails ATMP but still returns true when commited, but I don’t think it makes sense to create a functional test for this. Note that the test above by sdaftuar relies on bug #9019. Another (maybe minimal) test case could rely on bug #9316. Similarly you could come up with a test that pumps the mempoolminfee above the default wallet fee rate, I assume. Of course one could argue that some of these are features and not bugs, but as long as they appear in conjunction with -walletbroadcast enabled, I consider them bugs.

21. 
    morcos commented at 3:51 pm on December 11, 2016: member

    @MarcoFalke ah ok, yes I mostly agree. However I think that too long chains may end up being an exception to that where we want the wallet to generate and save the transaction even if its not accepted to the mempool. That’s obviously after a best efforts try to create the transaction without a too long chain.

    In any case, I think we both agree nothing more needed on this PR (but release notes still needed)

22. 
    instagibbs commented at 8:16 pm on December 12, 2016: member

    @sdaftuar fwiw #9262 will no longer change any behavior by default aside from attempt to make shorter chains so that test may be fine
23. 
    instagibbs commented at 8:17 pm on December 12, 2016: member

    utACK b3a74100b86423c553ac327f3ea6fdbc2c50890a
24. MarcoFalke assigned laanwj on Dec 12, 2016
25. 
    paveljanik commented at 8:58 am on December 13, 2016: contributor

    utACK https://github.com/bitcoin/bitcoin/commit/b3a74100b86423c553ac327f3ea6fdbc2c50890a
26. laanwj merged this on Dec 13, 2016
27. laanwj closed this on Dec 13, 2016

    ---

28. laanwj referenced this in commit b6abdc77d3 on Dec 13, 2016
29. laanwj referenced this in commit 782328660e on Dec 14, 2016
30. MarcoFalke removed the label Needs release notes on Dec 14, 2016
31. MarcoFalke referenced this in commit 8b5c2be8a0 on Dec 14, 2016
32. MarcoFalke removed the label Needs backport on Dec 14, 2016
33. 
    MarcoFalke commented at 12:09 pm on December 14, 2016: member

    Backport in #9347 (release notes already merged)
34. MarcoFalke referenced this in commit 44c342a1a1 on Dec 14, 2016
35. MarcoFalke referenced this in commit 322e48c93b on Dec 14, 2016
36. MarcoFalke referenced this in commit 1b51e7044b on Dec 14, 2016
37. MarcoFalke referenced this in commit f5d606e5ab on Dec 14, 2016
38. dexX7 referenced this in commit 2fdbc202ae on Jan 9, 2017
39. dexX7 referenced this in commit b1c266f414 on Jan 9, 2017
40. dexX7 referenced this in commit afb0c58e0b on Jan 23, 2017
41. dexX7 referenced this in commit a400d26561 on Jun 8, 2017
42. dexX7 referenced this in commit 2476c64723 on Jun 8, 2017
43. dexX7 referenced this in commit 3edbe7cc79 on Jun 8, 2017
44. codablock referenced this in commit d347d1fffe on Jan 16, 2018
45. codablock referenced this in commit 4c5a0fc74c on Jan 16, 2018
46. codablock referenced this in commit d0db70b01c on Jan 17, 2018
47. andvgal referenced this in commit 6df3f68faa on Jan 6, 2019
48. CryptoCentric referenced this in commit d247450ac5 on Feb 25, 2019
49. laanwj referenced this in commit 8a191148db on Oct 24, 2019
50. DrahtBot locked this on Sep 8, 2021

Contributors
sipa jonasschnelli morcos gmaxwell luke-jr sdaftuar CodeShark MarcoFalke instagibbs paveljanik

Labels
Bug Wallet

Milestone
0.13.2