-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: process/accept CbTx CL as new CL #5779
Conversation
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-invalid-clsig"); | ||
} | ||
chainlock_handler.ProcessNewChainLock(-1, cbtxcl, ::SerializeHash(cbtxcl)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a couple of issues with this:
- A syncing node will spam the network with old clsigs because of
RelayInv
inside ofProcessNewChainLock
. - We are going to run
VerifyChainLock
twice in a row it seems - first here and then again inside ofProcessNewChainLock
.
We should probably avoid processing any clsigs (these and p2p ones) until blockchain is synced. Pls see 93ed4e1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we instead extract this code
Lines 141 to 161 in d4e8aa7
{ | |
LOCK(cs); | |
bestChainLockHash = hash; | |
bestChainLock = clsig; | |
if (pindex != nullptr) { | |
if (pindex->nHeight != clsig.getHeight()) { | |
// Should not happen, same as the conflict check from above. | |
LogPrintf("CChainLocksHandler::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n", | |
__func__, clsig.ToString(), pindex->nHeight); | |
// Note: not relaying clsig here | |
return; | |
} | |
bestChainLockWithKnownBlock = bestChainLock; | |
bestChainLockBlockIndex = pindex; | |
} | |
// else if (pindex == nullptr) | |
// Note: make sure to still relay clsig further. | |
} |
So avoid the re-verification and enforcement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might work.. not sure
Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen. |
2 similar comments
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This pull request has conflicts, please rebase. |
Issue being fixed or feature implemented
We should process/accept CbTx CL as new CL as we process/accept new blocks.
What was done?
How Has This Been Tested?
Breaking Changes
Checklist: