[rpc] Allow fetching tx directly from specified block in getrawtransaction #10275

kallewoof · 2017-04-25T08:53:39Z

[Reviewer hint: use ?w=1 to avoid seeing a bunch of indentation changes.]

Presuming a user knows the block hash of the block containing a given transaction, this PR allows them to fetch the raw transaction, even without -txindex. It also enables support for getting transactions that are in orphaned blocks.

Note that supplying a block hash will override mempool and txindex support in GetTransaction. The rationale behind this is that a transaction may be in multiple places (orphaned blocks) and if the user supplies an explicit block hash it should be adhered to.

$ # a41.. is a tx inside an orphan block ..3c6f.. -- first try getting it normally
$ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1
error code: -5
error message:
No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions.
$ # now try with block hash
$ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 0000000000000000003c6fe479122bfa4a9187493937af1734e1e5cd9f198ec7
{
  "hex": "01000000014e7e81144e42f6d65550e59b715d470c9301fd7ac189[...]90488ac00000000",
  "inMainChain": false,
  "txid": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79",
  "hash": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79",
  "size": 225,
[...]
}
$ # another tx 6c66... in block 462000
$ ./bitcoin-cli getrawtransaction 6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735 1 00000000000000000217f2c12922e321f6d4aa933ce88005a9a493c503054a40
{
  "hex": "0200000004d157[...]88acaf0c0700",
  "inMainChain": true,
  "txid": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735",
  "hash": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735",
  "size": 666,
[...]
}
$

laanwj · 2017-04-25T11:47:12Z

src/rpc/rawtransaction.cpp

This functionality could come in useful.

As for the API I prefer to not do any string combining/parsing here, this makes the API less clean to work with at least in my experience. I'd prefer to add an optional (can be null or missing) fromblock argument.

gmaxwell

Concept ACK. I've really wanted this before.

Allowing it work on orphan blocks is an interesting idea. I'm a little less sure about that-- I think it could allow it to return transactions that have never been validated, which would be somewhat surprising.

jonasschnelli · 2017-04-25T19:37:42Z

Nice feature!
I agree with @laanwj about the string parsing. The <block>:<txid> schematics looks good at first sight, but we are using JSON and should stay with JSON and don't add another form or key/value encoding.

kallewoof · 2017-04-26T04:24:29Z

Fair enough, I'll add a blockhash argument instead. I was kind of toying with the idea of a new standard for referencing transactions which included the block height (not hash) so everyone could always find a tx presuming they had the block in question, and that thought sort of seeped in here.

kallewoof · 2017-04-26T05:12:44Z

@gmaxwell

Allowing it work on orphan blocks is an interesting idea. I'm a little less sure about that-- I think it could allow it to return transactions that have never been validated, which would be somewhat surprising.

I just realized my logic was flawed on this. I am passing the block height only to the GetTransaction method, which means it will always pick the active chain at the given height. Either I throw when the block is not in the main chain (i.e. no support for orphaned blocks) or I move the height determine logic over to validation.cpp. I am leaning towards the latter, but feedback welcome.

Edit: passing CBlockIndex to GetTransaction seems like a great way to do this. Going with that.

kallewoof · 2017-04-26T05:44:27Z

The code now works as advertised (see updated OP).

History:

2017-04-25: 1 (block hash support in getrawtx)
2017-04-26: → 2⊱1 (bh:txid to 2 params) → 3 (fix fVerbose parsing) → 4⊱1 (switch to block index in GetTx)

jnewbery · 2017-05-02T21:26:44Z

src/rpc/rawtransaction.cpp

nit: You can replace this and the next three lines with:

} else { fVerbose = (request.params[1].get_bool()); }

since get_bool() does the type testing for you and throws the JSONRPCError if the type isn't a VBOOL.

Up to you whether you think that's clearer.

Ohh, good point! Thanks.
Edit: I don't want it to throw for null values though.

So I end up with

if (request.params[1].isNum()) { fVerbose = (request.params[1].get_int() != 0); } else if (!request.params[1].isNull()) { fVerbose = (request.params[1].get_bool()); }

jnewbery · 2017-05-02T21:27:19Z

utACK, but I think this deserves a new functional test case.

kallewoof · 2017-05-04T13:31:16Z

@jnewbery I agree. Will get to work on that.

[...]:

2017-05-04: 5⊱2 (improve fVerbose)

kallewoof · 2017-05-08T04:28:06Z

@jnewbery Added some tests to rawtransactions.py for the included blockhash variant (a6b8461).

[...]:

2017-05-08: 6 (tests)

jnewbery

tested ACK the integration test in a6b84618ad04dccc628aa4905fced4784a62895a with a couple of nits.

jnewbery · 2017-05-15T19:28:59Z

test/functional/rawtransactions.py

nit: no need for brackets here. The following should do:

block1, block2 = self.nodes[2].generate(2)

jnewbery · 2017-05-15T19:30:21Z

test/functional/rawtransactions.py

nit: I think it's better to assert on the actual value here (ie verify that the getrawtransaction returned the correct transaction rather than returned anything). The following should do that:

assert_equal(self.nodes[0].getrawtransaction(tx, True, block1)['txid'], tx)

@jnewbery Good points, thanks. Addressed!

sipa · 2017-05-15T22:32:21Z

Is this still needed after #8704?

kallewoof · 2017-05-16T02:41:35Z

@sipa The use cases are quite different I think. #8704 lets you see all transactions in a given block. This lets you grab a transaction directly from a block without indexing.

sipa · 2017-05-16T02:42:06Z

@kallewoof #8704 does not require indexing either

kallewoof · 2017-05-16T02:56:41Z

Yeah, sorry, I meant that this is a way to get a specific transaction if you know the block hash, whereas #8704 shows you all transactions in the entire block. You get the info, but you have to wade through stuff to get it.

laanwj · 2017-05-16T05:25:08Z

In both cases the whole block has to be loaded from disk, and parsed, and searched linearly. The difference is whether the linear search step happens on the server or client.

I think both #8704 and this can be useful, but have to agree there's only superficial difference.

maaku · 2017-05-16T08:32:10Z

The difference between the two is the serialization and transmission and parsing of ~5MB of JSON data vs a few hundred bytes of hex encoded data. That's a 1,000x difference on the client side, and the same absolute improvement on the server -- although as a multiplier it'd be less since as you note the server still has to parse the block from disk. That's a nontrivial performance difference.

(Also, this would have greatly helped me in the past so another +1 from me.)

jnewbery · 2017-05-16T13:31:07Z

Agree with @maaku - simply encoding the data into 5MB of json could be time-consuming, during which time the cs_main lock is held. Having a command to return just a single transaction from a block seems very useful.

luke-jr · 2017-06-03T22:11:33Z

src/rpc/rawtransaction.cpp

Should probably mention that it MUST be in that block in this case...

luke-jr · 2017-06-03T22:13:56Z

src/rpc/rawtransaction.cpp

Remove "in chain". Maybe "in database"?

luke-jr · 2017-06-03T22:14:43Z

src/rpc/rawtransaction.cpp

Whether the block specified is in the main chain or not... This could be false with the tx being still in the main chain!

…nsaction from a block directly. Github-Pull: bitcoin#10275 Rebased-From: 4d006e785454e20cf0000b04e175d5e142b12e3f

… accepted). Github-Pull: bitcoin#10275 Rebased-From: 5b389a88b0e9ab92f103f78c9b063f678e8afa2f

TheBlueMatt · 2017-12-05T18:29:53Z

utACK 434526a

maflcko

utACK 434526a

maflcko · 2017-12-05T23:44:53Z

src/rpc/rawtransaction.cpp


    LOCK(cs_main);

+    bool in_active_chain = true;


~~nit: Should not be initialized, to aid static analysers.~~

Strike that. gcc would throw a warning.

maflcko · 2017-12-06T02:22:20Z

Sorry for the sloppy review. I messed up the parameter order when testing. Also gcc complains about my nit-fix.

Tested ACK now:

maflcko · 2017-12-06T03:30:38Z

