rpc: Prefer to use txindex if available for GetTransaction #22383

jlopp · 2021-07-01T14:23:28Z

Motivation: prevent excessive disk reads if txindex is enabled.

Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?

murchandamus · 2021-07-01T14:40:21Z

It looks like an empty line needs to be added to the commit message after the subject line for the linter to pass.

mjdietzx · 2021-07-01T16:31:20Z

I think this would change the behavior of GetTransaction in some cases. I'm not sure whether this is OK or not, but I'd think we should at least update comments here: https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L146, and docs here: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/rawtransaction.cpp#L78

The case I'm thinking about, which you noted, is when an incorrect block_index is provided to GetTransaction, but g_txindex->FindTx(hash, hashBlock, tx) finds and returns the tx. After this PR we would return the tx although it isn't in the block the user asked for. Idk the implications of this or if it matters, but if it does, you could probably check hashBlock against the user provided block_index and get the performance win you want without changing behavior (if necessary/practical)

Similarly, I'm not sure if checking the mempool first has any unintended consequences (other than deviating from current docs). That said, I'm not sure the if (mempool) { branch needs to move in this PR, can probably just leave it after the if (block_index) { branch as is. Again, not sure how we feel about returning a tx from the mempool when the user wanted to check a specific block_index

jlopp · 2021-07-01T18:37:23Z

I think a fundamental question is whether or not the block_index is intended to be used as a hint or as a validation parameter.

Given that the current logic of this function has various fallbacks, it seems to me that block_index is a hint and that the goal of the function is to try to return the requested transaction regardless of where it is stored. So it seems to me that reorganizing the logic for performance gains is a suitable goal.

sipa · 2021-07-01T18:45:21Z

FindTx returns the block hash, so it's very easy to check if it matches. If you do so, the question of whether the provided block is a hint or a search restriction doesn't apply.

promag

This is a behavior change, and no test fails which is unfortunate. If we want to do this then a release note would be nice.

src/validation.h

src/validation.cpp

src/validation.h

jlopp · 2021-07-02T12:58:05Z

I've added additional checks that should ensure there is no behavior change, only performance improvements. Now it should return nullptr faster in certain conditions.

src/validation.cpp

maflcko · 2021-07-03T07:38:43Z

review ACK 5dd11ea1406f4926ab9061702b69b409459723b4

jonatack

Concept ACK 5dd11ea1406f4926ab9061702b69b409459723b4

ISTM we could use additional test coverage. Edit: Am adding some, will propose separately.

src/rpc/rawtransaction.cpp

src/validation.cpp

Fixes bitcoin#22382

jonatack · 2021-07-03T12:03:14Z

ACK 78f4c8b

theStack · 2021-07-03T16:50:46Z

Concept ACK

mjdietzx · 2021-07-06T02:15:33Z

NACK -- In general, I think performance optimizations should come with data that shows they improve things. Especially in the case the code isn't being cleaned up or simplified (I think this PR makes the code slightly more complicated and harder to follow so I'd want to be sure the performance improvements warrant it). I also have other concerns which I lay out below. I can quickly come around on my review with some explanation/correction to my understanding if I'm off base anywhere.

Motivation: prevent excessive disk reads if txindex is enabled.

However, from #22382 which this PR fixes, the issue is reported as:

When using getrawtransaction with an included blockhash, performance should be better as there is no need to scan the entire txindex.

Was the issue misread? Because the issue expects scanning the entire txindex causes a slowdown, while this PR claims excessive disk reads (in the case that txindex is enabled) causes a slowdown. So to me the two seem to contradict each other.

To me it seems both ReadBlockFromDisk and g_txindex->FindTx do the same disk read, so I'm not sure how this change "prevents excessive disk reads" (please correct me if I'm misunderstanding something basic, I'm learning):

If I'm understanding this correctly, the main benefit of txindex being enabled is that we don't need blockhash to be specified to know which block to look in for the transaction, so we can still find it, but the same disk read is happens in both cases.

Now the measurements provided in the issue seem to show some performance discrepancy (which maybe @jlopp just looked at and knew what was going on right away). But I also don't know how much variation is normal in the timing of rpc calls, and whether one measurement (under unknown conditions) is good enough to warrant a performance optimization. On top of that it's only one data point - what if this tx reported in the issue happens to be at the beginning/end of txindex or who knows what?

promag · 2021-07-06T07:12:22Z

To me it seems both ReadBlockFromDisk and g_txindex->FindTx do the same disk read, so I'm not sure how this change "prevents excessive disk reads" (please correct me if I'm misunderstanding something basic, I'm learning):

@mjdietzx txindex stores the transaction position in the block, so it just seeks there to parse the transaction. Without txindex it needs to load the block - parse transactions, build hashes,... - and then search in those transactions.

mjdietzx · 2021-07-06T13:50:37Z

txindex stores the transaction position in the block, so it just seeks there to parse the transaction. Without txindex it needs to load the block - parse transactions, build hashes,... - and then search in those transactions.

Makes sense, and thanks for the explanation @promag. So is it safe to say scanning through the entire txindex and then "seeking to the tx's position in the block and reading it from disk" is much faster than " load the block - parse transactions, build hashes,... - and then search in those transactions"? Do we have any intuitions here, like big-O and/or worst-case runtimes?

If so, is it worth adding some good comments to the source code (not sure whether they are added in GetTransaction or in the ReadBlockFromDisk and FindTx declarations). Note, I have no intuition of the runtimes of these operations (due to my inexperience) so just lmk if this is obvious and unnecessary.

But here's where I'm coming from - how do I know the test measurements provided in #22382 are on mainnet (what if they are on some testnet where the txindex is extremely small? I can't find the transaction or block hashes from that issue in any block explorers). How do I even know -txindex is enabled for these measurements (and it's not just scanning through his wallet's transactions, and that's where the performance discrepancy comes from)?

Without some source code comments and data/measurments showing the performance differences here, it makes me wonder if the reason people pass block_hash in the first place is because it gets around scanning the entire txindex.

jlopp · 2021-07-06T14:22:40Z

I don't believe that "scanning through the entire txindex" is an accurate way to describe how txindex->FindTx functions. I expect it to be more of a O(1) constant time operation while ReadBlockFromDisk() is more of a O(n) operation that is directly correlated to block size.

promag · 2021-07-06T14:33:17Z

Right, it doesn't scan the entire txindex as keys are indexed. I think leveldb lookup is O(log n).

#22382 shows the performance difference from each approach.

orweinberger · 2021-07-13T07:38:27Z

FWIW, this is the compared performance with @jlopp's modifications

$ time ./src/bitcoin-cli getrawtransaction 20a93350cd6f4e03dfb6cbb3f104965293627f25c199c1be0e4d187a4ed5f4bb
020000000001018d3f7d8efaa377dda74892af26c1ca028466d97721d2ea6c74a5cbcf404feb9004000000171600147f5a49a2d89f86ff498103bdbcb1762f9f0ab5aefeffffff08a04a05000000000017a91467a849785bb12f1a03260b3b52ec9dff52b30e708724b80100000000001976a914f0b7e8a5ede232c2654fca3b47f8d0c95f7a561688ac50fe0b00000000001976a914ed06524fb21248a37e7f8f20cdc67e27f49c4f1588acf44f01000000000017a9147422dbf3bdf441a640d09a2a91e6fa40837b019f878d9b3100000000001976a9142a5cd9333e4e94c88f7cd33216b2fc0af15fbd8588ac89b400000000000017a914d05dbcdb4587f1a243cc1ed89e96cd4ec19e24a08778e169180000000017a914f520df8923419f60d9a76df60cfc7a0e3a3fc86f8708d806000000000017a9147246c684dd4890e9c8977e6312c30ecdea2b0a55870247304402205704dd3b6d8930e89c660011ab2f6a42aedcbde438d5f121998e3db6a66889310220054a506aee274f885229d6a9ed89eca17bac87dca401f9c63d07990e98b2f0a301210390e3b178ebc51da1589baee9de37de491dba0575838aacf88c2cd70a06a4486eb47c0a00

real	0m0.003s
user	0m0.002s
sys	0m0.000s


$ time ./src/bitcoin-cli getrawtransaction 20a93350cd6f4e03dfb6cbb3f104965293627f25c199c1be0e4d187a4ed5f4bb false 0000000000000000000a801776a92ced69dc5fb0a5919271872a9261a613d9c3
020000000001018d3f7d8efaa377dda74892af26c1ca028466d97721d2ea6c74a5cbcf404feb9004000000171600147f5a49a2d89f86ff498103bdbcb1762f9f0ab5aefeffffff08a04a05000000000017a91467a849785bb12f1a03260b3b52ec9dff52b30e708724b80100000000001976a914f0b7e8a5ede232c2654fca3b47f8d0c95f7a561688ac50fe0b00000000001976a914ed06524fb21248a37e7f8f20cdc67e27f49c4f1588acf44f01000000000017a9147422dbf3bdf441a640d09a2a91e6fa40837b019f878d9b3100000000001976a9142a5cd9333e4e94c88f7cd33216b2fc0af15fbd8588ac89b400000000000017a914d05dbcdb4587f1a243cc1ed89e96cd4ec19e24a08778e169180000000017a914f520df8923419f60d9a76df60cfc7a0e3a3fc86f8708d806000000000017a9147246c684dd4890e9c8977e6312c30ecdea2b0a55870247304402205704dd3b6d8930e89c660011ab2f6a42aedcbde438d5f121998e3db6a66889310220054a506aee274f885229d6a9ed89eca17bac87dca401f9c63d07990e98b2f0a301210390e3b178ebc51da1589baee9de37de491dba0575838aacf88c2cd70a06a4486eb47c0a00

real	0m0.003s
user	0m0.002s
sys	0m0.000s

jonatack · 2021-07-13T14:11:54Z

I cherry-picked this change onto #22437 that adds a lot of GetTransaction() coverage and it doesn't see any change in behavior; all is green.

mjdietzx

ACK

Based on the issue being clarified and some measurements being provided after this PR, and tests in #22437 I'm signing-off on this. I do think this PR would have been a good opportunity to benchmark performance with a few different test cases (instead of times on just one transaction that @orweinberger provided), but that's just my opinion. Don't want to hold things up bc I'm convinced this is a good optimization

mjdietzx · 2021-07-06T00:59:19Z

src/rpc/rawtransaction.cpp

-                "is in a block in the blockchain.\n"
-
+                "\nBy default, this call only returns a transaction if it is in the mempool. If -txindex is enabled\n"
+                "and no blockhash argument is passed, it will return the transaction if it is in the mempool or any block.\n"


nit: there is the case where -txindex is enabled and a blockhash argument is passed, where it will return the transaction only if the block it's found in matches the blockhash argument passed

theStack

Code review ACK 78f4c8b

Using -txindex if available makes perfect sense, regardless of whether a blockhash has been passed or not. To my understanding, the PR in its current form doesn't change any behaviour (in particular, if the passed blockhash doesn't match the one from the index, still the slower path of reading the block from disk is taken), i.e. the initial objections by reviewers (#22383 (comment), #22383 (review)) have been sorted out. Follow-up PR #22437 should further increase the confidence that this is the case, will also put that on my review list.
Also, the structure of GetTransaction looks more logical now, as the code blocks for getting a transaction appear in the order of "fast" (mempool) to "slow" (read block from disk).

LarryRuane

tACK 78f4c8b
suggestions are optional

LarryRuane · 2021-07-21T01:13:15Z

src/validation.h

 *
 * @param[in]  block_index     The block to read from disk, or nullptr
- * @param[in]  mempool         If block_index is not provided, look in the mempool, if provided
+ * @param[in]  mempool         If provided, check mempool for tx


Suggested change

* @param[in] mempool If provided, check mempool for tx

* @param[in] mempool If provided and block_index is not, check mempool for tx

I'd suggest just replacing this with Optional pointer to the node's mempool to avoid duplicating what's already documented above.

LarryRuane · 2021-07-21T11:43:57Z

src/validation.cpp

+            if (!block_index || block_index->GetBlockHash() == block_hash) {
+                hashBlock = block_hash;
+                return tx;
+            }


Suggested change

}

}

