Remove lookup-tx-by-utxo functionality #8170

1. 
   sipa commented at 12:54 pm on June 8, 2016: member

   There are 3 ways through which main.cpp (and by extension, the getrawtransaction and REST tx lookup) find transactions:

   • In the mempool (works always)
   • In the block index (works only when -txindex is enabled).
   • In the UTXO set (inefficient, and only for confirmed transactions that have some of their outputs not yet spent by other confirmed transactions)

   This third option is confusing because the conditions under which it works are complicated, and can make it seem like it always works even without -txindex. Remove it. If you need to call getrawtransaction for confirmed transactions, you almost certainly need -txindex.

   This only impacts the getrawtransaction and REST tx calls, so document those.

2. Remove unreliable lookup-tx-by-utxo functionality c3af143133
3. sipa added the label Brainstorming on Jun 8, 2016
4. sipa added the label RPC/REST/ZMQ on Jun 8, 2016
5. 
   laanwj commented at 2:24 pm on June 8, 2016: member

   What is the migration path for users that were using this to query the utxo set? I suppose gettxout and REST /rest/getutxos cover that?
6. 
   sipa commented at 2:27 pm on June 8, 2016: member

   Yes, use UTXO query functions to query the UTXO set.
7. 
   laanwj commented at 5:39 am on June 9, 2016: member

   This definitely needs mention in the release notes. Maybe deprecate this functionality in 0.13 (put it behind an option that is disabled by default) then actually remove it in 0.14?
8. 
   laanwj commented at 5:42 am on June 9, 2016: member

    0abandonconflict.py:
    1Initializing test directory /tmp/testvmbco_b4/4
    2start_node: bitcoind started, waiting for RPC to come up
    3start_node: RPC succesfully started
    4start_node: bitcoind started, waiting for RPC to come up
    5start_node: RPC succesfully started
    6JSONRPC error: No information available about transaction. Use `gettransaction` for wallet transactions, or enable -txindex to use getrawtransaction on confirmed transactions.
    7Stopping nodes
    8Not cleaning up dir /tmp/testvmbco_b4/4
    9Failed
   10stderr:
   11   File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/test_framework/test_framework.py", line 144, in main
   12    self.run_test()
   13  File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/abandonconflict.py", line 42, in run_test
   14    nA = next(i for i, vout in enumerate(self.nodes[0].getrawtransaction(txA, 1)["vout"]) if vout["value"] == Decimal("10"))
   15  File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/test_framework/coverage.py", line 49, in __call__
   16    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
   17  File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/test_framework/authproxy.py", line 144, in __call__
   18    raise JSONRPCException(response['error'])
   19Pass: False, Duration: 6 s
   

9. 
   dcousens commented at 6:40 am on June 9, 2016: contributor

   concept ACK
10. 
    gmaxwell commented at 6:43 am on June 9, 2016: contributor

    I agree that it’s so incomprehensible (and not desirable) in its behavior that it’s unlikely anyone was using it (or at least, using it and not silently getting screwed over by it). I do wish any of these queries that only worked with txindex had a non-txindex mechanism that took a location parameter (e.g. block height.)
11. 
    MarcoFalke commented at 11:39 am on June 9, 2016: member

    I don’t think we need to deprecate this behind a command line flag. (Keep in mind that there still would be no way for us to find out how many would toggle the flag)

    Considering that this is not really a functionality someone can rely on and it appears unlikely someone (other than our test framework) relies on it, it is probably safe to just remove it.

12. 
    laanwj commented at 2:40 pm on June 9, 2016: member

    I don’t think we need to deprecate this behind a command line flag. (Keep in mind that there still would be no way for us to find out how many would toggle the flag)

    That’s true, but it would give people the opportunity to complain about it, while not having to forego the upgrade.

    I don’t feel strongly about it though, if you guys believe that this is a change that can be made without warning in advance I’m fine with that. Depends on how urgent this change is as well.

    I do wish any of these queries that only worked with txindex had a non-txindex mechanism that took a location parameter (e.g. block height.)

    One radical idea would be to expose CDiskTxPos fields on the interface here and there, and add a REST/RPC call to get a transaction from such a (blockfile_id,offset) tuple. I wonder if that would make some things easier/possible, such as external indices without having to store the block chain twice, but with fast access. Probably these ‘pointers’ should be obfuscated somehow to discourage direct access to the block files from outside bitcoind.