@jtimon @jnewbery
Mind to re-ACK?

jonasschnelli · 2017-12-06T07:09:43Z

utACK 434526a

…n getrawtransaction 434526a [test] Add tests for getrawtransaction with block hash. (Karl-Johan Alm) b167951 [rpc] Allow getrawtransaction to take optional blockhash to fetch transaction from a block directly. (Karl-Johan Alm) a5f5a2c [rpc] Fix fVerbose parsing (remove excess if cases). (Karl-Johan Alm) Pull request description: [Reviewer hint: use [?w=1](https://github.com/bitcoin/bitcoin/pull/10275/files?w=1) to avoid seeing a bunch of indentation changes.] Presuming a user knows the block hash of the block containing a given transaction, this PR allows them to fetch the raw transaction, even without `-txindex`. It also enables support for getting transactions that are in orphaned blocks. Note that supplying a block hash will override mempool and txindex support in `GetTransaction`. The rationale behind this is that a transaction may be in multiple places (orphaned blocks) and if the user supplies an explicit block hash it should be adhered to. ```Bash $ # a41.. is a tx inside an orphan block ..3c6f.. -- first try getting it normally $ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 error code: -5 error message: No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions. $ # now try with block hash $ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 0000000000000000003c6fe479122bfa4a9187493937af1734e1e5cd9f198ec7 { "hex": "01000000014e7e81144e42f6d65550e59b715d470c9301fd7ac189[...]90488ac00000000", "inMainChain": false, "txid": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79", "hash": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79", "size": 225, [...] } $ # another tx 6c66... in block 462000 $ ./bitcoin-cli getrawtransaction 6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735 1 00000000000000000217f2c12922e321f6d4aa933ce88005a9a493c503054a40 { "hex": "0200000004d157[...]88acaf0c0700", "inMainChain": true, "txid": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735", "hash": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735", "size": 666, [...] } $ ``` Tree-SHA512: 279be3818141edd3cc194a9ee65929331920afb30297ab2d6da07293a2d7311afee5c8b00c6457477d9f1f86e86786a9b56878ea3ee19fa2629b829d042d0cda

jnewbery

Post-merge tested ACK. I have one style nit, which I can open a tidy-up PR for if people agree.

jnewbery · 2017-12-06T22:22:18Z

src/rpc/rawtransaction.cpp

+    }
+
+    if (!request.params[2].isNull()) {
+        uint256 blockhash = ParseHashV(request.params[2], "parameter 3");


Nit: these lines are confusing for me:

uint256 blockhash = ParseHashV(request.params[2], "parameter 3"); if (!blockhash.IsNull()) {

ParseHashV() can either throw or return a uint256. The only way it'll return a null uint256 is if the input is a 64 length string of 0s. That can't be a valid block (if someone finds a block that hashes to zero then they win Bitcoin The Game and we can all stop playing), so I think that this if condition is unnecessary - if parameter 3 is all zeroes, we should throw with the error that 0x0 is an invalid block.

I don't think this is a huge problem - if all zeroes is supplied as the block hash, we'll just drop through and search for the transaction in the {mempool , blockchain (if fTxIndex) , UTXO set} as if no block hash had been provided. However, it's a bit confusing for the code reader since it implies to me that we're expecting ParseHashV() to return a null value if it fails to parse the hash.

fa4c16d qa: Add getrawtransaction in_active_chain=False test (MarcoFalke) Pull request description: #10275 accidentally forgot to add a test for `in_active_chain==False`. This adds a test and also removes the special casing of `blockhash.IsNull()`, which makes no sense imo. Tree-SHA512: 6c51295820b3dcd53b0b48020ab2b8c8f5864cd5061ddab2b35d35d643eb3e60ef95ff20c06c985a2e47f7080e82f27f3e00ee61c85dce627776d5ea6febee8f

promag · 2018-02-22T01:24:47Z

For reference, This was proposed (and closed) in #2421.

kallewoof · 2018-02-22T04:23:09Z

@promag I believe the concerns raised in #2421 are not relevant anymore / in this implementation.

…ction (#1612) bitcoin/bitcoin#10275 bitcoin/bitcoin#9042 `GetTransaction` changes are easier to review ignoring whitespace changes (add `?w=1` to the PR's URL)

…block in getrawtransaction 434526a [test] Add tests for getrawtransaction with block hash. (Karl-Johan Alm) b167951 [rpc] Allow getrawtransaction to take optional blockhash to fetch transaction from a block directly. (Karl-Johan Alm) a5f5a2c [rpc] Fix fVerbose parsing (remove excess if cases). (Karl-Johan Alm) Pull request description: [Reviewer hint: use [?w=1](https://github.com/bitcoin/bitcoin/pull/10275/files?w=1) to avoid seeing a bunch of indentation changes.] Presuming a user knows the block hash of the block containing a given transaction, this PR allows them to fetch the raw transaction, even without `-txindex`. It also enables support for getting transactions that are in orphaned blocks. Note that supplying a block hash will override mempool and txindex support in `GetTransaction`. The rationale behind this is that a transaction may be in multiple places (orphaned blocks) and if the user supplies an explicit block hash it should be adhered to. ```Bash $ # a41.. is a tx inside an orphan block ..3c6f.. -- first try getting it normally $ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 error code: -5 error message: No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions. $ # now try with block hash $ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 0000000000000000003c6fe479122bfa4a9187493937af1734e1e5cd9f198ec7 { "hex": "01000000014e7e81144e42f6d65550e59b715d470c9301fd7ac189[...]90488ac00000000", "inMainChain": false, "txid": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79", "hash": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79", "size": 225, [...] } $ # another tx 6c66... in block 462000 $ ./bitcoin-cli getrawtransaction 6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735 1 00000000000000000217f2c12922e321f6d4aa933ce88005a9a493c503054a40 { "hex": "0200000004d157[...]88acaf0c0700", "inMainChain": true, "txid": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735", "hash": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735", "size": 666, [...] } $ ``` Tree-SHA512: 279be3818141edd3cc194a9ee65929331920afb30297ab2d6da07293a2d7311afee5c8b00c6457477d9f1f86e86786a9b56878ea3ee19fa2629b829d042d0cda

…test fa4c16d qa: Add getrawtransaction in_active_chain=False test (MarcoFalke) Pull request description: bitcoin#10275 accidentally forgot to add a test for `in_active_chain==False`. This adds a test and also removes the special casing of `blockhash.IsNull()`, which makes no sense imo. Tree-SHA512: 6c51295820b3dcd53b0b48020ab2b8c8f5864cd5061ddab2b35d35d643eb3e60ef95ff20c06c985a2e47f7080e82f27f3e00ee61c85dce627776d5ea6febee8f

Add blockhash parameter to getrawtransaction Code ported manually from bitcoin/bitcoin#10275 in attempt to fix #4475

3ffa282 [RPC] Update getrawtransaction warning message (random-zebra) Pull request description: bitcoin#9520+bitcoin#10275 have already been ported (back in #812), but the warning message in `getrawtransaction` help has not been updated yet. ACKs for top commit: furszy: utACK 3ffa282 Fuzzbawls: utACK 3ffa282 Tree-SHA512: c406b4cf691517919c9911915737e9b97dcde5959856889ed5b6a501bd5073dfe9179e0663d25f34ccb2d536755e5d153b1362aecc81e445d604e65b74ac797f

fanquake added the RPC/REST/ZMQ label Apr 25, 2017

laanwj reviewed Apr 25, 2017

View reviewed changes

gmaxwell reviewed Apr 25, 2017

View reviewed changes

kallewoof force-pushed the gettx-with-blockhash branch from dd3da7e to 456d6f8 Compare April 26, 2017 05:42

kallewoof force-pushed the gettx-with-blockhash branch 4 times, most recently from 91f783d to 23be83e Compare May 1, 2017 06:30

jnewbery reviewed May 2, 2017

View reviewed changes

kallewoof force-pushed the gettx-with-blockhash branch from 23be83e to ccb969a Compare May 4, 2017 13:28

kallewoof force-pushed the gettx-with-blockhash branch from 5b9b2e4 to 9f980f6 Compare May 6, 2017 03:49

jnewbery reviewed May 15, 2017

View reviewed changes

jnewbery mentioned this pull request May 23, 2017

RPC: getblock returns coinbase scriptsig #10385

Closed

luke-jr suggested changes Jun 3, 2017

View reviewed changes

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

[rpc] Allow getrawtransaction to take optional blockhash to fetch tra…

e096099

…nsaction from a block directly. Github-Pull: bitcoin#10275 Rebased-From: 4d006e785454e20cf0000b04e175d5e142b12e3f

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

[rpc] Fix fVerbose parsing (remove excess if cases and ensure null is…

99dd1fc

… accepted). Github-Pull: bitcoin#10275 Rebased-From: 5b389a88b0e9ab92f103f78c9b063f678e8afa2f

maflcko reviewed Dec 6, 2017

View reviewed changes

laanwj merged commit 434526a into bitcoin:master Dec 6, 2017

maflcko mentioned this pull request Dec 6, 2017

qa: Add getrawtransaction in_active_chain=False test #11838

Merged

jnewbery reviewed Dec 6, 2017

View reviewed changes

kallewoof deleted the gettx-with-blockhash branch December 7, 2017 01:52

jonasschnelli mentioned this pull request Dec 7, 2017

TODO for release notes 0.16.0 #11054

Closed

jonasschnelli mentioned this pull request Jan 4, 2018

Feature request: pruned chain and getrawtransaction #10609

Closed

promag mentioned this pull request Feb 22, 2018

Getrawtransaction working partially without -txindex is confusing #3220

Closed

dagurval mentioned this pull request Mar 18, 2019

[rpc] Allow fetching tx directly from specified block in getrawtransaction BitcoinUnlimited/BitcoinUnlimited#1612

Merged

oxarbitrage mentioned this pull request Jun 15, 2020

Add blockhash parameter to getrawtransaction zcash/zcash#4574

Merged

SomberNight mentioned this pull request Nov 9, 2020

change db schema, allow TXO-spender lookups, protocol 1.5 spesmilo/electrumx#80

Closed

zkbot added a commit to zcash/zcash that referenced this pull request Nov 10, 2020

Auto merge of #4574 - oxarbitrage:issue4475, r=daira

60331b9

Add blockhash parameter to getrawtransaction Code ported manually from bitcoin/bitcoin#10275 in attempt to fix #4475

random-zebra mentioned this pull request May 9, 2021

[Trivial][RPC] Update getrawtransaction warning message PIVX-Project/PIVX#2375

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[rpc] Allow fetching tx directly from specified block in getrawtransaction #10275

[rpc] Allow fetching tx directly from specified block in getrawtransaction #10275

Uh oh!

Conversation

kallewoof commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Apr 25, 2017 • 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!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 25, 2017

Uh oh!

kallewoof commented Apr 26, 2017

Uh oh!

kallewoof commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof May 4, 2017 • 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!

jnewbery commented May 2, 2017

Uh oh!

kallewoof commented May 4, 2017

Uh oh!

kallewoof commented May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery 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!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 15, 2017

Uh oh!

kallewoof commented May 16, 2017

Uh oh!

sipa commented May 16, 2017

Uh oh!

kallewoof commented May 16, 2017

Uh oh!

laanwj commented May 16, 2017

Uh oh!

maaku commented May 16, 2017

Uh oh!

jnewbery commented May 16, 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!

TheBlueMatt commented Dec 5, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 5, 2017 • 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!

maflcko commented Dec 6, 2017

Uh oh!

maflcko commented Dec 6, 2017

kallewoof commented Apr 25, 2017 •

edited

Loading

laanwj Apr 25, 2017 •

edited

Loading

kallewoof commented Apr 26, 2017 •

edited

Loading

kallewoof commented Apr 26, 2017 •

edited

Loading

kallewoof May 4, 2017 •

edited

Loading

kallewoof commented May 8, 2017 •

edited

Loading

maflcko Dec 5, 2017 •

edited

Loading