rpc, wallet: add abandoned field for all categories of transaction in ListTransaction #25158

pull brunoerg wants to merge 2 commits into bitcoin:master from brunoerg:2022-05-abandoned-listtransactions changing 3 files +15 −6

brunoerg commented at 4:14 PM on May 17, 2022: contributor

Fixes #25130
fanquake added the label RPC/REST/ZMQ on May 17, 2022
DrahtBot commented at 5:08 AM on May 18, 2022: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.

Type	Reviewers
ACK	achow101

in doc/release-notes.md:87 in 6ef89bf198 outdated

  82 | @@ -83,6 +83,9 @@ Low-level changes
  83 |  RPC
  84 |  ---
  85 |  
  86 | +- The `gettransaction` and `listtransactions` RPCs now return
  87 | +  the `abandoned` field for all categories of transactions.

jonatack commented at 8:17 AM on May 18, 2022:

Is this missing listsinceblock?

A few suggestions for your consideration:

-- The `gettransaction` and `listtransactions` RPCs now return
-  the `abandoned` field for all categories of transactions.
+- The `gettransaction`, `listtransactions`, and `listsinceblock` RPCs now return
+  the "abandoned" field for all transactions. Previously, the "abandoned" field
+  was only returned for sent transactions. (#25158)

brunoerg commented at 9:33 AM on May 18, 2022:

Yeah! Nice find, thanks!

in src/wallet/rpc/transactions.cpp:737 in 6ef89bf198 outdated

 733 | @@ -735,8 +734,7 @@ RPCHelpMan gettransaction()
 734 |                                  {RPCResult::Type::NUM, "vout", "the vout value"},
 735 |                                  {RPCResult::Type::STR_AMOUNT, "fee", /*optional=*/true, "The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n"
 736 |                                      "'send' category of transactions."},
 737 | -                                {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n"
 738 | -                                     "'send' category of transactions."},
 739 | +                                {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable)."},

jonatack commented at 8:27 AM on May 18, 2022:

For these three RPC helps, "abandoned" would no longer optional?

brunoerg commented at 1:16 PM on May 18, 2022:

I think so.

jonatack commented at 8:33 AM on May 18, 2022: contributor

Quick readover of the code as a break from seminar readings, but a key point would be to consider why this field was not returned other than for sent txns (and if concept ACKs, ensure there is appropriate test coverage in the tests that assert on this field. Quick grep:)

test/functional/feature_backwards_compatibility.py:197:                        assert not(txs[1]["abandoned"])
test/functional/feature_backwards_compatibility.py:200:                        assert txs[3]["abandoned"]
test/functional/wallet_abandonconflict.py:197:            assert_equal(tx["abandoned"], False)
test/functional/wallet_abandonconflict.py:202:        assert_equal(double_spend["abandoned"], False)
test/functional/wallet_abandonconflict.py:131:            assert_equal(tx['abandoned'], True)

brunoerg commented at 9:33 AM on May 18, 2022: contributor

a key point would be to consider why this field was not returned other than for sent txns

I couldn't find any resources about it, i'd like to understand more, btw Concept NACKs are welcome! I find #7739 which added this field but there is no explanation about why it is only in send.
brunoerg force-pushed on May 18, 2022
brunoerg commented at 1:35 PM on May 18, 2022: contributor

Force-pushed addressing @jonatack's comments

in src/wallet/rpc/transactions.cpp:466 in c3450ad2dc outdated

 482 | @@ -482,8 +483,7 @@ RPCHelpMan listtransactions()
 483 |                          },
 484 |                          TransactionDescriptionString()),
 485 |                          {
 486 | -                            {RPCResult::Type::BOOL, "abandoned", /*optional=*/true, "'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n"
 487 | -                                 "'send' category of transactions."},
 488 | +                            {RPCResult::Type::BOOL, "abandoned", "'true' if the transaction has been abandoned (inputs are respendable)."},

luke-jr commented at 7:07 PM on May 18, 2022:

This description only makes any sense for sent transactions

brunoerg commented at 1:42 PM on January 6, 2023:

Not specifically as we discussed on the issue.

DrahtBot added the label Needs rebase on Aug 16, 2022
brunoerg force-pushed on Aug 17, 2022
DrahtBot removed the label Needs rebase on Aug 17, 2022
DrahtBot added the label Needs rebase on Sep 16, 2022
achow101 commented at 7:21 PM on October 12, 2022: member

Are you still working on this?
brunoerg commented at 1:42 PM on January 6, 2023: contributor

Are you still working on this?

Yes.
brunoerg force-pushed on Jan 6, 2023
DrahtBot removed the label Needs rebase on Jan 6, 2023
achow101 commented at 12:01 AM on January 13, 2023: member

ACK efdb80a69000572a5f36fc702090da99c627238d

I think it's reasonable to output this for all transactions, even if we decide that one type cannot be abandoned. A trivial case where this is useful are send-to-self transactions as these show up as individual send and receive transactions.
maflcko commented at 9:03 AM on January 13, 2023: member

You will have to modify the RPC docs for gettransaction as well
rpc, wallet: add `abandoned` field for all categories of transactions in ListTransactions a1aaa7f51f
doc: add release notes for `abandoned` field in `gettransaction` and `listtransactions` 0c520679ab
brunoerg force-pushed on Jan 13, 2023
brunoerg commented at 1:29 PM on January 13, 2023: contributor

You will have to modify the RPC docs for gettransaction as well

Nice find, thanks! Force-pushed modifying it.
achow101 commented at 10:32 PM on January 13, 2023: member

re-ACK 0c520679ab5f0ba99584cb356ec28ef089f14735
fanquake requested review from Sjors on Feb 8, 2023
achow101 merged this on Apr 26, 2023
achow101 closed this on Apr 26, 2023
sidhujag referenced this in commit 031c0b26bd on Apr 28, 2023
bitcoin locked this on Apr 25, 2024

Contributors

Review Requested
Sjors

Labels

RPC/REST/ZMQ

Linked (view graph)

#25130 gettransaction does not contain the field "abandoned" for abandoned receiving tx

rpc, wallet: add abandoned field for all categories of transaction in ListTransaction #25158

Reviews

Conflicts