Report transaction conflicts, and tentative account balance fix #3671

Merged
merged 1 commit into from Feb 15, 2014

Projects

None yet

6 participants

@gavinandresen
Member

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
Member

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

Edit: As with the other patch, 'getunconfirmedbalance' includes the conflicted txes.

@laanwj laanwj commented on an outdated diff Feb 14, 2014
src/wallet.cpp
@@ -445,6 +482,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn)
wtxIn.GetHash().ToString(),
wtxIn.hashBlock.ToString());
}
+ AddToClones(normHash, hash);
@laanwj
laanwj Feb 14, 2014 Member

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

@laanwj laanwj and 1 other commented on an outdated diff Feb 14, 2014
src/wallet.cpp
{
uint256 hash = wtxIn.GetHash();
+ uint256 normHash = wtxIn.GetNormalizedHash();
+
+ if (fFromLoadWallet)
+ {
+ mapWallet[hash] = wtxIn;
+ AddToClones(normHash, hash);
@laanwj
laanwj Feb 14, 2014 Member

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
gavinandresen Feb 14, 2014 Member

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
laanwj Feb 14, 2014 Member

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.

@laanwj laanwj and 1 other commented on an outdated diff Feb 14, 2014
src/wallet.cpp
@@ -385,9 +398,33 @@ void CWallet::MarkDirty()
}
}
-bool CWallet::AddToWallet(const CWalletTx& wtxIn)
+void CWallet::AddToClones(const uint256& normHash, const uint256& hash)
+{
+ // Copy metadata from clone, if there is a clone:
+ if (mapTxClones.count(normHash) > 0)
+ {
+ const uint256& otherTxid = mapTxClones.find(normHash)->second;
+ const CWalletTx& otherTx = mapWallet[otherTxid];
+ CWalletTx& thisTx = mapWallet[hash];
+ thisTx.nTimeSmart = otherTx.nTimeSmart;
+ thisTx.fFromMe = otherTx.fFromMe;
+ thisTx.strFromAccount = otherTx.strFromAccount;
+ thisTx.nOrderPos = otherTx.nOrderPos;
@laanwj
laanwj Feb 14, 2014 Member

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

@laanwj laanwj and 2 others commented on an outdated diff Feb 14, 2014
src/wallet.h
@@ -108,6 +108,10 @@ class CWallet : public CCryptoKeyStore, public CWalletInterface
int64_t nNextResend;
int64_t nLastResend;
+ // Used to detect and report mutated transactions:
+ std::multimap<uint256, uint256> mapTxClones;
@laanwj
laanwj Feb 14, 2014 Member

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

@sipa
sipa Feb 14, 2014 Member

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
gavinandresen Feb 14, 2014 Member

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

@sipa
sipa Feb 14, 2014 Member

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
Member

@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
Member

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
Member

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

@apoelstra
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
Member

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.

@gavinandresen gavinandresen 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.
731b89b
@BitcoinPullTester

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
Member

ACK. (immature coins issue fixed too)

@laanwj
Member
laanwj commented Feb 15, 2014

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

@gavinandresen gavinandresen merged commit 085c621 into bitcoin:master Feb 15, 2014
@gavinandresen gavinandresen deleted the gavinandresen:txn_conflicts branch Mar 13, 2014
@zathey zathey referenced this pull request in txbits/txbits Oct 10, 2014
Closed

Wallet actor can get an incorrect balance #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment