Skip to content

Fix and improve txn_doublespend.py test #5881

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 3 commits into from
Jul 2, 2015
Merged

Fix and improve txn_doublespend.py test #5881

laanwj merged 3 commits into from
Jul 2, 2015

Conversation

@dgenr8
Copy link
Contributor

@dgenr8 dgenr8 commented Mar 12, 2015

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 dgenr8 mentioned this pull request Mar 12, 2015
Closed
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Mar 12, 2015

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 dgenr8 force-pushed the better_doublespend_test branch from 2152222 to d48eaf9 Compare March 12, 2015 14:39
@laanwj laanwj added the Wallet label Mar 13, 2015
@dgenr8 dgenr8 mentioned this pull request Mar 13, 2015
Closed
@gavinandresen
Copy link
Contributor

gavinandresen commented Mar 18, 2015

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 dgenr8 force-pushed the better_doublespend_test branch 2 times, most recently from e433686 to d92e611 Compare March 24, 2015 04:18
@dgenr8
Copy link
Contributor Author

dgenr8 commented Mar 24, 2015

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 dgenr8 mentioned this pull request Apr 8, 2015
Merged
dgenr8 added 2 commits April 11, 2015 19:03
@dgenr8
Implement CTransaction::IsEquivalentTo(...)
b2b3619 
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.
@dgenr8
Better txn_doublespend.py test
defd2d5 
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.
@dgenr8 dgenr8 force-pushed the better_doublespend_test branch from d92e611 to 62be2e2 Compare April 12, 2015 02:15
@dgenr8
Copy link
Contributor Author

dgenr8 commented Apr 12, 2015

  • Problem illustration squashed, archived here.
  • txn_clone.py updated to use rpc generate(...)

@laanwj
Copy link
Member

laanwj commented Apr 15, 2015

ACK on new test, for discussion with regard to adding a field to createrawtransaction RPC see #5936

@laanwj laanwj added the Tests label Apr 15, 2015
@dgenr8 dgenr8 force-pushed the better_doublespend_test branch from 62be2e2 to 8b224dd Compare May 12, 2015 00:00
@dgenr8
Copy link
Contributor Author

dgenr8 commented May 12, 2015

Removed the change to createrawtransaction.

@dgenr8
Add txn_clone.py test
5d34e16 
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.
@dgenr8 dgenr8 force-pushed the better_doublespend_test branch from 8b224dd to 5d34e16 Compare May 12, 2015 00:09
@dgenr8 dgenr8 mentioned this pull request Jun 1, 2015
Merged
@dgenr8 dgenr8 mentioned this pull request Jun 15, 2015
Closed
@laanwj laanwj merged commit 5d34e16 into bitcoin:master Jul 2, 2015
laanwj added a commit that referenced this pull request Jul 2, 2015
@laanwj
Merge pull request #5881
3203a08 
5d34e16 Add txn_clone.py test (Tom Harding)
defd2d5 Better txn_doublespend.py test (Tom Harding)
b2b3619 Implement CTransaction::IsEquivalentTo(...) (Tom Harding)
laanwj added a commit that referenced this pull request Jul 2, 2015
@laanwj
tests: fix txn_clone.py
3f16971 
Solve merge conflict of test added in #5881 with #6097.
laanwj added a commit to laanwj/bitcoin that referenced this pull request Jul 2, 2015
@laanwj
Move recently introduced CTransAction::IsEquivalentTo to CWalletTx
5a7304b 
CTransAction::IsEquivalentTo was introduced in bitcoin#5881.
This functionality is only useful to the wallet, and should never have
been added to the primitive transaction type.
@laanwj laanwj mentioned this pull request Jul 2, 2015
Merged
jonasschnelli pushed a commit to jonasschnelli/bitcoin that referenced this pull request Jul 8, 2015
@laanwj @jonasschnelli
Move recently introduced CTransAction::IsEquivalentTo to CWalletTx
c9e5cf9 
CTransAction::IsEquivalentTo was introduced in bitcoin#5881.
This functionality is only useful to the wallet, and should never have
been added to the primitive transaction type.
@str4d str4d mentioned this pull request Feb 14, 2017
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Feb 15, 2017
@zkbot
Auto merge of #2101 - str4d:2074-tests, r=<try>
57d420e 
Bitcoin 0.12 test PRs 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6337
- bitcoin/bitcoin#5881
- bitcoin/bitcoin#6365
- bitcoin/bitcoin#6390
- bitcoin/bitcoin#6414
- bitcoin/bitcoin#5515
- bitcoin/bitcoin#6287 (partial, remainder included in bitcoin/bitcoin#6703)
- bitcoin/bitcoin#6465

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Feb 20, 2017
@zkbot
Auto merge of #2101 - str4d:2074-tests, r=<try>
88c209d 
Bitcoin 0.12 test PRs 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6337
- bitcoin/bitcoin#5881
- bitcoin/bitcoin@3f16971
- bitcoin/bitcoin#6365
- bitcoin/bitcoin@56dc704
- bitcoin/bitcoin#6390
- bitcoin/bitcoin#6414
- bitcoin/bitcoin#5515
- bitcoin/bitcoin#6287 (partial, remainder included in bitcoin/bitcoin#6703)
- bitcoin/bitcoin#6465

Part of #2074.
@dgenr8 dgenr8 deleted the better_doublespend_test branch September 19, 2018 14:28
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dgenr8 @jonasschnelli @gavinandresen @laanwj