Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 44 additions & 23 deletions src/rpc/rawtransaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,15 @@ void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry)

UniValue getrawtransaction(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
throw std::runtime_error(
"getrawtransaction \"txid\" ( verbose )\n"
"getrawtransaction \"txid\" ( verbose \"blockhash\" )\n"

"\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 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"
"DEPRECATED: for now, it also works for transactions with unspent outputs.\n"

"\nReturn the raw transaction data.\n"
Expand All @@ -78,13 +81,15 @@ 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"
"2. verbose (bool, optional, default=false) If false, return a string, otherwise return a json object\n"
"3. \"blockhash\" (string, optional) The block in which to look for the transaction\n"

"\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"
" \"in_active_chain\": b, (bool) Whether specified block is in the active chain or not (only present with explicit \"blockhash\" argument)\n"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: alignment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

            "  \"in_active_chain\": b, (bool) Whether specified block is in the active chain or not (only present with explicit \"blockhash\" argument)\n"
            "  \"hex\" : \"data\",       (string) The serialized, hex-encoded data for 'txid'\n"

becomes

  "in_active_chain": b, (bool) Whether specified block is in the active chain or not (only present with explicit "blockhash" argument)
  "hex" : "data",       (string) The serialized, hex-encoded data for 'txid'

(two \ vs four \ means indentation diff in code)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course you're correct. The size, vsize, version, ... fields below aren't aligned by the same logic, but that's beyond the scope of this PR. Please disregard my previous comment.

" \"hex\" : \"data\", (string) The serialized, hex-encoded data for 'txid'\n"
" \"txid\" : \"id\", (string) The transaction id (same as provided)\n"
" \"hash\" : \"id\", (string) The transaction hash (differs from txid for witness transactions)\n"
Expand Down Expand Up @@ -132,42 +137,58 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
+ HelpExampleCli("getrawtransaction", "\"mytxid\"")
+ HelpExampleCli("getrawtransaction", "\"mytxid\" true")
+ HelpExampleRpc("getrawtransaction", "\"mytxid\", true")
+ HelpExampleCli("getrawtransaction", "\"mytxid\" false \"myblockhash\"")
+ HelpExampleCli("getrawtransaction", "\"mytxid\" true \"myblockhash\"")
);

LOCK(cs_main);

bool in_active_chain = true;

@maflcko maflcko Dec 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Strike that. gcc would throw a warning.

uint256 hash = ParseHashV(request.params[0], "parameter 1");
CBlockIndex* blockindex = nullptr;

// Accept either a bool (true) or a num (>=1) to indicate verbose output.
bool fVerbose = false;
if (!request.params[1].isNull()) {
if (request.params[1].isNum()) {
if (request.params[1].get_int() != 0) {
fVerbose = true;
}
}
else if(request.params[1].isBool()) {
if(request.params[1].isTrue()) {
fVerbose = true;
fVerbose = request.params[1].isNum() ? (request.params[1].get_int() != 0) : request.params[1].get_bool();
}

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

@promag promag Dec 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests for invalid hash?

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!blockhash.IsNull()) {
BlockMap::iterator it = mapBlockIndex.find(blockhash);
if (it == mapBlockIndex.end()) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found");
}
}
else {
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid type provided. Verbose parameter must be a boolean.");
blockindex = it->second;
in_active_chain = chainActive.Contains(blockindex);
}
}

CTransactionRef tx;
uint256 hashBlock;
if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true))
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex ? "No such mempool or blockchain transaction"
: "No such mempool transaction. Use -txindex to enable blockchain transaction queries") +
". Use gettransaction for wallet transactions.");
uint256 hash_block;
if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
std::string errmsg;
if (blockindex) {
if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {
throw JSONRPCError(RPC_MISC_ERROR, "Block not available");
}
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.");
}

if (!fVerbose)
if (!fVerbose) {
return EncodeHexTx(*tx, RPCSerializationFlags());
}

UniValue result(UniValue::VOBJ);
TxToJSON(*tx, hashBlock, result);
if (blockindex) result.push_back(Pair("in_active_chain", in_active_chain));

@promag promag Dec 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test for in_active_chain.

TxToJSON(*tx, hash_block, result);
return result;
}

Expand Down Expand Up @@ -995,7 +1016,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
static const CRPCCommand commands[] =
{ // category name actor (function) argNames
// --------------------- ------------------------ ----------------------- ----------
{ "rawtransactions", "getrawtransaction", &getrawtransaction, {"txid","verbose"} },
{ "rawtransactions", "getrawtransaction", &getrawtransaction, {"txid","verbose","blockhash"} },
{ "rawtransactions", "createrawtransaction", &createrawtransaction, {"inputs","outputs","locktime","replaceable"} },
{ "rawtransactions", "decoderawtransaction", &decoderawtransaction, {"hexstring"} },
{ "rawtransactions", "decodescript", &decodescript, {"hexstring"} },
Expand Down
70 changes: 37 additions & 33 deletions src/validation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -926,47 +926,51 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
return AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, pfMissingInputs, GetTime(), plTxnReplaced, bypass_limits, nAbsurdFee);
}

