[RPC] Remove lookup to UTXO set from GetTransaction #15159

• stop checking unspent UTXOs for a transaction when txindex is not enabled, as per conversation here: #3220 (comment)
• code contributed by sipa

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14802 (rpc: faster getblockstats using BlockUndo data by FelixWeis)
• #14053 (Add address-based index (attempt 4?) by marcinja)
• #13903 (Significantly reduce GetTransaction cs_main locking (TheBlueMatt) by MarcoFalke)

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.

Concept ACK

Slightly tend to NACK. If I am not mistaken, this will force some user to enable -txindex, which seems unintended. Otherwise they might run into a race condition where they are first notified of a tx in the mempool, then try to getrawtransaction that tx, which might sometimes fail because it was added to a block in the meantime.

Thanks for picking this up @amitiuttarwar! Looks good. I have a few high-level comments.

• The PR description (https://github.com/bitcoin/bitcoin/pull/15159#issue-244314271) gets added to the commit log when the PR is merged (see https://github.com/bitcoin/bitcoin/commit/1cfbb16b5da85647441bf84b4120d01e04d813ed for example). For that reason, I try to keep my PR descriptions as pure descriptions of the new/changed functionality, and add contextual notes (eg if this PR is the rebase of commits from another PR) to the first comment in the PR. See #13926 for example.
• We try to make sure that the binary builds and all tests pass on every intermediate commit, so we can use git bisect. That means that if functionality changes and the tests need to change to still pass, then the product and test changes should be in the same commit. For this PR, I suggest you have two commits: one for 'Update getrawtransaction interface' and one for '[RPC] Simplify gettxoutproof method'. If you're worried about losing authorship information in the commit logs, you can always add an extra line to the commit message like "Additional code contributed by <author>" (don't use @ or that user will get annoying notifications in github), although in this case I don't think that matters too much. I'm fine for you to use my code without the authorship data.
• As you've already pointed out to me, the RPC help text for gettxoutproof should be updated.

Otherwise they might run into a race condition where they are first notified of a tx in the mempool, then try to getrawtransaction that tx, which might sometimes fail because it was added to a block in the meantime.

Even without this change, that race condition exists (if the tx's outputs are spent in the same block).

I think the race is sufficiently rare that it's not realistically an issue, but we should definitely make sure that the error messages are clear enough in this case - ie it's explicit that the tx was not found in the mempool.

thanks for the feedback @jnewbery. LMK if the PR looks okay now.

Looking at this again, I recommend you drop the second commit entirely and save it for a follow-up PR.

Currently the first commit doesn't build on its own, since the change here: https://github.com/bitcoin/bitcoin/pull/15159/files#diff-01aa7d1d32f1b9e5a836c9c411978918R259 needs to be part of the first commit. The change to gettxoutproof should be removed from the first commit (since the gettxoutproof behaviour only changes in the second commit).

ok updated so this PR focuses solely on changes to getrawtransaction.

I think this needs mention in the release notes due to changed getrawtransaction semantics?

AppVeyor build is failing on a test that passes locally. I'll look into it soon, but if anyone cruises through here and has any pointers (first time using AppVeyor), feel free to share :)

AppVeyor has a lot of spurious failures. It can be ignored!

utACK 04da9f4834e1651da65ceb6379950cef9450591c

If -txindex is enabled then it should be used first? Is there another reason to enable it other than speed up transaction lookup (so it doesn't stress cs_main or mempool.cs under load)? Just asking it now since this PR changes its behavior.

hey @promag, can you clarify your questions?

• reason to enable -txindex for users running bitcoind? or in the tests?
• what changed behavior are you referring to? the way GetTransaction behaves when -txindex has not changed much.

@amitiuttarwar decided to create an issue with the question #15293.

I think the question about whether to try txindex before mempool can be disregarded in this PR, since it doesn't change that behaviour. #15293 can track that question, and the change described there could be merged before or after this PR if it's wanted.

@jnewbery indeed a separate discussion, that's why I've created an issue. PR title and commit message could be more clear, like "Remove lookup to UTXO set from GetTransaction".

utACK 04da9f4.

utACK 04da9f4834e1651da65ceb6379950cef9450591c

Restarted appveyor, which failed with

2019-01-27T02:59:07.394000Z TestFramework (ERROR): Assertion failed 
 Traceback (most recent call last):
   File "C:\projects\bitcoin\test\functional\test_framework\test_framework.py", line 173, in main
     self.run_test()
   File "C:\projects\bitcoin/test/functional/wallet_txn_doublespend.py", line 114, in run_test
     sync_blocks(self.nodes)
   File "C:\projects\bitcoin\test\functional\test_framework\util.py", line 392, in sync_blocks
     raise AssertionError("Block sync timed out:{}".format("".join("\n  {!r}".format(b) for b in best_hash)))
 AssertionError: Block sync timed out:
   '3080ca6e61d16e2bd8b236d733076c346d151670c26de9c27660b11771bf916d'
   '3080ca6e61d16e2bd8b236d733076c346d151670c26de9c27660b11771bf916d'
   '3f8f4778bbc8d606465fc0aeca065077722935875e3502af994f28bb740bc3ce'
   '3f8f4778bbc8d606465fc0aeca065077722935875e3502af994f28bb740bc3ce'

@promag if I'm not mistaken, I think #15253 will help debug that failure

well thats a most shittiest update you did.

• i obliged to call gettransaction to get blockhash for tx i need
• after this i need to call getrawtransaction with blockhash from previous call.

Before i did all in 1 call, not here are 2. What next?

@nikitasius What is in getrawtransaction that is not in gettransaction?

@MarcoFalke I think the only missing thing is that gettransaction only returns the hex raw tx, and has no ability to decode it directly. You can of course use decoderawtransaction on the hex output from gettransaction to accomplish the same as getrawtransaction (with txindex, and without knowing the block hash).

@MarcoFalke in my case: vins here is a tx:

• listunspent 3

[
  {
    "txid": "b423b2b22d6d74376de2887c07f7c84ae890a93d541fcc6dbf3686f2923b2753",
    "vout": 0,
    "address": "2N4p68W5AfTJiEX6B5DUgp2eBWP1m2d9pSY",
    "redeemScript": "0014c13a875f4565d9b39cb7898a30e011e7b03d14ad",
    "scriptPubKey": "a9147ee08cf27aed73648f2ba5df2d6c70a60dc0c74587",
    "amount": 2.99988992,
    "confirmations": 22133,
    "spendable": true,
    "solvable": true,
    "desc": "sh(wpkh([4dc444eb/0'/1'/139']028dd079912c6bd68cc9d2872f1d810aece81ac1f5404105b68cefa571e8ecd0cd))#rtxpgrnv",
    "safe": true
  }
]  

• gettransaction "b423b2b22d6d74376de2887c07f7c84ae890a93d541fcc6dbf3686f2923b2753"

{
  "amount": -2.00000000,
  "fee": -0.00010746,
  "confirmations": 22133,
  "blockhash": "00000000000000409e394d133807fd4f6b0fd27ade24a6e717ee257e825167e1",
  "blockindex": 4,
  "blocktime": 1559505152,
  "txid": "b423b2b22d6d74376de2887c07f7c84ae890a93d541fcc6dbf3686f2923b2753",
  "walletconflicts": [
  ],
  "time": 1559505089,
  "timereceived": 1559505089,
  "bip125-replaceable": "no",
  "details": [
    {
      "address": "2NAzsxhZUwLEg8ii2tnrUH9wxrPC3edDDaQ",
      "category": "send",
      "amount": -2.00000000,
      "label": "",
      "vout": 1,
      "fee": -0.00010746,
      "abandoned": false
    }
  ],
  "hex": "02000000000101fb78a5db1271cb652e1716a1e00dc23409dc9801deea53da5cde50e11e6877900100000017160014feaec8c89d5f76a80a3c0694ba92a82d2f1bfda8fdffffff020078e1110000000017a9147ee08cf27aed73648f2ba5df2d6c70a60dc0c7458700c2eb0b0000000017a914c2bbb902207a6ea4961ec41da629225a30f95f5a870247304402206f0666187b110c8fd689a08db838150fd919d6df516f8e74b6a7bf632e392d17022023edc46d0cc41e1d98fc5042364a065d7bfb54e2b71d2cf794cc3a17e8daee8701210330e02dbd607553975a833664b9c663a1c1ee81d9f2221b784fe9465bf32db24f7e2f1700"
}

• getrawtransaction b423b2b22d6d74376de2887c07f7c84ae890a93d541fcc6dbf3686f2923b2753 1 00000000000000409e394d133807fd4f6b0fd27ade24a6e717ee257e825167e1

{
  "in_active_chain": true,
  "txid": "b423b2b22d6d74376de2887c07f7c84ae890a93d541fcc6dbf3686f2923b2753",
  "hash": "8d497a20df578b57a3190c5380816197b78ce92150c1b62a60e3d1ee4d26e7c9",
  "version": 2,
  "size": 247,
  "vsize": 166,
  "weight": 661,
  "locktime": 1519486,
  "vin": [
    {
      "txid": "9077681ee150de5cda53eade0198dc0934c20de0a116172e65cb7112dba578fb",
      "vout": 1,
      "scriptSig": {
        "asm": "0014feaec8c89d5f76a80a3c0694ba92a82d2f1bfda8",
        "hex": "160014feaec8c89d5f76a80a3c0694ba92a82d2f1bfda8"
      },
      "txinwitness": [
        "304402206f0666187b110c8fd689a08db838150fd919d6df516f8e74b6a7bf632e392d17022023edc46d0cc41e1d98fc5042364a065d7bfb54e2b71d2cf794cc3a17e8daee8701",
        "0330e02dbd607553975a833664b9c663a1c1ee81d9f2221b784fe9465bf32db24f"
      ],
      "sequence": 4294967293
    }
  ],
  "vout": [
    {
      "value": 2.99988992,
      "n": 0,
      "scriptPubKey": {
        "asm": "OP_HASH160 7ee08cf27aed73648f2ba5df2d6c70a60dc0c745 OP_EQUAL",
        "hex": "a9147ee08cf27aed73648f2ba5df2d6c70a60dc0c74587",
        "reqSigs": 1,
        "type": "scripthash",
        "addresses": [
          "2N4p68W5AfTJiEX6B5DUgp2eBWP1m2d9pSY"
        ]
      }
    },
    {
      "value": 2.00000000,
      "n": 1,
      "scriptPubKey": {
        "asm": "OP_HASH160 c2bbb902207a6ea4961ec41da629225a30f95f5a OP_EQUAL",
        "hex": "a914c2bbb902207a6ea4961ec41da629225a30f95f5a87",
        "reqSigs": 1,
        "type": "scripthash",
        "addresses": [
          "2NAzsxhZUwLEg8ii2tnrUH9wxrPC3edDDaQ"
        ]
      }
    }
  ],
  "hex": "02000000000101fb78a5db1271cb652e1716a1e00dc23409dc9801deea53da5cde50e11e6877900100000017160014feaec8c89d5f76a80a3c0694ba92a82d2f1bfda8fdffffff020078e1110000000017a9147ee08cf27aed73648f2ba5df2d6c70a60dc0c7458700c2eb0b0000000017a914c2bbb902207a6ea4961ec41da629225a30f95f5a870247304402206f0666187b110c8fd689a08db838150fd919d6df516f8e74b6a7bf632e392d17022023edc46d0cc41e1d98fc5042364a065d7bfb54e2b71d2cf794cc3a17e8daee8701210330e02dbd607553975a833664b9c663a1c1ee81d9f2221b784fe9465bf32db24f7e2f1700",
  "blockhash": "00000000000000409e394d133807fd4f6b0fd27ade24a6e717ee257e825167e1",
  "confirmations": 22137,
  "time": 1559505152,
  "blocktime": 1559505152
}

• then i pick this vin array:

 "vin": [
    {
      "txid": "9077681ee150de5cda53eade0198dc0934c20de0a116172e65cb7112dba578fb",
      "vout": 1,
      "scriptSig": {
        "asm": "0014feaec8c89d5f76a80a3c0694ba92a82d2f1bfda8",
        "hex": "160014feaec8c89d5f76a80a3c0694ba92a82d2f1bfda8"
      },
      "txinwitness": [
        "304402206f0666187b110c8fd689a08db838150fd919d6df516f8e74b6a7bf632e392d17022023edc46d0cc41e1d98fc5042364a065d7bfb54e2b71d2cf794cc3a17e8daee8701",
        "0330e02dbd607553975a833664b9c663a1c1ee81d9f2221b784fe9465bf32db24f"
      ],
      "sequence": 4294967293
    }
  ]

• and call getrawtransaction for 9077681ee150de5cda53eade0198dc0934c20de0a116172e65cb7112dba578fb:

{
	"result": {
		"txid": "9077681ee150de5cda53eade0198dc0934c20de0a116172e65cb7112dba578fb",
		"hash": "58d247d5d571b95271b9b01603ac6283410d8c28d48fe69082431122029a1669",
		"version": 2,
		"size": 247,
		"vsize": 166,
		"weight": 661,
		"locktime": 0,
		"vin": [{
			"txid": "ab904f2596bafe5c29110ebe053dbd3d609d238456c7ae97d8c916a1c0a27665",
			"vout": 0,
			"scriptSig": {
				"asm": "0014b56400beea1c0c592f9d89e1f182583c04360743",
				"hex": "160014b56400beea1c0c592f9d89e1f182583c04360743"
			},
			"txinwitness": ["304402201ba93887222fdaac6001a1c54cea3fd78e1fc5b7d48f22c6ed647a330605746702205c4aaba5673efc4a21235cbbf5d61fd6e9ce83aa2fa25507db84a1ff3abdbca401", "0242adf8c0e977e622db8e2776b02129844fdc5d9fddf5bd963aa85ed87a8ee7ec"],
			"sequence": 4294967295
		}],
		"vout": [{
			"value": 11.90559816,
			"n": 0,
			"scriptPubKey": {
				"asm": "OP_HASH160 cabc590e4c35a02b44f77034cea6fcd823d6d18a OP_EQUAL",
				"hex": "a914cabc590e4c35a02b44f77034cea6fcd823d6d18a87",
				"reqSigs": 1,
				"type": "scripthash",
				"addresses": ["2NBjC7g1h3FN7oiLH21DRDCoSUyXhK5heGg"]
			}
		}, {
			"value": 4.99999738,
			"n": 1,
			"scriptPubKey": {
				"asm": "OP_HASH160 ec459c516bb17526fd4fffa02448c6b3f581c065 OP_EQUAL",
				"hex": "a914ec459c516bb17526fd4fffa02448c6b3f581c06587",
				"reqSigs": 1,
				"type": "scripthash",
				"addresses": ["2NEnWrBmNCmtwNQLsifKqkuRNxRtQsvKkd8"]
			}
		}],
		"hex": "020000000001016576a2c0a116c9d897aec75684239d603dbd3d05be0e11295cfeba96254f90ab0000000017160014b56400beea1c0c592f9d89e1f182583c04360743ffffffff024880f6460000000017a914cabc590e4c35a02b44f77034cea6fcd823d6d18a87fa63cd1d0000000017a914ec459c516bb17526fd4fffa02448c6b3f581c065870247304402201ba93887222fdaac6001a1c54cea3fd78e1fc5b7d48f22c6ed647a330605746702205c4aaba5673efc4a21235cbbf5d61fd6e9ce83aa2fa25507db84a1ff3abdbca401210242adf8c0e977e622db8e2776b02129844fdc5d9fddf5bd963aa85ed87a8ee7ec00000000",
		"blockhash": "0000000000000032d4bfc14d4783bc4269a7c1ffdc5bc41b9b2b7aa92baa82a5",
		"confirmations": 23389,
		"time": 1558701822,
		"blocktime": 1558701822
	},
	"error": null,
	"id": "1"
}

• then i see what vout n=1 from 2NEnWrBmNCmtwNQLsifKqkuRNxRtQsvKkd8.
• so i know which address sent this tx.

Discussion should move to the issues linked from here rather than on this closed PR.

I would like to make one observation about the last comment (https://github.com/bitcoin/bitcoin/pull/15159#issuecomment-500348023). A lot of the complexity comes from the fact that you're trying to apply a 'from address'. Such a concept doesn't exist in bitcoin. A transaction can have multiple txins, each of which refer to a txout that may have a scriptPubKey that encodes to an address.

Addresses should be seen more as single-use invoice identifiers. Trying to apply a from/to address model doesn't fit with the Bitcoin UTXO model and will only cause you headaches!