Fixpoint

This patch to The Real Bitcoin was published somewhere around 2016 by someone who as I gather showed up to dump some fairly verbose and undocumented patches then didn't stick around to advocate for them or keep them updated for the meanwhile changing tree.

One of them fills a still-unmet requirement of any split wallet so I'm reviewing it in hopes of saving some time on having to redo from scratch. Full patch is here.

+Value sendrawtransaction(const Array& params, bool fHelp)
+{
+    if (fHelp || params.size() < 1 || params.size() > 1)
+        throw runtime_error(
+            "sendrawtransaction <hex string>\n"
+            "Submits raw transaction (serialized, hex-encoded) to local node and network.");
+
+    vector<unsigned char> txData(ParseHex(params[0].get_str()));
+    CDataStream ssData(txData, SER_NETWORK, VERSION);

Above we see the three-argument form of the CDataStream constructor⁽ⁱ⁾ with explicit arguments matching the defaults for the nTypeIn and nVersionIn parameters.

Some documentation of what if anything these parameters are for would be nice (though not a deficiency of this patch). For now it seems to me a reasonable demand that the defaults never change, or at any rate that the costs of such a change be borne by he who makes it. Thus their inclusion here would be redundant, and not really the helpful kind of redundancy at that i.e. the kind that catches mistakes. I had omitted them in my getrawtransaction patch.

+    CTransaction tx;
+
+    try {
+        ssData >> tx;
+    }
+    catch (std::exception &e) {
+        throw JSONRPCError(-22, "tx decode failed");
+    }
+

The -22 isn't seen elsewhere in the tree, but matches the number in the PRB implementation and named RPC_DESERIALIZATION_ERROR.

The condition of excess data in the stream after the transaction does not appear to be detected, which I figure would be nice to have.

+    CTxDB txdb("r");
+    uint256 txHash = tx.GetHash();
+    uint256 hashBlock = 0;
+
+    if (TransactionSeen(txHash, hashBlock))
+    {
+        throw JSONRPCError(-5, string("tx already ") +
+            (hashBlock != 0 ?
+                string("included in block ") + hashBlock.ToString() :
+                "in memory pool"));
+    }
+

Here it gets iffy in my opinion. Just because my node sees the transaction, whether in block or mempool, it doesn't follow that other nodes do too. What if I want to re-broadcast?

Secondarily, the use of zero as a sentinel seems mildly unhygienic as it's a legal hash value, though I suppose if a preimage were found then the whole system would be screwed anyway. Along with the use of argument mutation, I see it more as an indication that too much implicit behavior is being packed into this new TransactionSeen interface, such that its (ONE!) caller needs a hack to do what it actually wants.

+    bool fMissingInputs = false;
+
+    if (!tx.AcceptToMemoryPool(txdb, true, &fMissingInputs) || fMissingInputs)
+    {
+        throw JSONRPCError(-22, string("tx rejected") +
+            (fMissingInputs ? ", missing inputs" : ""));
+    }
+

Perhaps a nitpick but why bother having error codes if you don't use them to distinguish things? Is the human text part of API now?

Looking into AcceptToMemoryPool, it doesn't actually do anything with that fMissingInputs besides initializing to false!

By the earlier argument it's also questionable whether that second fCheckInputs parameter should be true. It activates what looks like assorted anti-spam code of dubious value in this context.

+    SyncWithWallets(tx, NULL, true);
+    CInv inv(MSG_TX, txHash);
+    RelayMessage(inv, tx);
+
+    return txHash.GetHex();
+}

Oh yes, there's code to support handling wallets plural, not that anything is done with it.

The CInv boilerplate is demanded by the poor abstraction of RelayMessage. That function includes some 15-minute expiration mechanism whose purpose and workings aren't readily apparent to me.

Generally, it seems awkward that all these steps are needed here; shouldn't there be a function somewhere that handles transactions received from the network, that we'd just reuse? I'm almost afraid to look.

+// return wether a given transaction is found on mempool or already included in a block
+bool TransactionSeen(const uint256 &hash, uint256 &hashBlock)
+{
+    CRITICAL_BLOCK(cs_mapTransactions)
+        if (mapTransactions.count(hash))
+            return true;
+
+    CTransaction tx;
+    CTxDB txdb("r");
+    CTxIndex txindex;
+
+    if (tx.ReadFromDisk(txdb, COutPoint(hash, 0), txindex))
+    {
+        CBlock block;
+        if (block.ReadFromDisk(txindex.pos.nFile, txindex.pos.nBlockPos, false))
+            hashBlock = block.GetHash();
+        return true;
+    }
+
+    return false;
+}

All the objects getting passed around and mutated here are painful to follow. This appears to do what it says, though I'd much rather be rid of it.

Painfully apparent in the debts of the codebase is documentation of locking protocol. Getting this sort of thing wrong results in non-deterministic failures, either data corruption or deadlock depending on the direction of the mistake.

 // make sure all wallets know about the given transaction, in the given block
-void static SyncWithWallets(const CTransaction& tx, const CBlock* pblock = NULL, bool fUpdate = false)
+void SyncWithWallets(const CTransaction& tx, const CBlock* pblock, bool fUpdate)
 {
     BOOST_FOREACH(CWallet* pwallet, setpwalletRegistered)
         pwallet->AddToWalletIfInvolvingMe(tx, pblock, fUpdate);

Parameter defaults move to the declaration; I gather C++ puts the burden of setting these on the generated calling code, which seems sensible for a statically-compiled language.

 void RegisterWallet(CWallet* pwalletIn);
 void UnregisterWallet(CWallet* pwalletIn);
+void SyncWithWallets(const CTransaction& tx, const CBlock* pblock = NULL, bool fUpdate = false);
+bool TransactionSeen(const uint256 &hash, uint256 &hashBlock);
 bool ProcessBlock(CNode* pfrom, CBlock* pblock);
 bool CheckDiskSpace(uint64 nAdditionalBytes=0);
 FILE* OpenBlockFile(unsigned int nFile, unsigned int nBlockPos, const char* pszMode="rb");

Perhaps polarbeard had other patches that make use of this TransactionSeen. I'd rather this sort of patch not introduce globally-visible interfaces unnecessarily.

TransactionSeen is introduced as a globally-visible interface so that the underlying mapTransactions object stays static, in contrast with my more liberal approach.

In conclusion, the patch appears safe to apply and should do for now for the sake of testing my offline wallet, which might help inform any changes needed here. After some testing I might even end up signing it, though I'd much prefer something simpler and more clearly defined.

1. And here I long for a self-hosted and accurately cross-referenced i.e. language- and context-aware source browser, but thanks to jurov for this approximate one. [^]