/** Return transaction in txOut, and if it was found inside a block, its hash is placed in hashBlock */
bool GetTransaction(const uint256 &hash, CTransactionRef &txOut, const Consensus::Params& consensusParams, uint256 &hashBlock, bool fAllowSlow)
/**
* Return transaction in txOut, and if it was found inside a block, its hash is placed in hashBlock.
* If blockIndex is provided, the transaction is fetched from the corresponding block.
*/
bool GetTransaction(const uint256& hash, CTransactionRef& txOut, const Consensus::Params& consensusParams, uint256& hashBlock, bool fAllowSlow, CBlockIndex* blockIndex)
{
CBlockIndex *pindexSlow = nullptr;
CBlockIndex* pindexSlow = blockIndex;

LOCK(cs_main);

CTransactionRef ptx = mempool.get(hash);
if (ptx)
{
txOut = ptx;
return true;
}

if (fTxIndex) {
CDiskTxPos postx;
if (pblocktree->ReadTxIndex(hash, postx)) {
CAutoFile file(OpenBlockFile(postx, true), SER_DISK, CLIENT_VERSION);
if (file.IsNull())
return error("%s: OpenBlockFile failed", __func__);
CBlockHeader header;
try {
file >> header;
fseek(file.Get(), postx.nTxOffset, SEEK_CUR);
file >> txOut;
} catch (const std::exception& e) {
return error("%s: Deserialize or I/O error - %s", __func__, e.what());
}
hashBlock = header.GetHash();
if (txOut->GetHash() != hash)
return error("%s: txid mismatch", __func__);
if (!blockIndex) {
CTransactionRef ptx = mempool.get(hash);
if (ptx) {
txOut = ptx;
return true;
}

// transaction not found in index, nothing more can be done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

return false;
}
if (fTxIndex) {
CDiskTxPos postx;
if (pblocktree->ReadTxIndex(hash, postx)) {
CAutoFile file(OpenBlockFile(postx, true), SER_DISK, CLIENT_VERSION);
if (file.IsNull())
return error("%s: OpenBlockFile failed", __func__);
CBlockHeader header;
try {
file >> header;
fseek(file.Get(), postx.nTxOffset, SEEK_CUR);
file >> txOut;
} catch (const std::exception& e) {
return error("%s: Deserialize or I/O error - %s", __func__, e.what());
}
hashBlock = header.GetHash();
if (txOut->GetHash() != hash)
return error("%s: txid mismatch", __func__);
return true;
}

if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it
const Coin& coin = AccessByTxid(*pcoinsTip, hash);
if (!coin.IsSpent()) pindexSlow = chainActive[coin.nHeight];
// transaction not found in index, nothing more can be done
return false;
}

if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it
const Coin& coin = AccessByTxid(*pcoinsTip, hash);
if (!coin.IsSpent()) pindexSlow = chainActive[coin.nHeight];
}
}

if (pindexSlow) {
Expand Down
2 changes: 1 addition & 1 deletion src/validation.h
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ void ThreadScriptCheck();
/** Check whether we are doing an initial block download (synchronizing from disk or network) */
bool IsInitialBlockDownload();
/** Retrieve a transaction (from memory pool, or from disk, if possible) */
bool GetTransaction(const uint256 &hash, CTransactionRef &tx, const Consensus::Params& params, uint256 &hashBlock, bool fAllowSlow = false);
bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, bool fAllowSlow = false, CBlockIndex* blockIndex = nullptr);
/** Find the best known block, and make it the tip of the block chain */
bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock = std::shared_ptr<const CBlock>());
CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
Expand Down
30 changes: 27 additions & 3 deletions test/functional/rawtransactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,30 @@ 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'])

#####################################
# getrawtransaction with block hash #
#####################################

# make a tx by sending then generate 2 blocks; block1 has the tx in it
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, remaining comments are lowercase. Same below.

gottx = self.nodes[0].getrawtransaction(tx, True, block1)
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert_equal(gottx['txid'], tx)
assert 'in_active_chain' not in gottx
# We should not get the tx if we provide an unrelated block
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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert_raises_rpc_error(-8, "parameter 3 must be of length 64", self.nodes[0].getrawtransaction, tx, True, "abcd1234")
assert_raises_rpc_error(-5, "Block hash not found", self.nodes[0].getrawtransaction, tx, True, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")

#########################
# RAW TX MULTISIG TESTS #
#########################
Expand Down Expand Up @@ -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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, add space after ,. Same below.


# 7. invalid parameters - supply txid and empty array
assert_raises_rpc_error(-3,"Invalid type", self.nodes[0].getrawtransaction, txHash, [])
assert_raises_rpc_error(-1,"not a boolean", self.nodes[0].getrawtransaction, txHash, [])

# 8. invalid parameters - supply txid and empty dict
assert_raises_rpc_error(-3,"Invalid type", self.nodes[0].getrawtransaction, txHash, {})
assert_raises_rpc_error(-1,"not a boolean", self.nodes[0].getrawtransaction, txHash, {})

inputs = [ {'txid' : "1d1d4e24ed99057e84c3f80fd8fbec79ed9e1acee37da269356ecea000000000", 'vout' : 1, 'sequence' : 1000}]
outputs = { self.nodes[0].getnewaddress() : 1 }
Expand Down