core/consensus: fix qbft optimistic first qcommit

[KaloyanTanev]

Previously qCommit uses optimistically the first in the list. However, we never filtered the invalid ones, so it may as well pick up an invalid qCommit. Ensure it always picks a qCommit that has value the same as the one that is actually proposed.

The PR also includes many more qbft test cases capturing various bad weather scenarios.

N.B.: linter fails, but I've fixed this in #4619. Will rebase on main after it is merged, so that linter passes here as well.

category: bug
ticket: none

[Copilot]

🟡 Changes recommended

The new commit-round selection helper is order-dependent and can be influenced by attacker-controlled justification ordering, potentially skewing leader attribution or causing panics; it should be made robust before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR hardens the QBFT “optimistic” decided-path to ensure the decided value is derived from the actually-agreed value hash rather than assuming qcommit[0] is valid, preventing invalid/malicious qcommit ordering from influencing decision handling.

Changes:

• Track and resend the decided value explicitly in core/qbft (avoid using qCommit[0].Value()).
• Thread the agreed value hash into the consensus-layer decide callback and use it to resolve the decided value from the values map.
• Add an internal regression test ensuring extra non-matching COMMITs in MsgDecided justification don’t affect the decided value.

File summaries

File	Description
core/qbft/qbft.go	Stores the agreed qcommit value separately and uses it when rebroadcasting MsgDecided.
core/consensus/qbft/qbft.go	Extends the decide callback to include the agreed value hash; uses it when extracting the decided value; adds helper for commit-round selection.
core/consensus/qbft/sniffed_internal_test.go	Updates test callback signature to match the new decide callback shape.
core/consensus/qbft/qbft_internal_test.go	Adds a regression test for decided justification containing extra non-matching commits.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Changes recommended

The consensus Decide implementation can still be DoS’d by relying on qcommit[0] for the Values() map, allowing an attacker-controlled first commit to suppress delivery of an already-decided value.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

core/consensus/qbft/qbft.go:66

• Decide still relies on qcommit[0] to supply the Values() map. If a byzantine peer can get a COMMIT at index 0 whose values map omits the agreed valueHash (while other commits include it), this will return early and prevent subscribers from receiving the decided value. Prefer looking up valueHash across the qcommit messages rather than assuming index 0 is suitable.

			msg, ok := qcommit[0].(Msg)
			if !ok {
				log.Error(ctx, "Internal error: Invalid message type in qcommit. This indicates a consensus protocol bug that should be reported", nil, z.Str("got_type", fmt.Sprintf("%T", qcommit[0])))
				return

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.91%. Comparing base (ca99f9f) to head (643e972).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4622      +/-   ##
==========================================
+ Coverage   57.73%   57.91%   +0.18%     
==========================================
  Files         246      246              
  Lines       33993    33994       +1     
==========================================
+ Hits        19625    19687      +62     
+ Misses      11905    11836      -69     
- Partials     2463     2471       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

The newly added regression test has compile-time issues (invalid []byte→[32]byte conversion and int64 slice indexing) that must be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

🟡 Changes recommended

The PR description indicates the linter currently fails on this branch and needs a rebase (after #4619) to restore a green build before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Changes recommended

The new tests currently don’t compile because an int64 proto peer index is used directly as a slice index (p2pkeys[j.GetPeerIdx()]).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

core/consensus/qbft/qbft_internal_test.go:1114

• This won’t compile: p2pkeys is a []*k1.PrivateKey (from cluster.NewForT), but j.GetPeerIdx() is int64 and can’t be used as a slice index. Convert the peer index to int before indexing (optionally with bounds checks if you want extra safety in these adversarial tests).

			for _, j := range justification {
				resignedJust = append(resignedJust, signForDuty(
					p2pkeys[j.GetPeerIdx()],
					qbft.MsgType(j.GetType()),
					j.GetPeerIdx(),

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.