fix: do not check chainlock state in IsTxSafeForMining #5444
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
PastaPastaPasta
force-pushed
the
cl_dis_non_safe
branch
from
4c8d4d7 to
a85e6be
Compare
knst
reviewed
Jun 19, 2023
… mine non-locked txes
- wait for islocks on connected nodes - bump mocktime for 10 min on the isolated node
Member
|
Adding guix-build to this PR to ensure that guix-2 didn't break guix CI somehow; didn't break |
PastaPastaPasta
approved these changes
Jun 21, 2023
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK for squash merge
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Issue being fixed or feature implemented
Disabled or non-enforced Chainlocks does not mean you can safely mine non-locked txes, you could end up mining a block that is going to be rejected by everyone else if a conflicting tx (missing on your node) would be IS-locked. I can't find any reason why we have this besides "if Chainlocks are disabled then smth is wrong so let them all be mined" but we have spork_2 and spork_3 to control IS behaviour and we check them in
IsTxSafeForMiningalready, that would be a much more straightforward way to deal with a potential issue.Noticed this while reviewing #5150 and also while testing v19.2 during recent testnet v19 re-fork.
What was done?
Drop this check, adjust tests
How Has This Been Tested?
Run tests locally
Breaking Changes
Not quote breaking changes but a change in behaviour: with CLs disabled it will now take 10 minutes for non-locked txes to be mined, same as when CLs are enabled.
Checklist: