consensus: use signer pubkey to check for unique signatures and optimize performance, close XFN-03

[wanwiset25]

Proposed changes

use signer pubkey to check for unique signatures
ref audit ticket: XFN-03

Types of changes

What types of changes does your code introduce to XDC network?
Put an ✅ in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Update (if none of the other choices apply)
• Regular KTLO or any of the maintaince work. e.g code style
• CICD Improvement

Impacted Components

Which part of the codebase this PR will touch base on,

Put an ✅ in the boxes that apply

• Consensus
• Account
• Network
• Geth
• Smart Contract
• External components
• Not sure (Please specify below)

Checklist

Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that

• This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
• Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
• Tested the backwards compatibility.
• Tested with XDC nodes running this version co-exist with those running the previous version.
• Relevant documentation has been updated as part of this PR
• N/A

[wanwiset25]

ref #1643

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch XFN-03-v2

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gzliudan]

@wanwiset25 Why close this PR ?

[wanwiset25]

@gzliudan good suggestions, please check new optmize

[Copilot]

Pull request overview

This PR addresses audit ticket XFN-03 by refactoring signature verification to use signer public keys instead of signature bytes for uniqueness checking, improving both correctness and performance.

• Introduces a new countValidSignatures function that performs concurrent signature verification and uniqueness checking based on signer pubkeys
• Refactors verifyQC and verifyTC functions to use the new unified verification approach
• Removes the old UniqueSignatures function that checked uniqueness based on signature bytes

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
consensus/XDPoS/engines/engine_v2/utils.go	Adds new countValidSignatures function with concurrent signature verification and pubkey-based uniqueness checking; removes old UniqueSignatures function; enhances logging in verifyMsgSignature
consensus/XDPoS/engines/engine_v2/engine.go	Refactors verifyQC to use countValidSignatures and removes inline verification logic
consensus/XDPoS/engines/engine_v2/timeout.go	Refactors verifyTC to use countValidSignatures and removes inline verification logic
consensus/tests/engine_v2_tests/verify_header_test.go	Updates test assertion to check for new "duplicate signing found" error message

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The loop variables i and signature are being captured by the goroutine closure, which creates a race condition. All goroutines will share the same variables, and by the time they execute, these variables will likely have been modified by subsequent loop iterations. This means multiple goroutines may access the same index or signature, leading to incorrect results or data races when writing to signatureList[i] and pubkeys[i].

The loop variables must be captured properly by passing them as parameters to the goroutine or by creating local copies within the loop body before starting the goroutine.

Suggested change

	eg.Go(func() error {
	select {
	case <-ctx.Done():
	return ctx.Err()
	default:
	verified, signerAddress, err := x.verifyMsgSignature(messageHash, signature, candidates)
	if err != nil {
	log.Error("[verifySignatures] Error while verfying QC message signatures", "error", err)
	return err
	}
	if !verified {
	return fmt.Errorf("signature verification failed, signer is not part of masternode list. Signature: %v, SignedMessage: %v, SignerAddress: %v, Masternodes: %v", signature, messageHash.Hex(), signerAddress, candidates)
	}
	signatureList[i] = signature
	pubkeys[i] = signerAddress
	idx := i
	sig := signature
	eg.Go(func() error {
	select {
	case <-ctx.Done():
	return ctx.Err()
	default:
	verified, signerAddress, err := x.verifyMsgSignature(messageHash, sig, candidates)
	if err != nil {
	log.Error("[verifySignatures] Error while verfying QC message signatures", "error", err)
	return err
	}
	if !verified {
	return fmt.Errorf("signature verification failed, signer is not part of masternode list. Signature: %v, SignedMessage: %v, SignerAddress: %v, Masternodes: %v", sig, messageHash.Hex(), signerAddress, candidates)
	}
	signatureList[idx] = sig
	pubkeys[idx] = signerAddress

[Copilot]

Spelling error: "verfying" should be "verifying".

Suggested change

	log.Error("[verifySignatures] Error while verfying QC message signatures", "error", err)
	log.Error("[verifySignatures] Error while verifying QC message signatures", "error", err)

[Copilot]

Spelling error: "verfying" should be "verifying".

Suggested change

	log.Error("[verifyTC] Error while verfying TC message signatures", "tcRound", timeoutCert.Round, "tcGapNumber", timeoutCert.GapNumber, "tcSignLen", len(timeoutCert.Signatures), "Error", err)
	log.Error("[verifyTC] Error while verifying TC message signatures", "tcRound", timeoutCert.Round, "tcGapNumber", timeoutCert.GapNumber, "tcSignLen", len(timeoutCert.Signatures), "Error", err)

[Copilot]

Spelling error: "verfying" should be "verifying".

Suggested change

	log.Error("[verifyQC] Error while verfying QC message signatures", "Error", err)
	log.Error("[verifyQC] Error while verifying QC message signatures", "Error", err)

[Copilot]

The signature validation at line 174 uses snap.NextEpochCandidates to verify signatures, but the threshold check at line 185 uses epochInfo.MasternodesLen. This inconsistency could lead to incorrect validation if these two lists have different lengths. The TODO comment on line 173 acknowledges this issue but it's concerning that the threshold check happens against a different set of masternodes than what was used for signature verification.

Suggested change

	if float64(numValidSignatures) < float64(epochInfo.MasternodesLen)*certThreshold {
	log.Warn("[verifyTC] Invalid TC Signature is less or empty", "tcRound", timeoutCert.Round, "tcGapNumber", timeoutCert.GapNumber, "tcSignLen", len(timeoutCert.Signatures), "certThreshold", float64(epochInfo.MasternodesLen)*certThreshold)
	if float64(numValidSignatures) < float64(len(snap.NextEpochCandidates))*certThreshold {
	log.Warn("[verifyTC] Invalid TC Signature is less or empty", "tcRound", timeoutCert.Round, "tcGapNumber", timeoutCert.GapNumber, "tcSignLen", len(timeoutCert.Signatures), "certThreshold", float64(len(snap.NextEpochCandidates))*certThreshold)