RPC: Also serve txo from gettxout (not just utxo and mempool) #10822

jtimon · 2017-07-14T07:59:50Z

Expand gettxout to also serve spent txo if pruning or lack of -txindex don't get in the way.

If the output is spent, it will be slower to load, but there's no reason not to serve this if the data is there.

TODO (functional tests):

sipa · 2017-07-16T19:25:38Z

I don't think we should be expanding the impact of -txindex. This functionality is already available through getrawtransaction anyway.

jtimon · 2017-07-17T05:58:23Z

I don't think we should be expanding the impact of -txindex.

Is there a problem with that? This is just RPC anyway.
I also think making -txindex more useful may increase the incentives to run an archive full node.

My reasoning (with some errors corrected below it) is here: #9806 (comment)

This functionality is already available through getrawtransaction anyway.

By that logic, shouldn't we just fully remove gettxout?
That would be bad for gettxoutbyscript and gettxoutbyaddress though, I think.

gettxout is faster for utxo, so if one wants a txo (not knowing if stxo/utxo a priori), one could call gettxout first and only if it fails call getrawtransation. But I think a single call would be more convenient.
If not, I guess we need getoutpointfromscript and getoupointfrom address call too to serve the full txo.
One could, for example, call gettxoutbyaddress and if it fails, also call gettxoutbyaddress and getrawtransaction. It would be more cumbersome, but it would also work.

promag · 2017-08-21T22:49:19Z

src/rpc/blockchain.cpp

        fMempool = request.params[2].get_bool();

+    bool include_spent = false;
+    if (request.params.size() > 3) {


if (!request.params[3].isNull()) {

promag · 2017-08-21T22:49:40Z

src/rpc/blockchain.cpp

    ScriptPubKeyToUniv(coin.out.scriptPubKey, o, true);
    ret.push_back(Pair("scriptPubKey", o));
    ret.push_back(Pair("coinbase", (bool)coin.fCoinBase));
+    ret.push_back(Pair("spent", (bool)is_spent));


Remove the cast?

promag · 2017-08-21T22:53:34Z

src/rpc/blockchain.cpp


 extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry);

+static bool ReadTxoFromOutpoint(Coin& coin, bool& is_spent, const CBlockIndex* pindex, const COutPoint& out, bool include_spent)


pindex not used.

promag · 2017-08-21T22:58:40Z

src/rpc/blockchain.cpp

+            "1. \"txid\"             (string, required) The transaction id\n"
+            "2. \"n\"                (numeric, required) vout number\n"
+            "3. \"include_mempool\"  (boolean, optional) Only search in the mempool. Default: true\n"
+            "4. \"include_spent\"    (boolean, optional) Whether to include spent outputs. Default: false\n"


ATM only works if include_mempool is false.

promag · 2017-08-21T22:59:54Z

src/rpc/blockchain.cpp

+        include_spent = request.params[3].get_bool();
+    }
+
+    bool is_spent = false;


An output of a mempool transaction can be spent, right?

promag · 2017-08-21T23:00:22Z

src/validation.cpp

+    LOCK(cs_main);
+
+    CTransactionRef ptx = mempool.get(hash);
+    if (ptx)


promag · 2017-08-21T23:05:27Z

test/functional/wallet.py

        self.setup_clean_chain = True
        self.num_nodes = 4
        self.extra_args = [['-usehd={:d}'.format(i%2==0)] for i in range(4)]
+        self.extra_args[0].append('-txindex')


@jnewbery this is why I prefer isolated tests. This is unrelated to all these tests except the new.

promag · 2017-08-21T23:09:56Z

src/rpc/blockchain.cpp

+    bool include_spent = false;
+    if (request.params.size() > 3) {
+        include_spent = request.params[3].get_bool();
+    }


Disallow include_spent if include_mempool:

if (fMempool && include_spent) { throw JSONRPCError(RPC_INVALID_PARAMETER, "..."); }

If you add this, update test below.

promag · 2017-08-21T23:10:32Z

src/validation.cpp

+ * Return transaction in txOut, and if it was found inside a block,
+ * its hash is placed in hashBlock. Look in the mempool first.
+*/
+bool GetTransaction(const uint256 &hash, CTransactionRef &txOut, const Consensus::Params& consensusParams, uint256 &hashBlock, bool fAllowSlow)


