Skip to content
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.

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

Make claimpegin fail in the case of a double claim #615

Closed

Conversation

stevenroose
Copy link
Member

@stevenroose stevenroose commented May 8, 2019

Fixes #613. Builds on #614.

This bug got introduced with the 0.17 rebase. In 0.14.1 the double pegin
claim check was done in the beginning of the validation logic, where the
coins view was the actual UTXO set. In 0.17, Core moved several checks
into a new method called CheckTxInputs in tx_verify.cpp, where we also
put the pegin check.  In the mempool acceptance check, however (as
opposed to the ConnectBlock check), CheckTxInputs is called with only a
partial UTXO view for optimization reasons. This commit adds an extra
pegin check in the beginning of the mempool code where we have a full
UTXO view.
Currently it will commit to the transaction without it being able to
enter the mempool. This makes the block creation code include the tx
into the block and create an invalid block.
- changes the expected behavior of claimpegin to fail on double spend
- adds extra test to make sure a block with a double spend is also not
  accepted
@@ -226,8 +232,11 @@ def run_test(self):

pegtxid1 = sidechain.claimpegin(raw, proof)
# Make sure it can't get accepted twice.
pegtxid2 = sidechain.claimpegin(raw, proof)
assert(pegtxid2 not in sidechain.getrawmempool())
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

use assert_raises_rpc_error instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

also make it clearer that this is a mempool-mempool conflict, versus:

block-mempool

block-block

sidechain.generate(7)
if sidechain.gettransaction(pegtxid2)["confirmations"] > 0:
raise Exception("Pegin should not be able to get confirmed")
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

use assert_raises_rpc_error instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

note that this is block-mempool conflict

doublespendblock.hashMerkleRoot = doublespendblock.calc_merkle_root()
add_witness_commitment(doublespendblock)
doublespendblock.solve()
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

use assert_raises_rpc_error instead

@instagibbs
Copy link
Collaborator

247bcb08d1b87ab7d00d75d57948f1d97dfe55c4 : I don't think the commit message is right. Even if you commit to a transaction that is mempool-invalid, it won't get included into the block template. Only things in mempool are considered as far as I know.

@instagibbs
Copy link
Collaborator

We should try covering all the scenarios:

mempool-mempool
block-mempool
blockA-blockB
blockA-blockA(different txs)
blockA-blockA(same tx) (this was the Core inflation bug)

@stevenroose
Copy link
Member Author

Closing, merged into #614.

@stevenroose stevenroose closed this May 8, 2019
@stevenroose stevenroose deleted the fix-claimpegin branch February 2, 2020 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

claimpegin should not succeed if it is double-claiming a tx already in mempool or blockchain
2 participants