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] Allow fetching tx directly from specified block in getrawtransaction #10275

Merged
merged 3 commits into from Dec 6, 2017

Conversation

@kallewoof
Member

kallewoof commented Apr 25, 2017

[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,
[...]
}
$ 
Show outdated Hide outdated src/rpc/rawtransaction.cpp
uint256 hash;
int64_t blockHeight = -1;
bool inMainChain = true;
auto p = request.params[0].get_str().find(':');

This comment has been minimized.

@laanwj

laanwj Apr 25, 2017

Member

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.

@laanwj

laanwj Apr 25, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 25, 2017

Member

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.

Member

jonasschnelli commented Apr 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 26, 2017

Member

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.

Member

kallewoof commented Apr 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 26, 2017

Member

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

Member

kallewoof commented Apr 26, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Apr 26, 2017

Member

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

History:

Member

kallewoof commented Apr 26, 2017

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

History:

Show outdated Hide outdated src/rpc/rawtransaction.cpp
fVerbose = true;
}
fVerbose = (request.params[1].get_int() != 0);
} else if (request.params[1].isBool()) {

This comment has been minimized.

@jnewbery

jnewbery May 2, 2017

Member

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.

@jnewbery

jnewbery May 2, 2017

Member

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.

This comment has been minimized.

@kallewoof

kallewoof May 4, 2017

Member

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

@kallewoof

kallewoof May 4, 2017

Member

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

This comment has been minimized.

@kallewoof

kallewoof May 4, 2017

Member

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());
        }
@kallewoof

kallewoof May 4, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery May 2, 2017

Member

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

Member

jnewbery commented May 2, 2017

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

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof May 4, 2017

Member

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

[...]:

Member

kallewoof commented May 4, 2017

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

[...]:

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof May 8, 2017

Member

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

[...]:

Member

kallewoof commented May 8, 2017

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

[...]:

@jnewbery

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

Show outdated Hide outdated test/functional/rawtransactions.py
# make a tx by sending then generate 2 blocks; block1 has the tx in it,
# presumably
tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
[ block1, block2 ] = self.nodes[2].generate(2)

This comment has been minimized.

@jnewbery

jnewbery May 15, 2017

Member

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

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

@jnewbery

jnewbery May 15, 2017

Member

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

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

Show outdated Hide outdated test/functional/rawtransactions.py
[ block1, block2 ] = self.nodes[2].generate(2)
self.sync_all()
# We should be able to get the raw transaction by providing the correct block
assert self.nodes[0].getrawtransaction(tx, True, block1)

This comment has been minimized.

@jnewbery

jnewbery May 15, 2017

Member

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

jnewbery May 15, 2017

Member

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)

This comment has been minimized.

@kallewoof

kallewoof May 16, 2017

Member

@jnewbery Good points, thanks. Addressed!

@kallewoof

kallewoof May 16, 2017

Member

@jnewbery Good points, thanks. Addressed!

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 15, 2017

Member

Is this still needed after #8704?

Member

sipa commented May 15, 2017

Is this still needed after #8704?

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof May 16, 2017

Member

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

Member

kallewoof commented May 16, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 16, 2017

Member

@kallewoof #8704 does not require indexing either

Member

sipa commented May 16, 2017

@kallewoof #8704 does not require indexing either

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof May 16, 2017

Member

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.

Member

kallewoof commented May 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 16, 2017

Member

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.

Member

laanwj commented May 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@maaku

maaku May 16, 2017

Contributor

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

Contributor

maaku commented May 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery May 16, 2017

Member

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.

Member

jnewbery commented May 16, 2017

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.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
"\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
"enabled, it also works for blockchain transactions.\n"
"enabled, it also works for blockchain transactions. If the block hash is known, it can be provided\n"
"for nodes without -txindex.\n"

This comment has been minimized.

@luke-jr

luke-jr Jun 3, 2017

Member

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

@luke-jr

luke-jr Jun 3, 2017

Member

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

