trivial: Mention that wallet needs to be enabled for gettransaction #11797

hkjn commented at 11:18 AM on November 30, 2017: contributor

trivial: Mention that wallet needs to be enabled for gettransaction

If we were compiled from ./configure --disable-wallet, the gettransaction RPC will not be available.

~~After this change, when we were compiled from ./configure --disable-wallet, the message changes to no longer mention gettransaction RPC, which will not be available:~~

$ bitcoin-cli getrawtransaction 5644a275cfe20ca22f7de6bdd8c5313317b5937ae4bcefb6f109ca0dfdc346db error code: -5 error message: No such mempool transaction. Use -txindex to enable blockchain transaction queries.

~~When ./configure is run with --enable-wallet or without --disable-wallet (enabling wallet is the default), the message remains the same after this change:~~

$ bitcoin-cli getrawtransaction 5644a275cfe20ca22f7de6bdd8c5313317b5937ae4bcefb6f109ca0dfdc346db error code: -5 error message: No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions.

fanquake added the label RPC/REST/ZMQ on Nov 30, 2017

laanwj commented at 11:54 AM on November 30, 2017: member

Thanks for the contribution, but I think we can't merge this. One longer-term goal is to remove the dependency of the core library on ENABLE_WALLET - see #7965 .

Working around that is going to be a mess. What I'd suggest is to just add "if the wallet is enabled" to that help message.

hkjn force-pushed on Nov 30, 2017

hkjn renamed this:
~~trivial: Avoid mentioning gettransaction if wallet is not enabled~~
trivial: Mention that wallet needs to be enabled for gettransaction
on Nov 30, 2017

hkjn commented at 12:09 PM on November 30, 2017: contributor

Thanks for the review @laanwj, that makes sense to me.

I've updated the PR to just modify the existing message, please take another look.

laanwj commented at 12:17 PM on November 30, 2017: member

Looks good to me now, utACK https://github.com/bitcoin/bitcoin/pull/11797/commits/5ce6908092a6b3b5b818435af424800a28f457d0

hkjn force-pushed on Nov 30, 2017

hkjn commented at 1:27 PM on November 30, 2017: contributor

I noticed when trying to compile locally that C++ strings can't be concatenated with +.

My bad, I've removed the + to let the compiler concatenate them together, via https://stackoverflow.com/a/1135862/8414666.

laanwj commented at 1:43 PM on November 30, 2017: member

I noticed when trying to compile locally that C++ strings can't be concatenated with +.

Right - std::string objects can be concatenated through +, but C strings (const char*) cannot be because they're pointers and adding them isn't defined (and if it was defined it'd likely do something else than you want).

hkjn commented at 1:48 PM on November 30, 2017: contributor

Yup.. I know all this in theory, I wrote my thesis around a project I wrote in ANSI C, but that was some years back. :)

Let me know if you would prefer another way to concat the string than the implicit route I went now.

laanwj commented at 1:53 PM on November 30, 2017: member

Let me know if you would prefer another way to concat the string than the implicit route I went now.

"foo" "bar" is exactly the right way to concatenate strings at compile time.

in src/rpc/rawtransaction.cpp:162 in 6511ef6142 outdated

 157 | @@ -158,10 +158,12 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
 158 |  
 159 |      CTransactionRef tx;
 160 |      uint256 hashBlock;
 161 | -    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true))
 162 | -        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex ? "No such mempool or blockchain transaction"
 163 | -            : "No such mempool transaction. Use -txindex to enable blockchain transaction queries") +
 164 | -            ". Use gettransaction for wallet transactions.");
 165 | +    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true)) {
 166 | +        std::string msg = std::string(fTxIndex ? "No such mempool or blockchain transaction." :

MarcoFalke commented at 6:03 PM on November 30, 2017:

nit: missing a space after end of sentence?

hkjn commented at 12:02 PM on December 1, 2017:

Not sure I follow?

The row you're commenting on is the message when fTxIndex is true, i.e. -txindex is specified:

$./src/bitcoin-cli getrawtransaction 5644aC275cfe20ca22f7de6bdd8c5313317b5937ae4bcefb6f109ca0dfdc346db
error code: -5
error message:
No such mempool or blockchain transaction.

There is no space at the end of the sentence because that's the end of the message; the longer message on L163:164 for when -txindex is not set does have a space after the first sentence:

$ ./src/bitcoin-cli getrawtransaction 5644a275cfe20ca22f7de6bdd8c5313317b5937ae4bcefb6f109ca0dfdc346db
error code: -5
error message:
No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions, if wallet is enabled.

MarcoFalke commented at 6:10 PM on December 1, 2017:

Sorry, you are right.

MarcoFalke commented at 6:05 PM on November 30, 2017: member

Imo it is clear in the current form. If wallet is disabled, there are no wallet transaction, so can't use gettransaction

MarcoFalke commented at 6:07 PM on November 30, 2017: member

I think the goal was to split up those calls. See #8170 (comment) if you'd like to work on that.

laanwj commented at 6:20 PM on November 30, 2017: member

@MarcoFalke gettransaction and getrawtransaction are already separate calls, The intent here is just to document it better.

What you're linking to deals with splitting up getrawtransaction into explicit "look up utxo transaction in block" and "look up transaction in mempool" calls, it's not related to the wallet.

hkjn commented at 12:13 PM on December 1, 2017: contributor

Thanks all for your review.

Yes, as @laanwj says the goal here is just to make documentation more clear; I was confused that getrawtransaction mentioned an RPC that didn't exist on my node, since it was compiled with --disable-wallet. My original thought was to make all the wallet RPCs stubbed out in this scenario with a message that they're unable to return data without wallet support. In discussions on IRC others objected to that as too complex / leading to likely duplication of RPC method names, but agreed that the message could be more clear, which leads to the current PR.

I see now that #9520 introduced the current message, and I certainly could be missing historical context, so let me know if this change somehow makes things worse, or if there's a larger change we should pursue.

hkjn force-pushed on Dec 1, 2017

laanwj commented at 2:33 PM on December 1, 2017: member

Ideally gettransaction would have been called getwallettransaction, but it's far from the only RPC name that suffers from name scoping issues so I don't think there's any use in changing it now, just more API churn.

MarcoFalke added the label Docs on Dec 1, 2017

in src/rpc/rawtransaction.cpp:164 in 44f854a638 outdated

 163 | -            : "No such mempool transaction. Use -txindex to enable blockchain transaction queries") +
 164 | -            ". Use gettransaction for wallet transactions.");
 165 | +    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true)) {
 166 | +        std::string msg = std::string(fTxIndex ? "No such mempool or blockchain transaction." :
 167 | +                                                 "No such mempool transaction. Use -txindex to enable blockchain transaction queries. "
 168 | +                                                 "Use gettransaction for wallet transactions, if wallet is enabled.");

promag commented at 4:05 PM on December 4, 2017:

for wallet transactions hence wallet must enabled, therefore , if wallet is enabled not needed?

hkjn commented at 4:27 PM on December 4, 2017: contributor

Ok, so via @laanwj it seems that while we agree that some RPC endpoints have less-than-ideal naming (e.g. gettransaction should be getwallettransaction, which I'd agree would have hinted more heavily that it's tied to wallet txs), renaming the endpoints just to make the names clearer might not be worth it.

Short of renaming the methods, this PR instead tries to provide a clearer help message when the tx is not found. The PR originally used an #ifdef WALLET_ENABLED to conditionally tell the user that wallet needed to be enabled, but @laanwj pointed out that there's an effort to move away from that in #7965, so now the PR just appends if wallet is enabled to the message unconditionally, to make it more clear that this is required.

It seems that we're not in agreement on whether the suggested change here actually makes anything clearer..

I can drop this PR, if others feel that there's nothing to improve here, or can suggest a more widely acceptable change.

MarcoFalke commented at 4:06 PM on December 6, 2017: member

Needs rebase if still relevant.

hkjn force-pushed on Dec 6, 2017

trivial: Mention that wallet needs to be enabled for gettransaction

If we were compiled from `./configure --disable-wallet`, the `gettransaction` RPC will not be available.

b881eec682

hkjn force-pushed on Dec 6, 2017

hkjn commented at 5:25 PM on December 6, 2017: contributor

Rebased.

hkjn commented at 1:54 PM on December 9, 2017: contributor

I'll close this PR at current time to avoid clutter. Closing due to lack of interest, and some pushback that suggested changes doesn't really clarify the current message.

I believe we are in rough agreement (see comments) that current naming of some RPC methods is less than ideal; e.g. gettransaction might be better named as getwallettransaction, i.e. namespaced under wallet-specific calls.

So in case someone wants to provide future work to address the confusion around naming of RPC methods that led to this PR, I'd suggest that if the benefits outweigh the churn in name changes they could make a proposal for renaming RPC methods, where new names could be introduced, followed by eventual deprecation, and finally removal of old methods N releases later.

hkjn closed this on Dec 9, 2017

DrahtBot locked this on Sep 8, 2021