peg-in re-checking cleanup and fixes #425
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
Contributor
Author
|
Checked logs to make sure the spam is gone, and logic looks sound: |
stevenroose
reviewed
Oct 1, 2018
| # Don't make a block, race condition when pegin-invalid block | ||
| # is awaiting further validation, nodes reject subsequent blocks | ||
| # even ones they create | ||
| print("Now waiting for node to re-evaluate peg-in witness failed block... should take a few seconds") |
Contributor
There was a problem hiding this comment.
Now waiting for node to ...
I was expecting that to be followed by a sleep..
| // Check existence and validity of pegin witness | ||
| if (tx.wit.vtxinwit.size() <= i || !IsValidPeginWitness(tx.wit.vtxinwit[i].m_pegin_witness, prevout)) { | ||
| return state.DoS(0, false, REJECT_PEGIN, "bad-pegin-witness", true); | ||
| return state.DoS(0, false, REJECT_PEGIN, "bad-pegin-witness"); |
Contributor
There was a problem hiding this comment.
What exactly does this change behavior-wise?
Contributor
Author
There was a problem hiding this comment.
I note this in the issue itself but:
-
Blocks: block is marked bad and not reprocessed until the next retry interval
-
Transactions: transaction is rejected, like normal.
| '-mainchainrpcport=%s' % rpc_port(n), | ||
| '-mainchainrpcuser=%s' % rpc_u, | ||
| '-mainchainrpcpassword=%s' % rpc_p, | ||
| '-recheckpeginblockinterval=15', # Long enough to allow failure and repair before timeout |
Contributor
There was a problem hiding this comment.
You don't seem to wait for 15 seconds ever, so if you just want to make sure all things happen before the timeout, why not use the default?
Contributor
Author
There was a problem hiding this comment.
The node takes 15 seconds to re-try the block, so we do use it. I don't want it lost in node start/stop time noise.
Contributor
|
Tests work though, at 2716fab. But confused about the not waiting in the fedpeg test. |
Contributor
Author
|
I'll give the code some documentation to clear up some of these questions. |
stevenroose
reviewed
Oct 3, 2018
| * 2) Re-evaluates a list of blocks that have been deemed "bad" | ||
| * from the perspective of peg-in witness validation. Blocks are | ||
| * added to this queue in ConnectTip based on the error code returned. | ||
| */ |
Previously the peg-in validation logic was done in script which means it was handled correctly by mempool/block logic. We then moved these checks outside of script, which means that these checks aren't multithreaded/batched, and return the specific error states, including CorruptionPossible. This means that blocks are never considered permanently invalid and in certain circumstances tried in an infinite loop. Instead we mark it as a normal failure, and allow the peg-in invalid block queue to take care of it instead.
instagibbs
force-pushed
the
peg_in_corrupt
branch
from
4a2ba83 to
abb7b01
Compare
Contributor
Author
|
rebased, this time with Travis testing the bitcoind case! |
Contributor
|
tACK 4a2ba83 |
instagibbs
added a commit
that referenced
this pull request
Oct 3, 2018
abb7b01 Add some additional comments about BitcoindRPCCheck infra (Gregory Sanders) 62c56f4 don't set CorruptionPossible for peg-in transaction failure (Gregory Sanders) 5d7cd6b parameterize the rpc re-checking of peg-in validation failed blocks (Gregory Sanders) ba8d498 bitcoind rpc check should take main locks for mapBlockIndex, check for key (Gregory Sanders)
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.
Resolves #424
We ensure that peg-in invalid blocks are marked as "bad", and allow the re-checking logic to actually re-check instead of immediately re-evaluating the block.
Take locks and check for existence of keys in mapBlockIndex, when appropriate.
Also actually includes a valid test for it by making sure it takes more than a few seconds to fix itself but doesn't hit test timeout.