Fix and improve txn_doublespend.py test #5881

pull dgenr8 wants to merge 3 commits into bitcoin:master from dgenr8:better_doublespend_test changing 6 files +234 −27

dgenr8 commented at 1:22 AM on March 12, 2015: contributor

The first commit illustrates a problem with the current txn_doublespend.py test. The test relies on unsupportable behavior in the wallet's SyncMetaData method. The test is made to produce inconsistent results by making a small change to specific test "amount" parameters.

The second commit introduces a primitive method CTransaction::IsEquivalentTo and uses it in the wallet to ensure that SyncMetaData is only called for malleability clones, not general conflict (doublespend) transactions. This makes the output of the illustrative version of txn_doublespend.py consistent and predictable.

The third commit is a revised txn_doublespend.py test that does not rely on broken SyncMetaData. Instead, it expects the fixed version of SyncMetaData. It also removes a dependency on the soon-to-be-deprecated accounting "move" method.
dgenr8 cross-referenced this on Mar 12, 2015 from issue Test txn_doublespend.py passes, but should not by dgenr8
jonasschnelli commented at 7:30 AM on March 12, 2015: contributor

utACK. Travis fails test_bitcoin: chainparams.cpp:286: const CChainParams& Params(): Assertion 'pCurrentParams' failed.. You probably need to rebase because there was a travis error after a certain commit on master.
dgenr8 force-pushed on Mar 12, 2015
laanwj added the label Wallet on Mar 13, 2015
dgenr8 cross-referenced this on Mar 13, 2015 from issue Relay first double-spend as alert, notify wallet by dgenr8
gavinandresen commented at 8:28 PM on March 18, 2015: contributor

I don't like the new unit test-- you are testing the new "" account gets debited on a non-malleated doublespend.

But our wallet code will never produce a non-malleated double-spend (at least not without the user jumping through some hoops to copy the wallet to two machines, etc etc etc).

I'd be happier if the new unit test tested the new code, and created a malleated version of the spend-to-foo and made sure The Right Thing happened.

I'm also uncertain about changing the SyncMetaData behavior to only work for malleated transactions, but I think I can live with that (mostly because what you get depends on the order your node sees transactions, accounts are being deprecated, and non-malleated double-spends of a "spendfrom" should never happen unless you jump through hoops).
dgenr8 force-pushed on Mar 24, 2015
dgenr8 force-pushed on Mar 24, 2015
dgenr8 commented at 4:28 AM on March 24, 2015: contributor
Per @gavinandresen review:
- Improved implementation of IsEquivalentTo
- Added txn_clone.py, which tests malleability clone accounting
- Updated rpc-tests.sh to call the new test with and without the --mineblock option
The new test relies on setting nlocktime via createrawtransaction. There is also a stand-alone #5936 for this change.
dgenr8 cross-referenced this on Apr 8, 2015 from issue [RPC] Add optional locktime to createrawtransaction by dgenr8

Implement CTransaction::IsEquivalentTo(...)

Define CTransaction::IsEquivalentTo(const CTransaction& tx)

True if only scriptSigs are different.  In other words, true if
the two transactions are malleability clones.  In other words,
true if the two transactions have the same effect on the
outside universe.

In the wallet, only SyncMetaData for equivalent transactions.

b2b3619262

Better txn_doublespend.py test

Remove reliance on accounting "move" ledger entries.  Instead,
create funding transactions (and deal with fee complexities).

Do not rely on broken SyncMetaData.  Instead expect double-spend
amount to be debited from the default "" account.

defd2d55b7

dgenr8 force-pushed on Apr 12, 2015
dgenr8 commented at 2:35 AM on April 12, 2015: contributor
- Problem illustration squashed, archived here.
- txn_clone.py updated to use rpc generate(...)
laanwj commented at 10:34 AM on April 15, 2015: member

ACK on new test, for discussion with regard to adding a field to createrawtransaction RPC see #5936
laanwj added the label Tests on Apr 15, 2015
dgenr8 force-pushed on May 12, 2015
dgenr8 commented at 12:08 AM on May 12, 2015: contributor

Removed the change to createrawtransaction.

Add txn_clone.py test

Does what the old txnmall.sh test did.

Creates an equivalent malleated clone and tests that SyncMetaData
syncs the accounting effects from the original transaction to the
confirmed clone.

5d34e16d3a

dgenr8 force-pushed on May 12, 2015
dgenr8 cross-referenced this on Jun 1, 2015 from issue Port/fix txnmall.sh regression test by gavinandresen
dgenr8 cross-referenced this on Jun 15, 2015 from issue Fix conflict tests by dgenr8
laanwj merged this on Jul 2, 2015
laanwj closed this on Jul 2, 2015
laanwj referenced this in commit 3203a0832a on Jul 2, 2015
laanwj referenced this in commit 3f16971442 on Jul 2, 2015
laanwj referenced this in commit 5a7304b69d on Jul 2, 2015
laanwj cross-referenced this on Jul 2, 2015 from issue Move recently introduced CTransAction::IsEquivalentTo to CWalletTx by laanwj
jonasschnelli referenced this in commit c9e5cf9688 on Jul 8, 2015
str4d cross-referenced this on Feb 14, 2017 from issue Bitcoin 0.12 test PRs 1 by str4d
zkbot referenced this in commit 57d420e2f8 on Feb 15, 2017
zkbot referenced this in commit 88c209dba6 on Feb 20, 2017
dgenr8 deleted the branch on Sep 19, 2018
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#4570 Relay first double-spend as alert, notify wallet #5317 Port/fix txnmall.sh regression test #5550 Test txn_doublespend.py passes, but should not #5936 [RPC] Add optional locktime to createrawtransaction #6365 Move recently introduced CTransAction::IsEquivalentTo to CWalletTx