Test txn_doublespend.py passes, but should not

dgenr8 commented at 6:26 pm on December 27, 2014: contributor

The check at txn_doublespend.py, line 113 passes, but should not.

Transaction doublespend is created using gather_inputs and does not debit account foo. However, in the course of the test, account metadata is synced to doublespend from introduced-then-conflicted tx1, causing the test to pass.

The old test txnmall.sh checked that SyncMetaData protected accounts from the effects of clones that had been received and possibly confirmed, by syncing metadata from the first-seen spend of an input, to all spends of that input.

The new test does not introduce a clone, but rather a true (non-clone) double-spend. SyncMetaData does not define a clean mapping of accounts for conflicting transactions in general, and no such mapping is really possible. A conflict may respend several inputs, each of which had a different account in its first-spend.

I can commit an IsEquivalentTo check to SyncMetaData, so that only true clones have their metadata synced. This will cause the test to fail. The question is, how should the test be updated? What do we actually want to test regarding double-spends and accounts?

Ping @gavinandresen

dgenr8 commented at 11:27 pm on January 27, 2015: contributor

Ping @jonasschnelli

jonasschnelli commented at 9:21 am on January 28, 2015: contributor

@dgenr8: I didn’t take a close look at this. But it would help if you could update the test and remove/rewrite everything which is related to the accounting system because it has been marked as deprecated recently. And I think double spends are detectable without the accounting system.

If you create a pull request, make sure that the test won’t fail. Because this would mean, Bitcoin-core is malfunction. Rather try to find some double-spend cases. You then need to make sure, your test detects the double spends and will therefor not fail.

When writing a unit or functional test AFTER the implementation of the code which is covered in the test, make sure the test will succeed (unless you detect a bug in the code).

jonasschnelli commented at 9:29 am on January 28, 2015: contributor

@dgenr8: I didn’t take a close look at this. But it would help if you could update the test and remove/rewrite everything which is related to the accounting system because it has been marked as deprecated recently. And I think double spends are detectable without the accounting system.

If you create a pull request, make sure that the test won’t fail. Because this would mean, Bitcoin-core is malfunction. Rather try to find some double-spend cases. You then need to make sure, your test detects the double spends and will therefor not fail.

When writing a unit or functional test AFTER the implementation of the code which is covered in the test, make sure the test will succeed (unless you detect a bug in the code).

laanwj added the label Wallet on Jan 28, 2015

dgenr8 commented at 7:23 pm on January 28, 2015: contributor

@jonasschnelli I like your idea of removing everything accounting-related from the double-spend test. Testing double-spend detection is good, but currently the test’s success depends on syncing accounting data to true double-spends (not good).

We could reinstate the specific malleability test txnmall.sh for as long as accounting behavior matters and mutants are possible. @gavinandresen wrote this new test, so I’m hoping he can weigh in on this.

dgenr8 commented at 5:21 am on March 12, 2015: contributor

Closing, fix is in #5881.

dgenr8 closed this on Mar 12, 2015

MarcoFalke locked this on Sep 8, 2021

Test txn_doublespend.py passes, but should not #5550