return nullptr;

Nit: This improves the performance of the (unlikely) case where a block hash is specified, but the transaction is found in the txindex but in a different block. The if at line 1170 would be false, so we would call ReadBlockFromDisk() below, but would not find the transaction there either (assuming the txindex is accurate).

I cherry-picked this PR with this suggestion onto #22437 as @jonatack described above, and the tests all pass.

How is this different from the initial solution: #22383 (comment)?

@LarryRuane the idea is to allow fetching the transaction from a block out of the active chain - which -txindex doesn't take into account. Before this was possible.

That could be checked by seeing if the specified block index is in the main chain.

But I suppose even that has edge cases (eg, index out of sync), so maybe it's better to just leave it alone.

luke-jr

utACK 78f4c8b

Echo @LarryRuane's suggestion here https://github.com/bitcoin/bitcoin/pull/22383/files#r673899065

jnewbery

utACK 78f4c8b

Couple of minor documentation suggestions inline.

jnewbery · 2021-07-22T10:59:49Z

src/rpc/rawtransaction.cpp

+                "If -txindex is not enabled and a blockhash argument is passed, it will return the transaction if\n"
+                "the specified block is available and the transaction is found in that block.\n"


I think the following change addresses @mjdietzx's comment: https://github.com/bitcoin/bitcoin/pull/22383/files#r664172064

