Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 30, 2019

Conversation

Projects
None yet
9 participants
@amitiuttarwar
Copy link
Contributor

commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

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

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.

src/rpc/rawtransaction.cpp Outdated
g_txindex->BlockUntilSyncedToCurrentChain();
}
// Allow txindex to catch up if we need to query it and before we acquire cs_main.
if (!pblockindex) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 14, 2019

Member

pblockindex == nullptr will always hold here, right?

Show resolved Hide resolved test/functional/wallet_basic.py Outdated
@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

Concept ACK

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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.

@jnewbery
Copy link
Member

left a comment

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

  • The PR description (#15159 (comment)) gets added to the commit log when the PR is merged (see 1cfbb16 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 " (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.
Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
@jnewbery

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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.

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:13931_rebase branch 4 times, most recently Jan 18, 2019

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

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

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

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:13931_rebase branch Jan 18, 2019

@jnewbery
Copy link
Member

left a comment

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).

Show resolved Hide resolved src/rpc/rawtransaction.cpp
Show resolved Hide resolved test/functional/rpc_txoutproof.py Outdated
Show resolved Hide resolved test/functional/feature_segwit.py

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:13931_rebase branch to 94af589 Jan 21, 2019

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

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

Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
Show resolved Hide resolved test/functional/feature_segwit.py

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:13931_rebase branch from 94af589 to 91ec543 Jan 24, 2019

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

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 :)

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

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

Show resolved Hide resolved doc/release-notes.md Outdated
Show resolved Hide resolved doc/release-notes.md Outdated
Show resolved Hide resolved src/validation.cpp Outdated

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:13931_rebase branch 2 times, most recently from e351a94 to 02021ec Jan 27, 2019

@amitiuttarwar amitiuttarwar force-pushed the amitiuttarwar:13931_rebase branch from 02021ec to 04da9f4 Jan 27, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

utACK 04da9f4

@promag

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

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.

@amitiuttarwar

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

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.
@promag

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

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

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

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.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

@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.

"argument, getrawtransaction will return the transaction if the specified block is available and\n"
"the transaction is found in that block. When called without a blockhash argument, getrawtransaction\n"
"will return the transaction if it is in the mempool, or if -txindex is enabled and the transaction\n"
"is in a block in the blockchain.\n"

"\nReturn the raw transaction data.\n"

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 30, 2019

Member

nit: Could move this up to the first line, since this is the oneline-summary of what the function does. The paragraph above is the implementation detail and interface documentation.
Feel free to ignore, since you dind't introduce the issue.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 30, 2019

Member

Similarly, you could mention the corresponding raw transaction getter rpcs for the mempool and wallet, but that is another perpendicular doc cleanup fix.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

utACK 04da9f4

@MarcoFalke MarcoFalke changed the title [RPC] Update getrawtransaction [RPC] Remove lookup to UTXO set from GetTransaction Jan 30, 2019

@@ -246,6 +246,12 @@ in the Low-level Changes section below.

- See the [Mining](#mining) section for changes to `getblocktemplate`.

- The `getrawtransaction` RPC no longer checks the unspent UTXO set for

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 30, 2019

Member

doc-nit: Change applies to rpc and rest

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 30, 2019

Merge bitcoin#15159: [RPC] Remove lookup to UTXO set from GetTransaction
04da9f4 [RPC] Update getrawtransaction interface (Amiti Uttarwar)

Pull request description:

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

Tree-SHA512: aa07353bccc14b81b7803992a25d076d6bc06d15ec7c1b85828dc10aea7e0498d9b49f71783e352ab8a14b0bb2010cfb7835de3dfd1bc6f2323f460449348e66

@MarcoFalke MarcoFalke merged commit 04da9f4 into bitcoin:master Jan 30, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@promag

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

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'

@amitiuttarwar amitiuttarwar deleted the amitiuttarwar:13931_rebase branch Jan 30, 2019

@Empact

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

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

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 1, 2019

Merge bitcoin#15247: qa: Use wallet to retrieve raw transactions
fa5278a qa: Use wallet to retrieve raw transactions (MarcoFalke)
fa21983 qa: Style-only fixes in touched files (MarcoFalke)

Pull request description:

  Instead of asking the coin database and block storage about a transaction, pull it directly from the wallet in wallet related tests.

  This refactoring only makes sense in light of bitcoin#15159.

  <sub>This product may contain minor stylistic cleanups

Tree-SHA512: ec34c7150d873da9f19fead3f7e3f758baba5ef10061942384c470a47a6f320690109be9c5160f0c8bc228272a729653d44c78471455337318f657d6c164ba23

@tynes tynes referenced this pull request Feb 5, 2019

Open

getrawtransaction changes #690

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 5, 2019

Merge bitcoin#15332: [Docs] Small updates to getrawtransaction descri…
…ption

4701239 [Docs] Small updates to getrawtransaction description (Amiti Uttarwar)

Pull request description:

  As per review comments on bitcoin#15159

Tree-SHA512: 0bbbe956b47d177f7e67c5ab2048287783327d9e07a679d64d79aee3ea8633e769f75b59d3dbce517924ba5d64d6c44f26bf49e16d40612463e460ad1a238129

laanwj added a commit that referenced this pull request Feb 27, 2019

Merge #15477: doc: Remove misleading hint in getrawtransaction
9999879 refactor: Use RPCHelpMan::IsValidNumArgs in getrawtransaction (MarcoFalke)
fa9ff8f doc: Remove misleading hint in getrawtransaction (MarcoFalke)

Pull request description:

  For 0.18.0

  I asked this line to be added in #15159, which was wrong because getmempoolentry does not return the raw transaction hex.

Tree-SHA512: 7ac85500c8192314347b7283cd369196bb959c124863642b6c1ce73d5662b1cbe4f42ded9c374dac6657458ab70b01810caf1235dd1d2b404bf376ebf09efa69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.