Report transaction conflicts, and tentative account balance fix #3671

gavinandresen commented at 1:29 AM on February 14, 2014: contributor

This adds a "walletconflicts" array to wallet transaction JSON output, identifying other wallet transactions that also spend one of the inputs of this transaction.

Example output from the txnmall.sh regression test, spending 1 BTC from account "foo", then spending 2 BTC of the change from that transaction and spending it from account "bar". After a mutant of the "foo" transaction is then mined listtransactions reports:

    {
        "account" : "foo",
        "address" : "n2rhjnG8Gg24Dcx66u4KrtiUfxPhPw7suV",
        "category" : "conflicted",
        "amount" : -1.00000000,
        "fee" : 0.00000000,
        "confirmations" : -1,
        "txid" : "650e1c572593ba03e407faebb613c1d449dfc450d3d3f3868566367f482a105b",
        "walletconflicts" : [
            "587573bb1b620604eb39f75377358411f34dd133a8846d50e8947091b208eb1b"
        ],
        "time" : 1392405954,
        "timereceived" : 1392405954
    },
    {
        "account" : "bar",
        "address" : "n2rhjnG8Gg24Dcx66u4KrtiUfxPhPw7suV",
        "category" : "conflicted",
        "amount" : -2.00000000,
        "fee" : 0.00000000,
        "confirmations" : -1,
        "txid" : "222b3d25454a38de6ecd1c493692c7fce8848a2f8586825e4b32fe63c1f41d43",
        "walletconflicts" : [
        ],
        "time" : 1392405954,
        "timereceived" : 1392405954
    },
    {
        "account" : "foo",
        "address" : "n2rhjnG8Gg24Dcx66u4KrtiUfxPhPw7suV",
        "category" : "send",
        "amount" : -1.00000000,
        "fee" : 0.00000000,
        "confirmations" : 1,
        "blockhash" : "0000996962dbab0bf203ab187a683e7c610b2e946b3027c7ef167977dc1ea317",
        "blockindex" : 1,
        "blocktime" : 1392405954,
        "txid" : "587573bb1b620604eb39f75377358411f34dd133a8846d50e8947091b208eb1b",
        "walletconflicts" : [
            "650e1c572593ba03e407faebb613c1d449dfc450d3d3f3868566367f482a105b"
        ],
        "time" : 1392405954,
        "timereceived" : 1392405954
    }

gmaxwell commented at 2:44 AM on February 14, 2014: contributor

Any reason not to just list any transaction with one or more conflicting intput? e.g. generalize it to all double-spends and not just signature mutants?

This would make it include things like an identically outputted transaction with ANYONECANPAY inputs that has had additional inputs added for fees, for example.

apoelstra commented at 6:36 AM on February 14, 2014: contributor

Just tried this patch. It works successfully. I have four types of unconfirmed transactions in my wallet. The first was a malleability dupe (it was a coinjoin which I signed twice). The patch correctly reports it 'conflicted' and shows the confirmed tx under walletconflicts, while the confirmed transaction is still there (labelled 'send' and 'receive', not 'conflicted') and has the unconfirmed tx under walletconflicts.

The second one is a double-spend where the txes differ in output size (fee increase). The unconfirmed one is labelled 'conflicted' and has nothing under walletconflicts. The confirmed one is still there (labelled 'send' and 'receive', not 'conflicted') and has nothing under walletconflicts.

The third and fourth are my mutually-conflicting nonstandard unconfirmed txes, which are both removed from the list by this patch. As I got these into my wallet by disabling the fHave check in rpcsendrawtransactions, and not through any user-accessible use of bitcoind, IMO it's fine that they are lost.

<b>Edit:</b> As with the other patch, 'getunconfirmedbalance' includes the conflicted txes.

in src/wallet.cpp:None in 3d9b7e2621 outdated

 481 | @@ -445,6 +482,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn)
 482 |                               wtxIn.GetHash().ToString(),
 483 |                               wtxIn.hashBlock.ToString());
 484 |              }
 485 | +            AddToClones(normHash, hash);

laanwj commented at 7:32 AM on February 14, 2014:

~~Before this call, we should detect if there is an existing "clone" and if so copy the metadata (at least vOrderForm).~~ Edit: never mind, you already do that in AddToClones...

