rpc: fix regression in gettransaction #16873

1. 
   jonatack commented at 6:39 PM on September 14, 2019: member

   Closes #16872.

   PR #16866 renamed the decode argument in gettransaction to verbose to make it more consistent with other RPC calls like getrawtransaction. However, it inadvertently overloaded the "details" field when verbose is passed. The result is that the original "details" field is no longer returned correctly, which seems to be a breaking API change.

   This PR:

   • takes the simplest path to restoring the "details" field by renaming the decoded one back to "decoded" while leaving the verbose argument for API consistency, which was the main intent of #16866,

   • addresses this comment by mentioning in the RPC help that the new decoded field is equivalent to decoderawtransaction, and

   • updates the help, functional test, and release note.

   Reviewers, to test this manually, build and run bitcoin-cli help gettransaction and bitcoin-cli gettransaction <wallet txid> false true, and verify that the command returns both details and decoded fields.

2. rpc: fix regression in gettransaction

   PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.
   
   However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.
   
   This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.
   
   It also addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) to mention that the 'decoded' field is identical to decoderawtransaction.
   
   Update the RPC help, functional test, and release note.

   0f34f54888
3. DrahtBot added the label Docs on Sep 14, 2019
4. DrahtBot added the label RPC/REST/ZMQ on Sep 14, 2019
5. DrahtBot added the label Tests on Sep 14, 2019
6. DrahtBot added the label Wallet on Sep 14, 2019
7. meshcollider added this to the milestone 0.19.0 on Sep 14, 2019
8. meshcollider removed the label Docs on Sep 14, 2019
9. meshcollider removed the label Tests on Sep 14, 2019
10. meshcollider removed the label Wallet on Sep 14, 2019
11. 
    meshcollider commented at 9:23 PM on September 14, 2019: contributor

    Nice catch! utACK 0f34f54888f680bfbe7a29ac278636d7178a99bb

12. 
    promag commented at 9:48 PM on September 14, 2019: member

    I feel embarrassed for not catching this previously.. That said, to me this is also an indication that the verbose test is incomplete, it should test the full response if verbose=true. BTW I think this is a good practice because it allows to better test how parameters interact with each other and how it affects the result.

    Also related, and I already said this previously in other PR, I think all .pushKV calls to build a response should throw instead of overwriting. I feel that most certainly the developer doesn't want to overwrite, and as such execution should abort (note that these JSON keys are never user input).

    Tested ACK 0f34f54888f680bfbe7a29ac278636d7178a99bb.

13. 
    jnewbery commented at 11:12 PM on September 14, 2019: member

    concept ACK.

    I feel embarrassed for not catching this previously..

    ... but not as embarrassed as I feel for writing it!

    the verbose test is incomplete, it should test the full response if verbose=true

    I agree that wallet_basic.py should be updated to test the details field, with verbose set to both True and False.

    I think all .pushKV calls to build a response should throw instead of overwriting

    Concept ACK. Let me know when there's a PR to review.

14. 
    jonatack commented at 11:18 PM on September 14, 2019: member

    @promag, @jnewbery: :+1: agree with both of your ideas for improving the test coverage. I'll do that tomorrow and let @promag add .pushKV throwing instead of overwriting.

15. fanquake added the label Waiting for author on Sep 15, 2019
16. darosior approved
17. 
    darosior commented at 12:40 PM on September 15, 2019: member

    utACK 0f34f54

18. test: improve gettransaction test coverage

    - Test gettransaction response without verbose, with verbose=False, and with verbose=True.
    
    - In each case, test presence of expected fields in the output, including absence of the "decoded" field when `verbose` is not passed or false.
    
    - Test that the "details" field contains the expected receive vout in each case.

    1b41c2c8a1
19. jonatack force-pushed on Sep 15, 2019
20. 
    jnewbery commented at 5:07 PM on September 15, 2019: member

    tACK 1b41c2c8a126ef4be183e1d800a17d85cab8837b

    New tests are great. Thanks Jon!

21. 
    jonatack commented at 5:07 PM on September 15, 2019: member

    @meshcollider, @promag, @jnewbery, @darosior thanks for reviewing! Added a follow-up commit with more coverage. EDIT: Thanks, John!

22. meshcollider referenced this in commit b0a7a76c9d on Sep 15, 2019
23. meshcollider merged this on Sep 15, 2019
24. meshcollider closed this on Sep 15, 2019

    ---

25. jonatack deleted the branch on Sep 15, 2019
26. luke-jr referenced this in commit da9c4aca90 on Sep 21, 2019
27. luke-jr referenced this in commit f36f032d06 on Sep 21, 2019
28. laanwj removed the label Waiting for author on Oct 24, 2019
29. vansergen referenced this in commit 21ab707301 on Mar 26, 2020
30. jasonbcox referenced this in commit 567961d857 on Sep 7, 2020
31. MarcoFalke locked this on Dec 16, 2021

Milestone
0.19.0