Fixpoint

2019-12-05

Basic getrawtransaction patch proposal for bitcoind

Filed under: Bitcoin, Software — Jacob Welsh @ 17:35

I present a bitcoind patch, of my own authorship, for consideration: a basic but functional getrawtransaction RPC command. I expect the need for it is clear: if it's your node then you shouldn't accept any sort of "I'm sorry Dave, I'm afraid I can't do that" especially regarding the data it already has handy.

To speak plainly about some deficits, this time not my own: the Real Bitcoin project presently rests on some shaky foundations. Despite apparently intricate efforts, a Keccak based vtree has seen little progress, in that the original patch signers besides mod6 have not signed on to the regrinds of their work, and some recommended patches have not been reground at all, updated for the very necessary manifest spec, or included in the mainline collection. Further, the sorry state of the inherited code such as magic numbers everywhere has seen little improvement. Perhaps I will have to take up some of these burdens in time; for now I'll leave it as an entreaty to the elder hands to please find a way to make it happen!

The patch

As in the original introduced somewhere around PRB 0.7.0, this command takes a transaction ID (256 bits of hex), searches first the node's own memory pool then the confirmed transaction database (blkindex.dat) and returns a hex dump of the encoded transaction. Unlike the original it does not support a "verbose" option to give a JSON representation of the data. This task seems to me better suited to an external tool, but I could see including it here if the implementation is concise and obviously correct.

Backporting the original was not possible due to the many intervening changes, though I did consult it to confirm I hadn't missed anything important and matched its numeric error code.

Based on the overall idea in the V version control system of building yggdrasil, I'm breaking from one of the project's prior naming conventions by including "bitcoin" but not the author in the patch name; the author is still recorded in the enclosed manifest file. Due to the problems noted earlier with the prior patch tree it's also not a proper vpatch yet.

Download: bitcoin_getrawtransaction.draft.patch.

To try this you will need to:

  1. Perform a press to asciilifeform_whogaveblox.vpatch;
  2. Manually apply mod6_phexdigit_fix.vpatch (which could be missed otherwise due to lacking a manifest entry);
  3. Manually apply the patch in question.

In detail

I added a manifest entry for the phexdigit fix, to make its inclusion explicit:

--- a/bitcoin/manifest.txt
+++ b/bitcoin/manifest.txt
@@ -28,3 +28,5 @@
 542413 asciilifeform_aggressive_pushgetblocks asciilifeform Issue PushGetBlocks command to any peer that issues 'version' command
 542413 mod6_excise_hash_truncation mod6 Regrind of ben_vulpes original; removes truncation of hashes printed to TRB log file
 543661 asciilifeform_whogaveblox asciilifeform Record the origin of every incoming candidate block (whether accepted or rejected)
+606789 mod6_phexdigit_fix mod6 Fix decoding LUT which wrongly accepted certain invalid characters as hex
+606789 bitcoin_getrawtransaction jfw Add RPC to get transactions from memory pool or database (hex only)

The RPC itself is about as simple as it gets in this codebase. First we try the mempool, as this should be fast and may contain unconfirmed transactions.(i)

--- a/bitcoin/src/bitcoinrpc.cpp
+++ b/bitcoin/src/bitcoinrpc.cpp
@@ -1351,7 +1351,31 @@
     return entry;
 }