Suggested change

"If -txindex is not enabled and a blockhash argument is passed, it will return the transaction if\n"

"the specified block is available and the transaction is found in that block.\n"

"If a blockhash argument is passed, it will return the transaction if\n"

"the specified block is available and the transaction is in that block.\n"

jnewbery · 2021-07-22T11:08:32Z

src/validation.cpp

+            if (!block_index || block_index->GetBlockHash() == block_hash) {
+                hashBlock = block_hash;


This could do with a comment:

Suggested change

if (!block_index || block_index->GetBlockHash() == block_hash) {

hashBlock = block_hash;

if (!block_index || block_index->GetBlockHash() == block_hash) {

// Don't return the transaction if the provided block hash doesn't match.

// The case where a transaction appears in multiple blocks (e.g. reorgs or

// BIP30) is handled by the block lookup below.

hashBlock = block_hash;

rajarshimaitra

Code review ACK 78f4c8b

Doc updates as mentioned by @jnewbery #22383 (comment) and #22383 (comment) seems appropriate.

Would test #22437 and report back..

lsilva01

Code Review ACK and Tested ACK 78f4c8b on Ubuntu 20.04

maflcko · 2021-07-22T18:15:33Z

Would be nice to include the documentation suggested by @jnewbery to avoid someone "optimizing" this again in the future. Maybe #22528, which also moves the code, can be used for that?

maflcko · 2021-07-22T18:33:05Z

src/validation.cpp


 CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock)
 {
    LOCK(cs_main);


Just realized after merge that cs_main is not needed to read from the mempool (only to write to it)

…ransaction 78f4c8b prefer to use txindex if available for GetTransaction (Jameson Lopp) Pull request description: Fixes bitcoin#22382 Motivation: prevent excessive disk reads if txindex is enabled. Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen? ACKs for top commit: jonatack: ACK 78f4c8b theStack: Code review ACK 78f4c8b LarryRuane: tACK 78f4c8b luke-jr: utACK 78f4c8b jnewbery: utACK 78f4c8b rajarshimaitra: Code review ACK bitcoin@78f4c8b lsilva01: Code Review ACK and Tested ACK bitcoin@78f4c8b on Ubuntu 20.04 Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0

f685a13 doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery) abc57e1 refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner) Pull request description: ~This PR is based on #22383, which should be reviewed first~ (merged by now). In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background. Relevant IRC log: ``` 17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it. 17:53 <raj_> jnewbery, +1 17:53 <stickies-v> agreed! 17:54 <glozow> jnewbery ya 17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex 17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :) 17:55 <sipa> (before 0.8, validation itself used the txindex) 17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp) 17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things. 17:55 <sipa> jnewbery: sure, just providing background 17:56 <sipa> seems very reasonable to move it elsewhere now ``` The commit should be trivial to review with `--color-moved`. ACKs for top commit: jnewbery: Code review ACK f685a13 rajarshimaitra: tACK f685a13 mjdietzx: crACK f685a13 LarryRuane: Code review, test ACK f685a13 Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c

NelsonGaldeman · 2021-08-02T12:10:22Z

I was curious about response times so I ran some tests:

I picked up 5 random transactions and fetched them with and without the block hash against both latest release (v0.21.1) and 78f4c8b

v0.21.1 results

nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 278e5e132279ef4bbb0a612cb6b27d8b7510c2cc3be432b0641841cf56a7b0d5
[...]

real	0m0.118s
user	0m0.000s
sys	0m0.006s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 278e5e132279ef4bbb0a612cb6b27d8b7510c2cc3be432b0641841cf56a7b0d5 false 0000000000000000000611dd7a5bde18744ebd17dd0ee71123bb31c5a500f9c0
[...]

real	0m0.133s
user	0m0.005s
sys	0m0.001s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 2ad5c8d13cc87293145eefbf4f6b4ee22859c75006266b335248ae402d1495f8
[...]

real	0m0.117s
user	0m0.006s
sys	0m0.001s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 2ad5c8d13cc87293145eefbf4f6b4ee22859c75006266b335248ae402d1495f8 false 00000000000000000070de9aec218ad077a877f8061cf990ab321dbd2753bfbc
[...]

real	0m0.043s
user	0m0.006s
sys	0m0.000s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction b9d34e525aa764d7043abb2c83d51394ada5162d6c1a352047bc388de8845e4b false 000000000000000006cd4488b5c44d79983f466fc2e7821cb309e0523dee0e52
[...]

real	0m0.090s
user	0m0.000s
sys	0m0.006s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction b9d34e525aa764d7043abb2c83d51394ada5162d6c1a352047bc388de8845e4b
[...]

real	0m0.112s
user	0m0.000s
sys	0m0.005s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction c2def6c34dd508a36776ac76eab06391f1ce90fdb142355b6ddd4fdb90b60f24 false 00000000000004fa9ea9acff9060083ea60f9377589e6797bb132484263f4b7c
[...]

real	0m0.021s
user	0m0.003s
sys	0m0.004s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction c2def6c34dd508a36776ac76eab06391f1ce90fdb142355b6ddd4fdb90b60f24
[...]

real	0m0.102s
user	0m0.005s
sys	0m0.001s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 2722e08668a07b515d57a80922c2348c6a857f9afd1fa456851d489edc39b9e6
[...]

real	0m0.154s
user	0m0.000s
sys	0m0.006s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli  getrawtransaction 2722e08668a07b515d57a80922c2348c6a857f9afd1fa456851d489edc39b9e6 false 00000000000536414be8da7f69059a812bab7623ebd25abd779d1d24b06110fc
[...]

real	0m0.006s
user	0m0.002s
sys	0m0.003s

78f4c8b results

nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 278e5e132279ef4bbb0a612cb6b27d8b7510c2cc3be432b0641841cf56a7b0d5
[...]

real	0m0.095s
user	0m0.001s
sys	0m0.006s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 278e5e132279ef4bbb0a612cb6b27d8b7510c2cc3be432b0641841cf56a7b0d5 false 0000000000000000000611dd7a5bde18744ebd17dd0ee71123bb31c5a500f9c0
[...]

real	0m0.007s
user	0m0.002s
sys	0m0.005s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 2ad5c8d13cc87293145eefbf4f6b4ee22859c75006266b335248ae402d1495f8
[...]

real	0m0.124s
user	0m0.001s
sys	0m0.006s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 2ad5c8d13cc87293145eefbf4f6b4ee22859c75006266b335248ae402d1495f8 false 00000000000000000070de9aec218ad077a877f8061cf990ab321dbd2753bfbc
[...]
real	0m0.009s
user	0m0.000s
sys	0m0.007s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction b9d34e525aa764d7043abb2c83d51394ada5162d6c1a352047bc388de8845e4b false 000000000000000006cd4488b5c44d79983f466fc2e7821cb309e0523dee0e52
[...]

real	0m0.153s
user	0m0.003s
sys	0m0.004s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction b9d34e525aa764d7043abb2c83d51394ada5162d6c1a352047bc388de8845e4b
[...]

real	0m0.006s
user	0m0.006s
sys	0m0.000s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction c2def6c34dd508a36776ac76eab06391f1ce90fdb142355b6ddd4fdb90b60f24 false 00000000000004fa9ea9acff9060083ea60f9377589e6797bb132484263f4b7c
[...]

real	0m0.138s
user	0m0.000s
sys	0m0.006s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction c2def6c34dd508a36776ac76eab06391f1ce90fdb142355b6ddd4fdb90b60f24
[...]

real	0m0.006s
user	0m0.000s
sys	0m0.006s



nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 2722e08668a07b515d57a80922c2348c6a857f9afd1fa456851d489edc39b9e6
[...]

real	0m0.158s
user	0m0.000s
sys	0m0.005s
nelson@bitcoin-node-v3:~$ time ./bitcoin-cli getrawtransaction 2722e08668a07b515d57a80922c2348c6a857f9afd1fa456851d489edc39b9e6 false 00000000000536414be8da7f69059a812bab7623ebd25abd779d1d24b06110fc
[...]

real	0m0.006s
user	0m0.005s
sys	0m0.000s

System information
GCP n1-standard-2
2 vCPUs
7.5 GB memory
HDD Standard Disk

… lock acquire 4a1b2a7 [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner) Pull request description: This PR is a follow-up to #22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](bitcoin/bitcoin#22383 (comment))): https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.h#L554-L558 `CTxMemPool::get()` acquires this lock: https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.cpp#L822-L829 so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked. ACKs for top commit: tryphe: Concept ACK. tested 4a1b2a7 but not extensively. jnewbery: Code review ACK 4a1b2a7 Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c

…quire 4a1b2a7 [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner) Pull request description: This PR is a follow-up to bitcoin#22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](bitcoin#22383 (comment))): https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.h#L554-L558 `CTxMemPool::get()` acquires this lock: https://github.com/bitcoin/bitcoin/blob/b620b2d58a55a88ad21da70cb2000863ef17b651/src/txmempool.cpp#L822-L829 so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked. ACKs for top commit: tryphe: Concept ACK. tested 4a1b2a7 but not extensively. jnewbery: Code review ACK 4a1b2a7 Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c

…erage, improve rpc_rawtransaction 387355b test, refactor: rpc_rawtransaction PEP8 (Jon Atack) 7d5cec2 refactor: separate the rpc_rawtransaction tests into functions (Jon Atack) 409779d move-only: regroup similar rpc_rawtransaction tests together (Jon Atack) d861040 test: remove no longer needed (ASCII art) comments (Jon Atack) 14398b3 test: add and harmonize getrawtransaction logging (Jon Atack) 85d8869 test: run 2nd getrawtransaction section with/without -txindex (Jon Atack) 0097740 refactor: txid to constant in rpc_rawtransaction to isolate tests (Jon Atack) 8c19d13 refactor: dedup/reorg createrawtransaction sequence number tests (Jon Atack) 7f07359 Test src/node/transaction::GetTransaction() without -txindex (Jon Atack) Pull request description: Following up on bitcoin/bitcoin#22383 (review), this pull adds missing `src/node/transaction::GetTransaction()` test coverage for combinations of `-txindex` and `blockhash` and does some refactoring of the test file. ACKs for top commit: mjdietzx: reACK 387355b josibake: reACK bitcoin/bitcoin@387355b MarcoFalke: Approach ACK 387355b 🔆 Tree-SHA512: b47c4ff87d69c61434e5729c954b338bc13744eddaba0879ca9f5f42243ba2cb4640d94c5f74de9f2735a8bf5e66b3d1c3bd3b7c26cd7324da7d3270ce87c6fd

Summary: ``` Motivation: prevent excessive disk reads if txindex is enabled. ``` This will also benefit to `isfinaltransaction`. Backport of [[bitcoin/bitcoin#22383 | core#22383]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12208

fanquake added the Validation label Jul 1, 2021

jlopp force-pushed the GetTransactionPerformance branch from 2a3fe2a to dabd776 Compare July 1, 2021 18:29

jlopp force-pushed the GetTransactionPerformance branch from dabd776 to 5ab22ef Compare July 1, 2021 21:12

promag reviewed Jul 2, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

jlopp force-pushed the GetTransactionPerformance branch from 5ab22ef to 6dd28e7 Compare July 2, 2021 12:55

promag reviewed Jul 2, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jlopp force-pushed the GetTransactionPerformance branch from 6dd28e7 to 5dd11ea Compare July 2, 2021 20:51

promag reviewed Jul 2, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

maflcko changed the title ~~prefer to use txindex if available for GetTransaction~~ rpc: Prefer to use txindex if available for GetTransaction Jul 3, 2021

maflcko added the RPC/REST/ZMQ label Jul 3, 2021

jonatack reviewed Jul 3, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

prefer to use txindex if available for GetTransaction

78f4c8b

Fixes bitcoin#22382

jlopp force-pushed the GetTransactionPerformance branch from 5dd11ea to 78f4c8b Compare July 3, 2021 11:31

jonatack mentioned this pull request Jul 13, 2021

test, refactor: add GetTransaction() coverage, improve rpc_rawtransaction #22437

Merged

mjdietzx reviewed Jul 14, 2021

View reviewed changes

theStack approved these changes Jul 18, 2021

View reviewed changes

LarryRuane approved these changes Jul 21, 2021

View reviewed changes

LarryRuane reviewed Jul 21, 2021

View reviewed changes

luke-jr approved these changes Jul 22, 2021

View reviewed changes

jnewbery reviewed Jul 22, 2021

View reviewed changes

rajarshimaitra reviewed Jul 22, 2021

View reviewed changes

theStack mentioned this pull request Jul 22, 2021

refactor: move GetTransaction to node/transaction.cpp #22528

Merged

lsilva01 approved these changes Jul 22, 2021

View reviewed changes

maflcko merged commit 7925f3a into bitcoin:master Jul 22, 2021

maflcko reviewed Jul 22, 2021

View reviewed changes

theStack pushed a commit to theStack/bitcoin that referenced this pull request Jul 22, 2021

doc: GetTransaction()/getrawtransaction follow-ups to bitcoin#22383

f685a13

Pantamis mentioned this pull request Jul 31, 2021

Speeding up getrawtransaction when txindex=1 romanz/electrs#451

Closed

theStack mentioned this pull request Aug 2, 2021

[GetTransaction] remove unneeded cs_main lock acquire #22609

Merged

jlopp deleted the GetTransactionPerformance branch September 9, 2021 15:49

promag mentioned this pull request Sep 17, 2021

GetTransaction should only use TxIndex if -txindex is set #15293

Closed

romanz mentioned this pull request Sep 18, 2021

Check txindex and use blockhash if disabled romanz/electrs#452

Closed

romanz mentioned this pull request Oct 11, 2021

0.9.0 Performance on rPi romanz/electrs#539

Closed

bitcoin locked and limited conversation to collaborators Oct 30, 2022

	* @param[in] mempool If provided, check mempool for tx
	* @param[in] mempool If provided and block_index is not, check mempool for tx

		"If -txindex is not enabled and a blockhash argument is passed, it will return the transaction if\n"
		"the specified block is available and the transaction is found in that block.\n"

		if (!block_index \|\| block_index->GetBlockHash() == block_hash) {
		hashBlock = block_hash;

rpc: Prefer to use txindex if available for GetTransaction #22383

rpc: Prefer to use txindex if available for GetTransaction #22383

Uh oh!

Conversation

jlopp commented Jul 1, 2021

Uh oh!

murchandamus commented Jul 1, 2021

Uh oh!

mjdietzx commented Jul 1, 2021

Uh oh!

jlopp commented Jul 1, 2021

Uh oh!

sipa commented Jul 1, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jlopp commented Jul 2, 2021

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jul 3, 2021

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jul 3, 2021

Uh oh!

theStack commented Jul 3, 2021

Uh oh!

mjdietzx commented Jul 6, 2021

Uh oh!

promag commented Jul 6, 2021

Uh oh!

mjdietzx commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlopp commented Jul 6, 2021

Uh oh!

promag commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orweinberger commented Jul 13, 2021

Uh oh!

jonatack commented Jul 13, 2021

Uh oh!

mjdietzx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane left a comment

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!

LarryRuane Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment •

edited

Loading

mjdietzx commented Jul 6, 2021 •

edited

Loading

promag commented Jul 6, 2021 •

edited

Loading

LarryRuane Jul 21, 2021 •

edited

Loading

NelsonGaldeman commented Aug 2, 2021 •

edited

Loading