13. 
    sipa commented at 2:44 pm on June 9, 2016: member

    Depends on how urgent this change is as well.

    The changes here are not urgent. If we choose to not adopt them quickly, I think at least the updated error message in getrawtransaction is useful.

14. 
    petertodd commented at 4:49 am on June 15, 2016: contributor

    One of my clients has something that was using this FWIW, so I’d target actual removal for v0.14
15. 
    laanwj commented at 9:09 am on June 16, 2016: member

    One of my clients has something that was using this FWIW, so I’d target actual removal for v0.14

    That’s interesting. I somehow expected that, if some functionality is available for so long, some people will come to rely on it. What is their use case? Could they (in principle, given warning upfront to update their code) use the other UTXO query commands as replacement?

16. 
    petertodd commented at 10:55 am on June 16, 2016: contributor

    @laanwj Basically their wallet code happens to depend on it; with a fair amount of new code they could work around the problem (remember that simply saving the txs when created isn’t enough due to malleability).

    I don’t think that’s a show-stopper, but we should give users some kind of warning period. Perhaps we could make v0.13.0 appear to not have this support by default, but then re-enable it with a command line flag, that’s marked as “will be removed in v0.14.0”?

17. 
    laanwj commented at 7:30 am on June 18, 2016: member

    Perhaps we could make v0.13.0 appear to not have this support by default, but then re-enable it with a command line flag, that’s marked as “will be removed in v0.14.0”?

    +1. That’s exactly what I suggested too:

    Maybe deprecate this functionality in 0.13 (put it behind an option that is disabled by default) then actually remove it in 0.14?

18. 
    petertodd commented at 9:03 pm on June 18, 2016: contributor

    @laanwj Cool, sounds like we’re in agreement. :)
19. 
    sipa commented at 9:05 pm on June 18, 2016: member

    Ok I’ll reword this to just be the extra RPC help/error, and a message that it is deprecated?
20. 
    petertodd commented at 9:06 pm on June 18, 2016: contributor

    @sipa Sounds good to me, thanks!
21. 
    laanwj commented at 12:54 pm on August 2, 2016: member

    Looks like this causes a few RPC tests to fail:

    0wallet.py                      | False  | 9 s
    1rest.py                        | False  | 7 s
    2abandonconflict.py             | False  | 7 s
    

22. 
    dcousens commented at 1:27 pm on August 2, 2016: contributor

    utACK c3af143, but needs the command line option (or maybe just a short-term compile flag?)
23. 
    laanwj commented at 7:08 pm on October 18, 2016: member

    Still failing travis.
24. 
    gmaxwell commented at 9:39 pm on December 4, 2016: contributor

    @sipa Needs rebase.
25. 
    morcos commented at 7:04 pm on December 5, 2016: member

    I’m ever so slightly against this change. I find it’s routinely useful for me when I’m trying to debug behavior on a random node.
26. 
    sipa commented at 8:26 pm on December 5, 2016: member

    @morcos Perhaps that’s a good reason to introduce a getmempooltransaction RPC? My reason for this (or a related) change is that getrawtransaction has very unclear conditions under which it works, and I keep seeing people say it doesn’t work.
27. 
    laanwj commented at 10:51 am on December 21, 2016: member

    Perhaps that’s a good reason to introduce a getmempooltransaction RPC

    Agree with @morcos that the way toward fixing this could also be make the API clearer instead of removing functionality (that is apparently used by some people sometimes, even if only by developers for troubleshooting).

    +1 for splitting up getrawtransaction into calls that look in one place only.

28. sipa closed this on Jan 11, 2017

    ---

29. MarcoFalke locked this on Sep 8, 2021

Contributors
sipa laanwj dcousens gmaxwell MarcoFalke petertodd morcos

Labels
Brainstorming RPC/REST/ZMQ