Show outdated Hide outdated src/rpc/rawtransaction.cpp
if (!blockHash.IsNull()) {
BlockMap::iterator it = mapBlockIndex.find(blockHash);
if (it == mapBlockIndex.end()) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found in chain");

This comment has been minimized.

@luke-jr

luke-jr Jun 3, 2017

Member

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

@luke-jr

luke-jr Jun 3, 2017

Member

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

Show outdated Hide outdated src/rpc/rawtransaction.cpp
"\nResult (if verbose is not set or set to false):\n"
"\"data\" (string) The serialized, hex-encoded data for 'txid'\n"
"\nResult (if verbose is set to true):\n"
"{\n"
" \"inMainChain\": b, (bool) Whether transaction is in the main chain or not. Only visible when specifying block hash\n"

This comment has been minimized.

@luke-jr

luke-jr Jun 3, 2017

Member

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

@luke-jr

luke-jr Jun 3, 2017

Member

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

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

[rpc] Allow getrawtransaction to take optional blockhash to fetch tra…
…nsaction from a block directly.

Github-Pull: #10275
Rebased-From: 4d006e7

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

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

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

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

f'test nits
Github-Pull: #10275
Rebased-From: 8f84e8c
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 3, 2017

Member

Suggested message fix on my gettx-with-blockhash-0.14 branch (cherry-pick).

Member

luke-jr commented Jun 3, 2017

Suggested message fix on my gettx-with-blockhash-0.14 branch (cherry-pick).

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 4, 2017

Member

Needs rebase.

Member

sipa commented Jun 4, 2017

Needs rebase.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jun 5, 2017

Member

@luke-jr Thanks for the review! I cherry-picked your commit.

[...]:

@sipa Rebased.

Member

kallewoof commented Jun 5, 2017

@luke-jr Thanks for the review! I cherry-picked your commit.

[...]:

@sipa Rebased.

@TheBlueMatt

You may wish to squash "[rpc] Fix fVerbose parsing (remove excess if cases and ensure null is…" and "[test] Updated rawtransactions.py to assert for adjusted exception.".
Generally we try to make sure that after each individual commit, at least it builds and all tests pass.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
}
if (request.params.size() > 2 && !request.params[2].isNull()) {
uint256 blockHash = ParseHashV(request.params[2], "parameter 3");

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

Hmm? Shouldn't we use the parameter's name here instead of "parameter 3"?

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

Hmm? Shouldn't we use the parameter's name here instead of "parameter 3"?

This comment has been minimized.

@kallewoof

kallewoof Jun 8, 2017

Member

The general tendency seems to be to identify the parameter index so I stuck with that. I agree it may be better to be more descriptive though...

@kallewoof

kallewoof Jun 8, 2017

Member

The general tendency seems to be to identify the parameter index so I stuck with that. I agree it may be better to be more descriptive though...

Show outdated Hide outdated src/rpc/rawtransaction.cpp
"\nResult (if verbose is not set or set to false):\n"
"\"data\" (string) The serialized, hex-encoded data for 'txid'\n"
"\nResult (if verbose is set to true):\n"
"{\n"
" \"inMainChain\": b, (bool) Whether specified block is in the main chain or not\n"

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

Hmm, maybe say "if blockhash is specified" or otherwise mention this wont appear unless a blockhash is provided. Even better, fill it out if GetTransaction returns the blockhash cause it found it via UTXO/txindex.

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

Hmm, maybe say "if blockhash is specified" or otherwise mention this wont appear unless a blockhash is provided. Even better, fill it out if GetTransaction returns the blockhash cause it found it via UTXO/txindex.

This comment has been minimized.

@kallewoof

kallewoof Jun 8, 2017

Member

I was sure I did, but guess not:

            "  \"inMainChain\": b,     (bool) Whether specified block is in the main chain or not (only present with explicit \"blockhash\" argument)\n"

I like the idea of including when able but will keep it out of this PR for now.

@kallewoof

kallewoof Jun 8, 2017

Member

I was sure I did, but guess not:

            "  \"inMainChain\": b,     (bool) Whether specified block is in the main chain or not (only present with explicit \"blockhash\" argument)\n"

I like the idea of including when able but will keep it out of this PR for now.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
"\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
"enabled, it also works for blockchain transactions.\n"
"enabled, it also works for blockchain transactions. If the block hash is known, it can be provided\n"
"for nodes without -txindex, in which case the transaction will only be found if it is in that\n"

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

grammar nit: this reads funny to me, and could be a bit more explicit. Maybe:

"If the block which contains the transaction is known, its hash can be provided even for nodes without -txindex."
"Note that if a blockhash is provided, only it will be searched and if the transaction is in mempool, other blocks, or if this node does not have the given block available, the transaction will not be found."

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

grammar nit: this reads funny to me, and could be a bit more explicit. Maybe:

"If the block which contains the transaction is known, its hash can be provided even for nodes without -txindex."
"Note that if a blockhash is provided, only it will be searched and if the transaction is in mempool, other blocks, or if this node does not have the given block available, the transaction will not be found."

This comment has been minimized.

@kallewoof

kallewoof Jun 8, 2017

Member

Thanks, that looks better yeah. Adding with minor tweaks.

            "\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
            "enabled, it also works for blockchain transactions. If the block which contains the transaction\n"
            "is known, its hash can be provided even for nodes without -txindex. Note that if a blockhash is\n"
            "provided, only that block will be searched and if the transaction is in the mempool or other\n"
            "blocks, or if this node does not have the given block available, the transaction will not be found.\n"
@kallewoof

kallewoof Jun 8, 2017

Member

Thanks, that looks better yeah. Adding with minor tweaks.

            "\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
            "enabled, it also works for blockchain transactions. If the block which contains the transaction\n"
            "is known, its hash can be provided even for nodes without -txindex. Note that if a blockhash is\n"
            "provided, only that block will be searched and if the transaction is in the mempool or other\n"
            "blocks, or if this node does not have the given block available, the transaction will not be found.\n"
Show outdated Hide outdated src/rpc/rawtransaction.cpp
if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true))
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex ? "No such mempool or blockchain transaction"
if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true, blockIndex))
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex || blockIndex ? "No such mempool or blockchain transaction"

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

Maybe further update this error message, eg if (blockIndex) "No such transaction found in the provided block".

@TheBlueMatt

TheBlueMatt Jun 7, 2017

Contributor

Maybe further update this error message, eg if (blockIndex) "No such transaction found in the provided block".

This comment has been minimized.

@kallewoof

kallewoof Jun 8, 2017

Member

Yeah, I wanted to avoid ?:?:. Rewritten as:

    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true, blockIndex)) {
        std::string errmsg;
        if (blockIndex) {
            errmsg = "No such transaction found in the provided block";
        } else {
            errmsg = fTxIndex
              ? "No such mempool or blockchain transaction"
              : "No such mempool transaction. Use -txindex to enable blockchain transaction queries";
        }
        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, errmsg + ". Use gettransaction for wallet transactions.");
    }
@kallewoof

kallewoof Jun 8, 2017

Member

Yeah, I wanted to avoid ?:?:. Rewritten as:

    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true, blockIndex)) {
        std::string errmsg;
        if (blockIndex) {
            errmsg = "No such transaction found in the provided block";
        } else {
            errmsg = fTxIndex
              ? "No such mempool or blockchain transaction"
              : "No such mempool transaction. Use -txindex to enable blockchain transaction queries";
        }
        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, errmsg + ". Use gettransaction for wallet transactions.");
    }
@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jun 8, 2017

Member

@TheBlueMatt Thanks for the review!

Generally we try to make sure that after each individual commit, at least it builds and all tests pass.

We try to keep tests as separate commits though, so that would assume tests and code changes come in pairs (tests will fail before test commit or after test commit and before change commit, obv). That was my intention with the split here. I may have screwed up. I'll double check and/or squash as appropriate.

Edit: I noticed the order was off (two fixes then two tests). Rearranged them. The commit/test pairs now pass make check individually (i.e. a43ec61 and 8f2ce52).

Edit 2: [...]:

Member

kallewoof commented Jun 8, 2017

@TheBlueMatt Thanks for the review!

Generally we try to make sure that after each individual commit, at least it builds and all tests pass.

We try to keep tests as separate commits though, so that would assume tests and code changes come in pairs (tests will fail before test commit or after test commit and before change commit, obv). That was my intention with the split here. I may have screwed up. I'll double check and/or squash as appropriate.

Edit: I noticed the order was off (two fixes then two tests). Rearranged them. The commit/test pairs now pass make check individually (i.e. a43ec61 and 8f2ce52).

Edit 2: [...]:

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

[rpc] Allow getrawtransaction to take optional blockhash to fetch tra…
…nsaction from a block directly.

Github-Pull: #10275
Rebased-From: 8485239

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 22, 2017

Contributor

utACK 8f2ce52

Contributor

TheBlueMatt commented Jun 22, 2017

utACK 8f2ce52

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 14, 2017

Member

Needs rebase.

Member

jonasschnelli commented Jul 14, 2017

Needs rebase.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jul 14, 2017

Member

Rebased.

Member

kallewoof commented Jul 14, 2017

Rebased.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 14, 2017

Member

a) Is there a reason why mainchain height is not supported as alternative for the blockhash? Eventually with a security of only accepting heights of a hundred blocks below the tip as a reorganisation protection (but I'd prefer to not add this protection).

b) @kallewoof the idea about the standard for a transaction reference has already been worked into a BIP: bitcoin/bips#555 (maybe we can support this – if we agree on that BIP to be worth implementing – also via getrawtransaction).

Member

jonasschnelli commented Jul 14, 2017

