Skip to content

Problem: Stateful validation doesn't raise double spend exception#2422

Merged
ttmc merged 2 commits intobigchaindb:masterfrom
kansi:iss/raise-error-on-double-spends
Jul 31, 2018
Merged

Problem: Stateful validation doesn't raise double spend exception#2422
ttmc merged 2 commits intobigchaindb:masterfrom
kansi:iss/raise-error-on-double-spends

Conversation

@kansi
Copy link
Copy Markdown
Contributor

@kansi kansi commented Jul 31, 2018

Solution: Transaction.validate should raise exception DoubleSpend if the given that the transaction is already a part of the database.

@kansi kansi requested review from ldmberman and ttmc July 31, 2018 09:11

try:
self.started_queue.get_nowait()
self.started_queue.get(timeout=0.01)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some debugging I found that even when the stated_queue contained the message 'STARTED' the get_nowait would return to soon and hence the correct exception RuntimeError wasn't being raised. I have changed it to use a timeout of 10 milliseconds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: This fix is to avoid random failure for test tests/test_events.py::test_event_handler_raises_when_called_after_start when running tests.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 31, 2018

Codecov Report

Merging #2422 into master will not change coverage.
The diff coverage is 50%.

@@           Coverage Diff           @@
##           master    #2422   +/-   ##
=======================================
  Coverage   88.43%   88.43%           
=======================================
  Files          39       39           
  Lines        2275     2275           
=======================================
  Hits         2012     2012           
  Misses        263      263

spent = bigchain.get_spent(input_txid, input_.fulfills.output,
current_transactions)
if spent and spent.id != self.id:
if spent:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a more strict condition. IMHO there is no need to match ids i.e. if there a txn which spends the current input then regardless of the transaction id it should be considered a double spend.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because we also check to see if the current transaction (self) has more than one input trying to spend the same output (below). Actually, the line:

raise DoubleSpend('tx "{}" spends inputs twice'.format(self.id))

should say

raise DoubleSpend('tx "{}" spends the same output more than once'.format(self.id))

Maybe make that change in this PR as well, because it helps to make the logic more clear.

Solution: Transaction.validate should raise exception DoubleSpend if the given
transaction is already a part of the database
@kansi kansi force-pushed the iss/raise-error-on-double-spends branch from 86509fb to 22b462a Compare July 31, 2018 09:42
Copy link
Copy Markdown
Contributor

@ttmc ttmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested a small change in a related line of code.

Solution: The exception message should state that the double spend is because of
spending the same input more than once in the transaction
@ttmc
Copy link
Copy Markdown
Contributor

ttmc commented Jul 31, 2018

I don't know what Codecov is upset about. I'm going to ignore it.

@ttmc ttmc merged commit f13c9a9 into bigchaindb:master Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants