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

Improve double pegin claim behavior #614

Merged
merged 4 commits into from May 8, 2019

Conversation

Projects
None yet
2 participants
@stevenroose
Copy link
Member

commented May 7, 2019

Fixes #612 and #613.

  • prevents a double claim from entering mempool
  • makes claimpegin throw an exception when trying to double claim a pegin
  • check the last commit message on all the extra cases that are tested.

@stevenroose stevenroose added the 0.17 label May 7, 2019

@stevenroose stevenroose requested a review from instagibbs May 7, 2019

// ELEMENTS:
// Don't look for coins that only exist in parent chain
// For pegin inputs check whether the pegins have already been claimed before.

This comment has been minimized.

Copy link
@instagibbs

instagibbs May 7, 2019

Collaborator

need to note this is just a coin UTXO check, not a mempool check at all. At the mempool level we rely on the GetConflictTx check.

This comment has been minimized.

Copy link
@stevenroose

stevenroose May 8, 2019

Author Member

I added some explanation.

@instagibbs

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

Can you also explain the regression from 0.14 in the OP for people reading code history?

@stevenroose stevenroose force-pushed the stevenroose:fix-double-pegin branch from ed73b8c to 5d9b098 May 8, 2019

@stevenroose

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Can you also explain the regression from 0.14 in the OP for people reading code history?

Please verify the commit message of the new commit.

@stevenroose stevenroose force-pushed the stevenroose:fix-double-pegin branch from 5d9b098 to 5c4b0f1 May 8, 2019

@stevenroose stevenroose changed the title Prevent pegin double spends from entering the mempool Improve double pegin claim behavior May 8, 2019

stevenroose added some commits May 7, 2019

Prevent pegin double spends from entering the mempool
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
because the intermediate method CheckInputs was removed and some logic
moved to 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.
Make claimpegin fail in the case of a double claim
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.

@stevenroose stevenroose force-pushed the stevenroose:fix-double-pegin branch from 5c4b0f1 to 52ef283 May 8, 2019

@stevenroose

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

I think I also addressed all comments on the other PR about using raises_rpc_exception and the case-by-case explanation.

Improve the fedpeg test with extra double claim tests
- 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:
  1. a tx with two identical pegin inputs (mempool & block)
  2. a tx claiming a pegin that another mempool tx also claims
  3. a tx claiming a pegin that a confirmed tx already claimed (mempool
     & block)
// To check if it's not double spending an existing pegin UTXO, we check mempool acceptance.
CValidationState acceptState;
bool accepted = ::AcceptToMemoryPool(mempool, acceptState, MakeTransactionRef(mtx), nullptr /* pfMissingInputs */,
nullptr /* plTxnReplaced */, false /* bypass_limits */, maxTxFee, true /* test_accept */);

This comment has been minimized.

Copy link
@instagibbs

instagibbs May 8, 2019

Collaborator

ah, cute!

@instagibbs

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2019

ACK!

@instagibbs instagibbs merged commit 52ef283 into ElementsProject:master May 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

instagibbs added a commit that referenced this pull request May 8, 2019

Merge #614: Improve double pegin claim behavior
52ef283 Improve the fedpeg test with extra double claim tests (Steven Roose)
48a82fa Make claimpegin fail in the case of a double claim (Steven Roose)
af22646 Prevent pegin double spends from entering the mempool (Steven Roose)
50c5570 Report IsValidPeginWitness error message (Steven Roose)

Pull request description:

  Fixes #612 and #613.

  - prevents a double claim from entering mempool
  - makes `claimpegin` throw an exception when trying to double claim a pegin
  - check the last commit message on all the extra cases that are tested.

Tree-SHA512: dd9602cd4bc78a3e8f7a6b566f881ede09dba54900425348c1eb528268a3e869416e3c9ca97c1a8da4d120875ef59c4e21a857256312ce77e06b7b6ae04abf92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.