in src/wallet.cpp:None in 3d9b7e2621 outdated

 420 | +    uint256 normHash = wtxIn.GetNormalizedHash();
 421 | +
 422 | +    if (fFromLoadWallet)
 423 | +    {
 424 | +        mapWallet[hash] = wtxIn;
 425 | +        AddToClones(normHash, hash);

laanwj commented at 7:38 AM on February 14, 2014:

I'm uncertain about doing the metadata copying while loading from the wallet. The wallet transactions aren't loaded in a specific order, so it will basically judge a random one of them to be the 'original clone' and copy that one's metadata. Every time the wallet loads.

gavinandresen commented at 1:46 PM on February 14, 2014:

Good catch, I made a mental note to myself last night to look into how "tx" records are ordered in the wallet. I think nOrderPos might give the ordering we want (metadata should always come from the transaction the wallet created, not the mutated one seen later).

Doing the metadata copying while loading the wallet is important for clean recovery; I could bump the wallet version number and run it as a one-time conversion, but since performance isn't an issue here (metadata is only cloned in the rare case where a mutant gets mined over your original transaction) I don't think that is worth it.

laanwj commented at 1:56 PM on February 14, 2014:

Defining the one with the lowest orderpos as 'original clone' sounds like a sensible resolution. As we create the transactions in increasing orderpos, that will the one created by the client itself.

Edit: agree on not bumping the wallet version for this. When we need to bump the wallet version for another reason at some time in the future we can always move it to the upgrader there.

in src/wallet.cpp:None in 3d9b7e2621 outdated

 408 | +        const CWalletTx& otherTx = mapWallet[otherTxid];
 409 | +        CWalletTx& thisTx = mapWallet[hash];
 410 | +        thisTx.nTimeSmart = otherTx.nTimeSmart;
 411 | +        thisTx.fFromMe = otherTx.fFromMe;
 412 | +        thisTx.strFromAccount = otherTx.strFromAccount;
 413 | +        thisTx.nOrderPos = otherTx.nOrderPos;

laanwj commented at 8:18 AM on February 14, 2014:

Please copy vOrderForm so that we don't lose payment request data and URI 'message' fields

gavinandresen commented at 1:41 PM on February 14, 2014:

ACK

in src/wallet.h:None in 3d9b7e2621 outdated

 107 | @@ -108,6 +108,10 @@ class CWallet : public CCryptoKeyStore, public CWalletInterface
 108 |      int64_t nNextResend;
 109 |      int64_t nLastResend;
 110 |  
 111 | +    // Used to detect and report mutated transactions:
 112 | +    std::multimap<uint256, uint256> mapTxClones;

laanwj commented at 8:24 AM on February 14, 2014:

@sipa Is it safe to use the sipahash(TM) here, or would it be better to use CTxIn.prevout (COutPoint) as you proposed?

sipa commented at 12:45 PM on February 14, 2014:

No point in using a hash here IMO. I'd just use a multimap from COutPoints to the wallet transacitons they are spent int.

gavinandresen commented at 2:26 PM on February 14, 2014:

@sipa: will do. I got stuck yesterday going down the rabbit-hole of COutPoints to possibly-not-a-wallet-transaction....

sipa commented at 2:30 PM on February 14, 2014:

To detect whether a transaction is viable, and possible conflicts, you can rely on the mempool (aka confirmations < 0). You only need the clone/double spend detection to copy metadata I think.

gavinandresen commented at 2:00 PM on February 14, 2014: contributor

@gmaxwell : generalizing to arbitrary double-spends is tricky. If they are a wallet transaction in an old wallet and the double-spend happened long ago and you're not running with -txindex, then the code can know that an input is spent (is not in the UTXO set) but might have no idea which transaction double-spent (the double-spend might not be a wallet transaction-- it might have inputs and outputs completely unrelated to your wallet, but double-spend an input that was involved in one of your transactions but didn't belong to you).

In that case, the best we can do is what the code does now: mark it as category : conflicted.

Ignoring old wallets, it would be possible to write code to store the conflicting txid in the wallet, but even then reporting it is probably a bad idea, because you wouldn't necessarily be able to look it up unless you're running with -txindex.

gavinandresen commented at 7:51 PM on February 14, 2014: contributor

Reworked to use a multimap<COutpoint, uint256> to keep track of conflicts. I still might change this to be more memory-thrifty and store CWalletTx*'s as the values instead of their hashes.

This will handle most double-spends (if both spends are wallet transactions) in addition to mutated/malleable transactions.

I fixed the bug in getunconfirmedbalance. The logic for uncofirmed balance is: count non-final transactions (wallet never creates any, so this is harmless) ... and 0-confirmation transactions that are not IsTrusted.

I removed the credit account balances commit, I will create a separate pull for it because There Be Dragons. With this commit, account balances may be lower than they should be, but will never be higher.

gmaxwell commented at 8:26 PM on February 14, 2014: contributor

It's currently counting all generated coin as a conflict. :)

apoelstra commented at 9:30 PM on February 14, 2014: contributor

OK, with the new patch I am seeing the same listtransaction behaviour as before, except: the txpair which conflicted because they differed in fee, now show each others' txids under 'walletconflicts'. So everything is good here.

getbalance returns my (correct) balance. getunconfirmedbalance returns 0, as it should.

gavinandresen commented at 10:34 PM on February 14, 2014: contributor

Fixed the generated-coins-counted-as-conflict, but gmaxwell found getbalance '*' is counting immature coinbases...

UPDATE: so does 0.9.0rc1, so I don't feel so bad. I'll fix.

Track and report wallet transaction clones

Adds a "walletconflicts" array to transaction info; if
a wallet transaction is mutated, the alternate transaction id
or ids are reported there (usually the array will be empty).

Metadata from the original transaction is copied to the mutant,
so the transaction time and "from" account of the mutant are
reported correctly.

731b89b8b5

BitcoinPullTester commented at 11:58 PM on February 14, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/731b89b8b53cb2ea4d2d5c8f2875def515766ea1 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

gmaxwell commented at 12:50 AM on February 15, 2014: contributor

ACK. (immature coins issue fixed too)

laanwj commented at 8:09 AM on February 15, 2014: member

ACK code changes, works for me (tested with conflicts, mined transactions and unconfirmed transactions, as well as spending unconfirmed change).

gavinandresen referenced this in commit 085c62149a on Feb 15, 2014

gavinandresen merged this on Feb 15, 2014

gavinandresen closed this on Feb 15, 2014

gavinandresen deleted the branch on Mar 13, 2014

MarcoFalke referenced this in commit 2a0164085b on Oct 10, 2019

DrahtBot locked this on Sep 8, 2021