rpc: fix "trusted" field in TransactionDescriptionString(), add coverage #23139

jonatack commented at 8:13 PM on September 29, 2021: member

The RPC gettransaction, listtransactions, and listsinceblock helps returned by TransactionDescriptionString() inform the user that the trusted boolean field is only present if the transaction is trusted and safe to spend from.

The field is in fact returned by WalletTxToJSON() when the transaction has 0 confirmations (or negative confirmations, if conflicted), and it can be true or false.

This patch fixes the help, adds test coverage, and touches up the help for the neighboring generate field.

rpc: fix "trusted" description in TransactionDescriptionString

The helps for RPCs gettransaction, listtransactions, and
listsinceblock returned by TransactionDescriptionString()
state that the "trusted" boolean field is only present if the
transaction is trusted and safe to spend from.

The "trusted" boolean field is in fact returned by
WalletTxToJSON() when the transaction has 0 confirmations,
or negative confirmations, if conflicted, and it can be
true or false.

This commit updates TransactionDescriptionString() to a
more accurate description for "trusted" and updates the
existing line of test coverage to fail more helpfully.

d95913fc43

test: add listtransactions/listsinceblock "trusted" coverage 296cfa312f

rpc: improve TransactionDescriptionString() "generated" help 66f6efc70a

jonatack force-pushed on Sep 29, 2021

DrahtBot added the label RPC/REST/ZMQ on Sep 29, 2021

DrahtBot added the label Wallet on Sep 29, 2021

MarcoFalke commented at 5:45 AM on September 30, 2021: member

Does it make sense to return this for negative confirmations?

jonatack commented at 11:40 AM on September 30, 2021: member

Does it make sense to return this for negative confirmations?

In this case per CachedTxIsTrusted() the trusted field will always return false, which, along with the walletconflicts field returning txids of conflicting txs, seems good to return to the user. So the existing behavior appears to be fine but the RPC helps are incorrect. Though I could imagine one could also argue for always returning the trusted field or only returning it if confirmations equals 0.

in src/wallet/rpcwallet.cpp:1393 in 66f6efc70a

1387 | @@ -1388,8 +1388,9 @@ static const std::vector<RPCResult> TransactionDescriptionString()
1388 |  {
1389 |      return{{RPCResult::Type::NUM, "confirmations", "The number of confirmations for the transaction. Negative confirmations means the\n"
1390 |                 "transaction conflicted that many blocks ago."},
1391 | -           {RPCResult::Type::BOOL, "generated", /* optional */ true, "Only present if transaction only input is a coinbase one."},
1392 | -           {RPCResult::Type::BOOL, "trusted", /* optional */ true, "Only present if we consider transaction to be trusted and so safe to spend from."},
1393 | +           {RPCResult::Type::BOOL, "generated", /* optional */ true, "Only present if the transaction's only input is a coinbase one."},
1394 | +           {RPCResult::Type::BOOL, "trusted", /* optional */ true, "Whether we consider the transaction to be trusted and safe to spend from.\n"
1395 | +                "Only present when the transaction has 0 confirmations (or negative confirmations, if conflicted)."},

rajarshimaitra commented at 5:12 PM on September 30, 2021:

Noob question, what does it mean to have "negative confirmation"?

jonatack commented at 9:01 PM on September 30, 2021:

"Negative confirmations" are those having a value less than zero, like -1. (Would it be clearer to write "less than zero" instead?)

Negative confirmations signify that the transaction conflicted that many blocks ago.

Code-wise, WalletTxToJSON() in src/wallet/rpcwallet.cpp calls to CWallet::GetTxDepthInMainChain() in wallet.{h,cpp} to fetch the value. In the following line, that function changes the height to a negative value if the transaction is conflicted:

    return (GetLastBlockHeight() - wtx.m_confirm.block_height + 1) * (wtx.isConflicted() ? -1 : 1);

#23146 opened today adds testing that explores this behavior.

jonatack commented at 9:05 PM on September 30, 2021:

See also CWallet::MarkConflicted() and its callers, and the documentation in CWallet::transactionRemovedFromMempool().

rajarshimaitra commented at 12:56 PM on October 1, 2021:

Ah thanks.. No I think the comment with "negative confirmation" is fine.. I was just curious to understand when it will occur.

rajarshimaitra commented at 5:41 PM on September 30, 2021: contributor

tACK https://github.com/bitcoin/bitcoin/pull/23139/commits/66f6efc70a72cc1613906fd3c10281f9af0ba0db

luke-jr approved

luke-jr commented at 6:45 PM on October 3, 2021: member

crACK

theStack approved

theStack commented at 4:50 PM on October 17, 2021: member

Tested ACK 66f6efc70a72cc1613906fd3c10281f9af0ba0db

laanwj merged this on Oct 22, 2021

laanwj closed this on Oct 22, 2021

sidhujag referenced this in commit 625130e8fc on Oct 22, 2021

jonatack deleted the branch on Oct 23, 2021

luke-jr referenced this in commit eaec8a9b25 on Dec 14, 2021

luke-jr referenced this in commit 4fd4c5f54b on Dec 14, 2021

luke-jr referenced this in commit aafa54c573 on Dec 14, 2021

DrahtBot locked this on Oct 30, 2022