wallet, rpc: add wtxid in WalletTxToJSON #24198

brunoerg commented at 10:18 PM on January 28, 2022: member

This PR add wtxid in WalletTxToJSON which allows to return this field in listsinceblock, listtransactions and gettransaction (RPCs).

w0xlt commented at 10:57 PM on January 28, 2022: contributor

Concept ACK

DrahtBot added the label RPC/REST/ZMQ on Jan 29, 2022

DrahtBot added the label Wallet on Jan 29, 2022

brunoerg marked this as ready for review on Jan 29, 2022

luke-jr changes_requested

luke-jr commented at 3:55 AM on January 31, 2022: member

Needs docs updated

brunoerg commented at 11:32 AM on January 31, 2022: member

Done, thanks @luke-jr.

brunoerg force-pushed on Jan 31, 2022

in src/wallet/rpc/transactions.cpp:38 in fe65dab3db outdated

  32 | @@ -33,7 +33,9 @@ static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue
  33 |          entry.pushKV("trusted", CachedTxIsTrusted(wallet, wtx));
  34 |      }
  35 |      uint256 hash = wtx.GetHash();
  36 | +    uint256 witness_hash = wtx.GetWitnessHash();
  37 |      entry.pushKV("txid", hash.GetHex());
  38 | +    entry.pushKV("wtxid", witness_hash.GetHex());

achow101 commented at 6:12 PM on January 31, 2022:

In fe65dab3dbef51e49b484e155ffbf7fd01ea2e8e "wallet: add wtxid in WalletTxToJSON"

nit: witness_hash is not used anywhere else, so it does not need to be a separate variable

    entry.pushKV("wtxid", wtx.GetWitnessHash().GetHex());

brunoerg commented at 6:33 PM on January 31, 2022:

Done!

achow101 commented at 6:12 PM on January 31, 2022: member

ACK 96d2cab3b06639c4ab4fa79b72aa81bdefe03492

brunoerg force-pushed on Jan 31, 2022

brunoerg commented at 6:33 PM on January 31, 2022: member

force-pushed addressing @achow101's comment.

luke-jr commented at 9:50 PM on January 31, 2022: member

Eh? Still don't see docs updated in the diff...

brunoerg commented at 9:53 PM on January 31, 2022: member

Eh? Still don't see docs updated in the diff... @luke-jr I created doc/release-notes-24198.md, isn't it the right way? See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes

luke-jr commented at 10:24 PM on January 31, 2022: member

Look in src/wallet/rpc/transactions.cpp for:

    return RPCHelpMan{"gettransaction",
                "\nGet detailed information about in-wallet transaction <txid>\n",
                {
                    {"txid", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction id"},

You need to add "wtxid" here too. Same for the other affected RPC methods.

brunoerg commented at 10:44 PM on January 31, 2022: member

You need to add "wtxid" here too. Same for the other affected RPC methods. @luke-jr Sorry, my bad. I was with the release notes in my mind, I forgot that one. Force pushed including it. Thank you!

luke-jr approved

luke-jr commented at 11:17 PM on January 31, 2022: member

utACK

wallet: add GetWitnessHash() 7482b6f895

wallet: add wtxid in WalletTxToJSON e8c659a297

test: add wtxid in expected_fields for wallet_basic a5b66738f1

brunoerg force-pushed on Feb 1, 2022

brunoerg commented at 11:46 AM on February 1, 2022: member

Force-pushed - rebasing on master now CI works.

achow101 commented at 7:50 PM on February 1, 2022: member

ACK 79d6764c133638c7feccf4fb1f98242a9ba0863e

MarcoFalke commented at 7:56 PM on February 1, 2022: member

Is there a specific use case or is this just for fun?

brunoerg commented at 12:18 AM on February 2, 2022: member

Is there a specific use case or is this just for fun?

Not sure if there is a specific use case. I use listsinceblock a lot in a project (and I know many exchanges use it a lot as well, maybe for deposits) and missed it since it could help me to analyze transactions.

in doc/release-notes-24198.md:5 in 79d6764c13 outdated

   0 | @@ -0,0 +1,5 @@
   1 | +Updated RPCs
   2 | +------------
   3 | +
   4 | +- The following RPCs: `listtransactions`, `gettransaction` and `listsinceblock`
   5 | +  now include the `wtxid` of the transaction.

luke-jr commented at 9:14 PM on February 8, 2022:

nit: Probably a better way to phrase this

brunoerg commented at 12:27 AM on February 9, 2022:

Hm, interesting. Maybe:

"Every transaction returned by the following RPCs: listtransactions, gettransaction and listsinceblock now include a wtxid field (hash of serialized transaction, including witness data)."

luke-jr commented at 7:19 PM on February 9, 2022:

Suggest "The listtransactions, gettransaction, and listsinceblock RPC methods now include a wtxid field (hash of serialized transaction, including witness data) for each transaction."

brunoerg commented at 11:15 PM on February 9, 2022:

Cool!

in src/wallet/rpc/transactions.cpp:435 in 79d6764c13 outdated

 431 | @@ -431,6 +432,7 @@ static const std::vector<RPCResult> TransactionDescriptionString()
 432 |             {RPCResult::Type::NUM, "blockindex", /*optional=*/true, "The index of the transaction in the block that includes it."},
 433 |             {RPCResult::Type::NUM_TIME, "blocktime", /*optional=*/true, "The block time expressed in " + UNIX_EPOCH_TIME + "."},
 434 |             {RPCResult::Type::STR_HEX, "txid", "The transaction id."},
 435 | +           {RPCResult::Type::STR_HEX, "wtxid", "The witness transaction id."},

luke-jr commented at 9:16 PM on February 8, 2022:

nit: The two existing "wtxid" descriptions are different.

I prefer "hash of serialized transaction, including witness data"

brunoerg commented at 12:12 AM on February 9, 2022:

I also prefer "hash of serialized transaction, including witness data", going to change it.

taveechaimekwan commented at 1:15 AM on February 9, 2022:

luke-jr approved

luke-jr commented at 9:17 PM on February 8, 2022: member

re-utACK 79d6764c133638c7feccf4fb1f98242a9ba0863e

nit: IMO squash 79d6764c133 (RPC docs) and a5b66738f19 (QA) into e8c659a2970 (feature addition)

DrahtBot commented at 8:40 PM on February 9, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24293 ((Refactor) QA: wallet_basic: Split wtx expected_fields over multiple lines to minimise merge conflicts by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

brunoerg force-pushed on Feb 9, 2022

brunoerg commented at 11:21 PM on February 9, 2022: member

Force-pushed addressing @luke-jr's comments

doc: add wtxid info in release-notes 2d596bce6f

doc: include wtxid in TransactionDescriptionString 7abd8b21ba

brunoerg force-pushed on Feb 10, 2022

w0xlt approved

w0xlt commented at 2:20 PM on February 10, 2022: contributor

crACK 7abd8b2

achow101 commented at 1:15 PM on March 8, 2022: member

re-ACK 7abd8b21ba34f6a42db2cd2d59a4c0e08561f609

achow101 commented at 1:16 PM on March 8, 2022: member

@luke-jr re-review this?

luke-jr approved

luke-jr commented at 7:22 PM on March 8, 2022: member

re-utACK 7abd8b21ba34f6a42db2cd2d59a4c0e08561f609

achow101 merged this on Mar 8, 2022

achow101 closed this on Mar 8, 2022

sidhujag referenced this in commit db3a7ef421 on Mar 9, 2022

DrahtBot locked this on Mar 8, 2023