a) Is there a reason why mainchain height is not supported as alternative for the blockhash? Eventually with a security of only accepting heights of a hundred blocks below the tip as a reorganisation protection (but I'd prefer to not add this protection).

b) @kallewoof the idea about the standard for a transaction reference has already been worked into a BIP: bitcoin/bips#555 (maybe we can support this – if we agree on that BIP to be worth implementing – also via getrawtransaction).

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Jul 14, 2017

Member

@jonasschnelli Regarding height, I chose not to include it as it could potentially cause unexpected results when a reorg happens, but if people don't think that's an issue it should be fairly straightforward to allow for both.

Edit: as for the standard, that looks exciting for sure. If it matures enough and this PR isn't merged already I may take a stab at it.

Member

kallewoof commented Jul 14, 2017

@jonasschnelli Regarding height, I chose not to include it as it could potentially cause unexpected results when a reorg happens, but if people don't think that's an issue it should be fairly straightforward to allow for both.

Edit: as for the standard, that looks exciting for sure. If it matures enough and this PR isn't merged already I may take a stab at it.

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Sep 4, 2017

Member

I try to keep tests and code changes as separate commits. Why would you want it to succeed between the test update? (Or am I confused about what you're asking?)

I think the principal is that each individual commit should be at least internally consistent (should build and pass its own tests). Your first commit updates the product code so the returned error changes, and your second commit updates the test code to pass with the new error string. That means that if I run the tests after the first commit they'll fail.

I understand the desire to structure the commits in a way that tells a story, but this breaks tools like git bisect that rely on commits to be internally consistent.

Member

jnewbery commented Sep 4, 2017

I try to keep tests and code changes as separate commits. Why would you want it to succeed between the test update? (Or am I confused about what you're asking?)

I think the principal is that each individual commit should be at least internally consistent (should build and pass its own tests). Your first commit updates the product code so the returned error changes, and your second commit updates the test code to pass with the new error string. That means that if I run the tests after the first commit they'll fail.

I understand the desire to structure the commits in a way that tells a story, but this breaks tools like git bisect that rely on commits to be internally consistent.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Sep 5, 2017

Member

@jnewbery You can git bisect skip if you land on a commit squeezed in between tests. I don't really agree with squashing test updates and code changes together, personally. That's how I've always done it, anyway.

Member

kallewoof commented Sep 5, 2017

@jnewbery You can git bisect skip if you land on a commit squeezed in between tests. I don't really agree with squashing test updates and code changes together, personally. That's how I've always done it, anyway.

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Sep 5, 2017

Member

I agree with @jnewbery, one commit should not break the code, tests including. If you revert that commit, codes stays green. IMO makes reviewing easier.

Member

promag commented Sep 5, 2017

I agree with @jnewbery, one commit should not break the code, tests including. If you revert that commit, codes stays green. IMO makes reviewing easier.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
std::string errmsg;
if (blockindex) {
bool block_available = true;
if (gArgs.GetArg("-prune", 0)) {

This comment has been minimized.

@luke-jr

luke-jr Sep 5, 2017

Member

Use fHavePruned

@luke-jr

luke-jr Sep 5, 2017

Member

Use fHavePruned

Show outdated Hide outdated src/rpc/rawtransaction.cpp
if (gArgs.GetArg("-prune", 0)) {
// we are pruning; see if block is available at all
CBlock block;
block_available = ReadBlockFromDisk(block, blockindex, Params().GetConsensus());

This comment has been minimized.

@luke-jr

luke-jr Sep 5, 2017

Member

Use !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0 (see getblock)

@luke-jr

luke-jr Sep 5, 2017

Member

Use !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0 (see getblock)

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Sep 5, 2017

Member

I agree individual commits should never break the tests ideally. That doesn't mean that you can't introduce new functionality and its new tests in separated commits when that's not the case.
Needs rebase again.

Member

jtimon commented Sep 5, 2017

I agree individual commits should never break the tests ideally. That doesn't mean that you can't introduce new functionality and its new tests in separated commits when that's not the case.
Needs rebase again.

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Sep 5, 2017

Member

Last two commits look good. For the pruning error, I think you can error out earlier by re-using the pruned test from getblock:

     if (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0)
        throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");

I don't really agree with squashing test updates and code changes together, personally.

ok. I think both ways are valid, and I've come across both before. I'm not sure if this project has a preferred style.

Member

jnewbery commented Sep 5, 2017

Last two commits look good. For the pruning error, I think you can error out earlier by re-using the pruned test from getblock:

     if (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0)
        throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");

I don't really agree with squashing test updates and code changes together, personally.

ok. I think both ways are valid, and I've come across both before. I'm not sure if this project has a preferred style.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Sep 6, 2017

Member

I'm merging those commits as it's not new functionality but just a fix that changes an error message.

Member

kallewoof commented Sep 6, 2017

I'm merging those commits as it's not new functionality but just a fix that changes an error message.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
Member

kallewoof commented Sep 6, 2017

@luke-jr Nice!

Show outdated Hide outdated src/rpc/rawtransaction.cpp
if (blockindex) {
bool block_pruned = fHavePruned && !(blockindex->nStatus & BLOCK_HAVE_DATA) && blockindex->nTx > 0;
errmsg = block_pruned
? "Block not available (pruned data)"

This comment has been minimized.

@jnewbery

jnewbery Sep 6, 2017

Member

supernit: Not finding a block because it's been pruned is not a RPC_INVALID_ADDRESS_OR_KEY error. It would more accurately be a RPC_MISC_ERROR.

My suggestion would be to do the pruned check above, immediately after the throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found"); line

@jnewbery

jnewbery Sep 6, 2017

Member

supernit: Not finding a block because it's been pruned is not a RPC_INVALID_ADDRESS_OR_KEY error. It would more accurately be a RPC_MISC_ERROR.

My suggestion would be to do the pruned check above, immediately after the throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found"); line

This comment has been minimized.

@kallewoof

kallewoof Sep 7, 2017

Member

Yeah, I was thinking about that too. Fixing.

@kallewoof

kallewoof Sep 7, 2017

Member

Yeah, I was thinking about that too. Fixing.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Sep 23, 2017

Member

re-utACK 300a5f1

Member

jtimon commented Sep 23, 2017

re-utACK 300a5f1

@TheBlueMatt

Needs trivial rebase. Note that first commit message is no longer correct (says "ensure null is accepted" but null is already accepted on master, commit contents are still good, though). Otherwise largely looks good.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
std::string errmsg;
if (blockindex) {
if (fHavePruned && !(blockindex->nStatus & BLOCK_HAVE_DATA) && blockindex->nTx > 0) {

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Nov 27, 2017

Contributor

Why check nTx here? Its just as useful to provide an error of "Block not available" if it has been pruned as if it has not yet been received in full.

@TheBlueMatt

TheBlueMatt Nov 27, 2017

Contributor

Why check nTx here? Its just as useful to provide an error of "Block not available" if it has been pruned as if it has not yet been received in full.

This comment has been minimized.

@kallewoof

kallewoof Nov 28, 2017

Member

This is how the check is done in a lot of places. E.g.
https://github.com/kallewoof/bitcoin/blame/gettx-with-blockhash/src/rest.cpp#L219

I'm not sure why an additional nTx check is needed, to be honest.

@kallewoof

kallewoof Nov 28, 2017

Member

This is how the check is done in a lot of places. E.g.
https://github.com/kallewoof/bitcoin/blame/gettx-with-blockhash/src/rest.cpp#L219

I'm not sure why an additional nTx check is needed, to be honest.

This comment has been minimized.

@sipa

sipa Nov 28, 2017

Member

"nTx > 0" means we had the block's contents at some point.

@sipa

sipa Nov 28, 2017

Member

"nTx > 0" means we had the block's contents at some point.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Nov 28, 2017

Contributor

Yes, I'm saying remove the fHavePruned and nTx check here - if we have a block's header but not its body, then we'll throw a "No such transaction found in the provided block" error, which is nonsense. Easier to just make this error generic.

@TheBlueMatt

TheBlueMatt Nov 28, 2017

Contributor

Yes, I'm saying remove the fHavePruned and nTx check here - if we have a block's header but not its body, then we'll throw a "No such transaction found in the provided block" error, which is nonsense. Easier to just make this error generic.

This comment has been minimized.

@kallewoof

kallewoof Nov 28, 2017

Member

Ahh, OK.

@kallewoof

kallewoof Nov 28, 2017

Member

Ahh, OK.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
std::string errmsg;
if (blockindex) {
if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {
throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Dec 4, 2017

Contributor

Now the error message is wrong :/. You should drop the "(pruned data)" note, as this can happen if you've received the header but not (yet) the block.

@TheBlueMatt

TheBlueMatt Dec 4, 2017

Contributor

Now the error message is wrong :/. You should drop the "(pruned data)" note, as this can happen if you've received the header but not (yet) the block.

This comment has been minimized.

@kallewoof

kallewoof Dec 4, 2017

Member

Oh, damn. Sorry about that. Fixed.

@kallewoof

kallewoof Dec 4, 2017

Member

Oh, damn. Sorry about that. Fixed.

return true;
}
// transaction not found in index, nothing more can be done

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Dec 4, 2017

Contributor

Looks like this line was lost in a rebase? Shouldn't remove it.

@TheBlueMatt

TheBlueMatt Dec 4, 2017

Contributor

Looks like this line was lost in a rebase? Shouldn't remove it.

This comment has been minimized.

@kallewoof

kallewoof Dec 4, 2017

Member

Fixed!

@kallewoof

kallewoof Dec 4, 2017

Member

Fixed!

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Dec 5, 2017

Member

I realize I'm very late with this, so this is just for consideration and not an objection to this PR.

I believe getrawtransaction is already a weird mixed bag of semantics (it works when a transaction is confirmed but at least one output is not yet spent by a confirmed transaction, or if the transaction is unconfirmed, or if txindex is available for everything but the genesis block). Adding yet another combination that works in some cases will only further the difficulty of explaining it.

Ideally, I think this RPC should be replaced with a lookuprawtransaction (or so) which only works when -txindex is enabled. In case you only know a transaction is unconfirmed, use getmempoolentry which already exists. This functionality provided here should IMHO just be a separate RPC.

Member

sipa commented Dec 5, 2017

I realize I'm very late with this, so this is just for consideration and not an objection to this PR.

I believe getrawtransaction is already a weird mixed bag of semantics (it works when a transaction is confirmed but at least one output is not yet spent by a confirmed transaction, or if the transaction is unconfirmed, or if txindex is available for everything but the genesis block). Adding yet another combination that works in some cases will only further the difficulty of explaining it.

Ideally, I think this RPC should be replaced with a lookuprawtransaction (or so) which only works when -txindex is enabled. In case you only know a transaction is unconfirmed, use getmempoolentry which already exists. This functionality provided here should IMHO just be a separate RPC.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Dec 5, 2017

Contributor
Contributor

TheBlueMatt commented Dec 5, 2017

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Dec 5, 2017

Member

I'm explicitly not objecting in any way, just offering a view to consider.

Member

sipa commented Dec 5, 2017

I'm explicitly not objecting in any way, just offering a view to consider.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Dec 5, 2017

Member

I agree getrawtransaction is very confusing until you understand its limitations. I'll gladly work on streamlining it into potentially two or more RPC calls post-merge. Biggest concern is backwards compatibility and (alternatively) bloat.

Edit: I do think the confusion will be decreased when users are given the option of providing a block hash, as there was no way to get the tx before this PR.

Member

kallewoof commented Dec 5, 2017

I agree getrawtransaction is very confusing until you understand its limitations. I'll gladly work on streamlining it into potentially two or more RPC calls post-merge. Biggest concern is backwards compatibility and (alternatively) bloat.

Edit: I do think the confusion will be decreased when users are given the option of providing a block hash, as there was no way to get the tx before this PR.

@promag

IMO overloaded API's are harder to maintain, test and document. Anyway, some comments below.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
else {
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid type provided. Verbose parameter must be a boolean.");
blockindex = it->second;
in_active_chain = (chainActive[blockindex->nHeight] == blockindex);

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Use chainActive.Contains()?

@promag

promag Dec 5, 2017

Member

Use chainActive.Contains()?

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

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Add tests for invalid hash?

  • must be string;
  • must be hexadecimal string;
  • must be of length.
@promag

promag Dec 5, 2017

Member

Add tests for invalid hash?

  • must be string;
  • must be hexadecimal string;
  • must be of length.
UniValue result(UniValue::VOBJ);
TxToJSON(*tx, hashBlock, result);
if (blockindex) result.push_back(Pair("in_active_chain", in_active_chain));

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Missing test for in_active_chain.

@promag

promag Dec 5, 2017

Member

Missing test for in_active_chain.

Show outdated Hide outdated test/functional/rawtransactions.py
@@ -50,6 +50,19 @@ def run_test(self):
# This will raise an exception since there are missing inputs
assert_raises_rpc_error(-25, "Missing inputs", self.nodes[2].sendrawtransaction, rawtx['hex'])
############################
# getrawtx with block hash #

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

# getrawtransaction with block hash #?

@promag

promag Dec 5, 2017

Member

# getrawtransaction with block hash #?

Show outdated Hide outdated src/validation.cpp
@@ -927,46 +927,47 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
}
/** Return transaction in txOut, and if it was found inside a block, its hash is placed in hashBlock */

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Care to explain the new argument?

@promag

promag Dec 5, 2017

Member

Care to explain the new argument?

Show outdated Hide outdated src/rpc/rawtransaction.cpp
@@ -79,12 +82,14 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
"\nArguments:\n"
"1. \"txid\" (string, required) The transaction id\n"
"2. verbose (bool, optional, default=false) If false, return a string, otherwise return a json object\n"

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Nit, mind to fix verbose alignment?

@promag

promag Dec 5, 2017

Member

Nit, mind to fix verbose alignment?

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Dec 5, 2017

Member

@promag Thanks for the review! I think I addressed all your comments.

Member

kallewoof commented Dec 5, 2017

@promag Thanks for the review! I think I addressed all your comments.

@promag

Missing a test checking that in_active_chain is not in the response when blockhash is not provided.

assert_raises_rpc_error(-5, "No such transaction found", self.nodes[0].getrawtransaction, tx, True, block2)
# An invalid block hash should raise the correct errors
assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, True)
assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, "foobar")

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Can remove this.

@promag

promag Dec 5, 2017

Member

Can remove this.

This comment has been minimized.

@kallewoof

kallewoof Dec 5, 2017

Member

First test checks for non-string, this one for non-hexadecimal string. May be redundant but they check different things.

@kallewoof

kallewoof Dec 5, 2017

Member

First test checks for non-string, this one for non-hexadecimal string. May be redundant but they check different things.

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Dec 5, 2017

Member

@promag Added test for unset in_active_chain.

Member

kallewoof commented Dec 5, 2017

@promag Added test for unset in_active_chain.

@promag

Mind some nits.

@@ -188,13 +212,13 @@ def run_test(self):
assert_equal(self.nodes[0].getrawtransaction(txHash, True)["hex"], rawTxSigned['hex'])
# 6. invalid parameters - supply txid and string "Flase"
assert_raises_rpc_error(-3,"Invalid type", self.nodes[0].getrawtransaction, txHash, "Flase")
assert_raises_rpc_error(-1,"not a boolean", self.nodes[0].getrawtransaction, txHash, "Flase")

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Nit, add space after ,. Same below.

@promag

promag Dec 5, 2017

Member

Nit, add space after ,. Same below.

tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
block1, block2 = self.nodes[2].generate(2)
self.sync_all()
# We should be able to get the raw transaction by providing the correct block

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Nit, remaining comments are lowercase. Same below.

@promag

promag Dec 5, 2017

Member

Nit, remaining comments are lowercase. Same below.

assert_equal(gottx['txid'], tx)
assert_equal(gottx['in_active_chain'], True)
# We should not have the 'in_active_chain' flag when we don't provide a block
gottx = self.nodes[0].getrawtransaction(tx, True)

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Calling without blockhash in a section where the RPC is tested with a block hash :trollface: maybe move this elsewhere?

@promag

promag Dec 5, 2017

Member

Calling without blockhash in a section where the RPC is tested with a block hash :trollface: maybe move this elsewhere?

This comment has been minimized.

@kallewoof

kallewoof Dec 6, 2017

Member

in_active_chain is a part of the blockhash stuff, though, so it sort of makes sense to keep the test together with the others.

@kallewoof

kallewoof Dec 6, 2017

Member

in_active_chain is a part of the blockhash stuff, though, so it sort of makes sense to keep the test together with the others.

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

This comment has been minimized.

@promag

promag Dec 5, 2017

Member

Could use parameter name instead of parameter 3? This is ugly for named args. (fix others in a new commit or in a follow up PR, whatever).

@promag

promag Dec 5, 2017

Member

Could use parameter name instead of parameter 3? This is ugly for named args. (fix others in a new commit or in a follow up PR, whatever).

This comment has been minimized.

@kallewoof

kallewoof Dec 6, 2017

Member

This is used in a ton of places. I never saw consensus on what exactly to do and chose to stick with it for now. As you said, different PR.

@kallewoof

kallewoof Dec 6, 2017

Member

This is used in a ton of places. I never saw consensus on what exactly to do and chose to stick with it for now. As you said, different PR.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Dec 5, 2017

Contributor

utACK 434526a

Contributor

TheBlueMatt commented Dec 5, 2017

utACK 434526a

);
LOCK(cs_main);
bool in_active_chain = true;

This comment has been minimized.

@MarcoFalke

MarcoFalke Dec 6, 2017

Member

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

Strike that. gcc would throw a warning.

@MarcoFalke

MarcoFalke Dec 6, 2017

Member

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

Strike that. gcc would throw a warning.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 6, 2017

Member

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

Tested ACK now:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Tested ACK 434526aba680cb73208e018a02827d51a71cfff6
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaJ1PQAAoJENLqSFDnUoslonMQAJz8bVoePuyYUgOHkbGIGemj
4yUyywvp34A4BocYrczH9Im6q+/VBoLrxd+7BqcrhWclHC2G7tvVircgXS/O/RHE
SrEjox5dpLnQ5vW2Ik1hZkdlWvJvn6KBPRqLxZ8qLVFE2ym9BapDhZy33kePh4If
BPBalz7U/1rMz2p2a7yKLo26+MArSwt9kiGebuzgUeaUREjn3gQhaouo4mGSVN59
3Fc1zw3TS1zRuXek/EGL6uuRNQsZZkS08maHtZcsigtwS9drrekrPgx2LswvqGoN
HNkF3AkVK2RZ96cvyG1DbGPG8PkY+EQxO97de2Jlq8/8qQ7wu/BrwJvhjGpdoqGI
SL2pA9KgPfWDTDr13Tcavm+iGk+SbI99Hk+MgsyRArplPJYUtTi43WID/uRaydyE
mCykCPFHkDehVIb8FL/MxSCyBzqJxn4IGXL/Q4A+BbbEYgu1Yi8xtE1o/sCqHyvD
qoKlj1ns9JhNOIpdZNhaWvVb30VuugcZh1/I7CE4NJINdaP7oYKLS0JCiBjwxLKN
6lvBYfoJmNLOvEOiB5DGeQEtnU/mBeGrDgoRddNpnKWRQMOIKi53aKvmeJ4XNw0T
UcIAxaj6c+lZNKYeedePcuRKapqIW9fEQ9IAI8+duRisl6TBcn7/0bJHjWLgy062
NtvMJ3zBCmEhX3Vs4km5
=6+iY
-----END PGP SIGNATURE-----
Member

MarcoFalke commented Dec 6, 2017

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

Tested ACK now:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Tested ACK 434526aba680cb73208e018a02827d51a71cfff6
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaJ1PQAAoJENLqSFDnUoslonMQAJz8bVoePuyYUgOHkbGIGemj
4yUyywvp34A4BocYrczH9Im6q+/VBoLrxd+7BqcrhWclHC2G7tvVircgXS/O/RHE
SrEjox5dpLnQ5vW2Ik1hZkdlWvJvn6KBPRqLxZ8qLVFE2ym9BapDhZy33kePh4If
BPBalz7U/1rMz2p2a7yKLo26+MArSwt9kiGebuzgUeaUREjn3gQhaouo4mGSVN59
3Fc1zw3TS1zRuXek/EGL6uuRNQsZZkS08maHtZcsigtwS9drrekrPgx2LswvqGoN
HNkF3AkVK2RZ96cvyG1DbGPG8PkY+EQxO97de2Jlq8/8qQ7wu/BrwJvhjGpdoqGI
SL2pA9KgPfWDTDr13Tcavm+iGk+SbI99Hk+MgsyRArplPJYUtTi43WID/uRaydyE
mCykCPFHkDehVIb8FL/MxSCyBzqJxn4IGXL/Q4A+BbbEYgu1Yi8xtE1o/sCqHyvD
qoKlj1ns9JhNOIpdZNhaWvVb30VuugcZh1/I7CE4NJINdaP7oYKLS0JCiBjwxLKN
6lvBYfoJmNLOvEOiB5DGeQEtnU/mBeGrDgoRddNpnKWRQMOIKi53aKvmeJ4XNw0T
UcIAxaj6c+lZNKYeedePcuRKapqIW9fEQ9IAI8+duRisl6TBcn7/0bJHjWLgy062
NtvMJ3zBCmEhX3Vs4km5
=6+iY
-----END PGP SIGNATURE-----
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Dec 6, 2017

Member

@jtimon @jnewbery
Mind to re-ACK?

Member

MarcoFalke commented Dec 6, 2017

@jtimon @jnewbery
Mind to re-ACK?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
Member

jonasschnelli commented Dec 6, 2017

utACK 434526a

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 6, 2017

Merge #10275: [rpc] Allow fetching tx directly from specified block i…
…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.

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

This comment has been minimized.

@jnewbery

jnewbery Dec 6, 2017

Member

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.

@jnewbery

jnewbery Dec 6, 2017

Member

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.

@kallewoof kallewoof deleted the kallewoof:gettx-with-blockhash branch Dec 7, 2017

laanwj added a commit that referenced this pull request Dec 7, 2017

Merge #11838: qa: Add getrawtransaction in_active_chain=False test
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

This comment has been minimized.

Show comment
Hide comment
@promag

promag Feb 22, 2018

Member

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

Member

promag commented Feb 22, 2018

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

@kallewoof

This comment has been minimized.

Show comment
Hide comment
@kallewoof

kallewoof Feb 22, 2018

Member

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

Member

kallewoof commented Feb 22, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment