Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from Dec 6, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5f5a2c
[rpc] Fix fVerbose parsing (remove excess if cases).
kallewoof May 6, 2017
b167951
[rpc] Allow getrawtransaction to take optional blockhash to fetch tra…
kallewoof Apr 25, 2017
434526a
[test] Add tests for getrawtransaction with block hash.
kallewoof May 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 45 additions & 12 deletions src/rpc/rawtransaction.cpp
View file
Open in desktop
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
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
Member 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
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,30 +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;
Copy link
Member

@maflcko maflcko Dec 5, 2017 •
edited

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()) {
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");
Copy link
Member

@promag promag Dec 5, 2017 •
edited

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
Member

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
Member 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
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");
}
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";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would also hit this error if the block had been pruned. In that case, this error is misleading.

Is there any way to test above for whether the block has been pruned and error out before doing the transaction lookup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

I added code (3c0cb43) that checks if pruning is enabled, and if it is, checks if the block is actually available by reading it from disk and setting the error message based on the results of that.

} 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));
Copy link
Member

@promag promag Dec 5, 2017 •
edited

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 @@ -983,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
View file
Open in desktop
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the diff can be less disruptive by moving everything inside if (!blockIndex) {...} to a new static function defined right above instead of indenting all of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a huge fan of changing code just to make diffs smaller, and as I mention above, you can put ?w=1 to get diff without indentation changes.

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

// transaction not found in index, nothing more can be done
Copy link
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
Member 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
Copy link
Member

Choose a reason for hiding this comment

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

should skip this logic if user has provided the blockhash?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is inside the if (!blockIndex) block so it should never execute if a block hash was provided, no?

Copy link
Member

Choose a reason for hiding this comment

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

Oops! Not good at counting brackets.

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
View file
Open in desktop
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