snake_case.

@sipa

6d2d2eb RPC: gettxout: Slightly improve doc and tests (Jorge Timón) Pull request description: Slightly related to #10822 in the sense that I felt the documentation and testing wasn't as good as it could be while writing it. Ping @sipa since we discussed this on IRC. Tree-SHA512: a0b3ffdac65245a0429e772fc2d8bcc1e829b02c70fb2af6ee0b7578cae46683f6c51a824b4d703d4dc3f99b6f03a658d6bbc818bf32981516f24124249a211d

jtimon · 2017-09-06T00:05:08Z

Needs rebase and has nits but since I didn't heard any enthusiasm for more functionality regarding -txindex and @sipa was negative about it, I am closing this for now.
@promag thanks for the review and your nits will be taken into account if I ever reopen this.

EDIT: Happy to reopen if people want it (perhaps after/if there's an index for scriptPubKey -> txid:n )

@sipa

6d2d2eb RPC: gettxout: Slightly improve doc and tests (Jorge Timón) Pull request description: Slightly related to bitcoin/bitcoin#10822 in the sense that I felt the documentation and testing wasn't as good as it could be while writing it. Ping @sipa since we discussed this on IRC. Tree-SHA512: a0b3ffdac65245a0429e772fc2d8bcc1e829b02c70fb2af6ee0b7578cae46683f6c51a824b4d703d4dc3f99b6f03a658d6bbc818bf32981516f24124249a211d

@sipa

6d2d2eb RPC: gettxout: Slightly improve doc and tests (Jorge Timón) Pull request description: Slightly related to bitcoin#10822 in the sense that I felt the documentation and testing wasn't as good as it could be while writing it. Ping @sipa since we discussed this on IRC. Tree-SHA512: a0b3ffdac65245a0429e772fc2d8bcc1e829b02c70fb2af6ee0b7578cae46683f6c51a824b4d703d4dc3f99b6f03a658d6bbc818bf32981516f24124249a211d

@sipa

6d2d2eb49 RPC: gettxout: Slightly improve doc and tests (Jorge Timón) Pull request description: Slightly related to bitcoin/bitcoin#10822 in the sense that I felt the documentation and testing wasn't as good as it could be while writing it. Ping @sipa since we discussed this on IRC. Tree-SHA512: a0b3ffdac65245a0429e772fc2d8bcc1e829b02c70fb2af6ee0b7578cae46683f6c51a824b4d703d4dc3f99b6f03a658d6bbc818bf32981516f24124249a211d

jtimon changed the title ~~TOTEST: Also server txo from gettxout (not just utxo and mempool)~~ TOTEST: Also serve txo from gettxout (not just utxo and mempool) Jul 14, 2017

jtimon mentioned this pull request Jul 14, 2017

txoutsbyaddress index (take 3) #9806

Closed

jtimon force-pushed the b15-rpc-txo branch from a052bda to a032769 Compare July 14, 2017 09:05

fanquake added the RPC/REST/ZMQ label Jul 15, 2017

jtimon force-pushed the b15-rpc-txo branch 2 times, most recently from d983ccf to ffa0def Compare July 16, 2017 09:10

RPC: Also serve txo from gettxout (not just utxo and mempool)

efb8b71

jtimon force-pushed the b15-rpc-txo branch from ffa0def to efb8b71 Compare July 16, 2017 09:22

jtimon changed the title ~~TOTEST: Also serve txo from gettxout (not just utxo and mempool)~~ RPC: Also serve txo from gettxout (not just utxo and mempool) Jul 16, 2017

jtimon mentioned this pull request Jul 18, 2017

RPC: gettxout: Slightly improve doc and tests #10859

Merged

promag reviewed Aug 21, 2017

View reviewed changes

jtimon closed this Sep 6, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry);

		static bool ReadTxoFromOutpoint(Coin& coin, bool& is_spent, const CBlockIndex* pindex, const COutPoint& out, bool include_spent)

RPC: Also serve txo from gettxout (not just utxo and mempool) #10822

RPC: Also serve txo from gettxout (not just utxo and mempool) #10822

Uh oh!

Conversation

jtimon commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jul 16, 2017

Uh oh!

jtimon commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtimon commented Jul 14, 2017 •

edited

Loading

jtimon commented Sep 6, 2017 •

edited

Loading