Improve double pegin claim behavior #614
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
instagibbs
reviewed
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. |
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
I added some explanation.
Contributor
|
Can you also explain the regression from 0.14 in the OP for people reading code history? |
stevenroose
force-pushed
the
fix-double-pegin
branch
from
ed73b8c to
5d9b098
Compare
Contributor
Author
|
stevenroose
force-pushed
the
fix-double-pegin
branch
from
5d9b098 to
5c4b0f1
Compare
stevenroose
changed the title
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.
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
force-pushed
the
fix-double-pegin
branch
from
5c4b0f1 to
52ef283
Compare
Contributor
Author
|
I think I also addressed all comments on the other PR about using |
- 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)
instagibbs
reviewed
May 8, 2019
| // 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 */); |
Contributor
|
ACK! |
instagibbs
added a commit
that referenced
this pull request
May 8, 2019
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #612 and #613.
claimpeginthrow an exception when trying to double claim a pegin