+Value getrawtransaction(const Array& params, bool fHelp)
+{
+    if (fHelp || params.size() != 1)
+        throw runtime_error(
+            "getrawtransaction <txid>\n"
+            "Get hex serialization of <txid> from memory pool or database.");

+    uint256 hash;
+    map<uint256, CTransaction>::iterator it;
+    CTransaction tx;
+    CDataStream ssTx;
+
+    hash.SetHex(params[0].get_str());
+    it = mapTransactions.find(hash);
+    if (it != mapTransactions.end())
+        tx = it->second;

Functions everywhere have to open their own database connections - though some have the luxury of getting one passed in - which then implies a whole caching mechanism so as not to be horribly inefficient. Odin knows why there couldn't just be one global (or at least per-thread) "This Is The Database; Use It" object.

+    else {
+        CTxDB txdb("r");
+        if (!txdb.ReadDiskTx(hash, tx))
+            throw JSONRPCError(-5, "Transaction not found in memory pool or database.");
+    }
+    ssTx << tx;
+    return HexStr(ssTx.begin(), ssTx.end());
+}
+
 Value backupwallet(const Array& params, bool fHelp)
 {
     if (fHelp || params.size() != 1)

Wiring the function into the RPC dispatch table (I don't recall how I chose where to insert it, as the list was already non-alphabetical; probably based on where it seemed sensible in the help listing):

@@ -1865,6 +1889,7 @@
     make_pair("getreceivedbyaccount",   &getreceivedbyaccount),
     make_pair("listreceivedbyaddress",  &listreceivedbyaddress),
     make_pair("listreceivedbyaccount",  &listreceivedbyaccount),
+    make_pair("getrawtransaction",      &getrawtransaction),
     make_pair("backupwallet",           &backupwallet),
     make_pair("keypoolrefill",          &keypoolrefill),
     make_pair("walletpassphrase",       &walletpassphrase),

The mempool object now needs to be visible between compilation units. I suggest doing a grep to verify this introduces no name conflicts.

--- a/bitcoin/src/main.cpp
+++ b/bitcoin/src/main.cpp
@@ -26,7 +26,7 @@

 CCriticalSection cs_main;

-static map<uint256, CTransaction> mapTransactions;
+map<uint256, CTransaction> mapTransactions;
 CCriticalSection cs_mapTransactions;
 unsigned int nTransactionsUpdated = 0;
 map<COutPoint, CInPoint> mapNextTx;
--- a/bitcoin/src/main.h
+++ b/bitcoin/src/main.h
@@ -46,6 +46,7 @@

 extern CCriticalSection cs_main;
+extern std::map<uint256, CTransaction> mapTransactions;
 extern std::map<uint256, CBlockIndex*> mapBlockIndex;
 extern uint256 hashGenesisBlock;
 extern CBlockIndex* pindexGenesisBlock;

I tested that it builds, successfully fetches transactions from both mempool and database, and returns the expected errors for missing argument or transaction not found. It does accept invalid hex strings, perhaps a flaw in that SetHex method. I've been running the patch in production since around August 10th of this year.

  1. The original cause for my writing the patch was a stuck transaction that wasn't getting relayed to miners or block explorers for unknown reasons. Upon fishing the raw tx from the mempool and submitting it to one such site, a useful error was finally obtained identifying the problem as the S-value normalization mess; the -lows option provided a workaround, after double-spending to self for safety, which was a whole other pain. [^]

7 Comments »

  1. > better suited to an external tool

    Logically the node should really be two parts, a curl part and an awk part so to speak. There's really no reason for the awk part to be always-on, nor for the always-on part to contain a whole, turing-complete, domain language baked in. Blindness to this glaringly obvious split is what drove power ranger idiocy in the "verbose" json vein and so on. Simple droplets.

    > couldn't just be one global (or at least per-thread) "This Is The Database; Use It"

    See ?

    > It does accept invalid hex strings, perhaps a flaw in that SetHex method.

    This needs a looking into tho.

    Comment by Mircea Popescu — 2019-12-05 @ 17:52

  2. "This needs a looking into tho." - for sure & will do.

    I may be missing some locking around the mapTransactions access. There appears to be a coarse-grained lock on cs_main for all the RPCs, which probably explains why they're so damn laggy.

    Comment by Jacob Welsh — 2019-12-15 @ 20:20

  3. Aha.

    Comment by Mircea Popescu — 2019-12-19 @ 06:00

  4. [...] I previously noted about my getrawtransaction patch, It does accept invalid hex strings, perhaps a flaw in that SetHex [...]

    Pingback by Preliminary report on the bitcoind hex conversion mess « Fixpoint — 2020-04-11 @ 16:46

  5. [...] on Fixpoint, I introduced my implementation of the getrawtransaction command and reviewed an existing one for sendrawtransaction. There were some more and less serious problems [...]

    Pingback by New and improved raw transaction support for bitcoind « Fixpoint — 2020-04-22 @ 05:25

  6. [...] enough, and the field naming and formatting follows existing precedent. To my mind this command is another one of those things that should have been there from the start; so barring any valid objections, I will consider it [...]

    Pingback by Mapping the bitcoind block acceptance code, with bonus patch for index access « Fixpoint — 2022-03-22 @ 04:12

  7. [...] [^]Some exceptions were a series presenting a new program while it was still somewhat in flux, my cautious first step into proposing changes to the big scary TRB, and an in-depth review of a desired patch from an unknown entity. [^]The joke is that in my [...]

    Pingback by Sending buffer overflows fixed in bitcoind, and other cleanups « Fixpoint — 2022-05-21 @ 22:40

RSS feed for comments on this post. TrackBack URL

Leave a comment

Powered by MP-WP. Copyright Jacob Welsh.