Hacktoberfest 2026: the issues maintainers tagged for October, open and beginner-friendly. Browse Hacktoberfest issues

Follow-ups to #681: decision count for a stopped view, regression tests, and cleanups

Open
  • Is something wrong?
#714 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
35/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
go
Domain
distributed-systems

Research direction

Start by reading #681 and the controller and view changer code around Controller.decide, run, sync, and commitInFlightProposal; the issue does not name source or test file paths. Check completion_signal_test.go and trace how aborts interact with buffered decisions and in-flight Decide or Sync calls. Done means fixing the stopped-current-view decision count, adding tests that fail on the old behavior, and addressing the listed cleanups; the optional late-result recovery is a separate improvement.

Written by the indexing model from the issue text.

Description

Written by Claude (Anthropic's AI assistant) on behalf of Hagar Meir.

These are the points left open in the review of #681 (avoid blocking on completion signals when the waiter has left). They should be done in a separate PR once #681 is merged.

1. Decision count when the deciding view is still current but stopped (possible small bug)

In Controller.decide, isRunningCurrentView(d.view) returns false in two different cases:

  • The view was replaced. Skipping all of the view bookkeeping is right.
  • The view is still current but already stopped, for example when the run loop handles abortViewChan before the decision buffered in decisionChan. The decision was delivered in this view, and the checkpoint moved to its sequence, but incrementCurrentDecisionsInView is skipped.

In the second case, if the node then syncs and finds nothing newer, it restarts the same view with a count that is one too low:

  • run calls changeView(c.getCurrentViewNumber(), vs.ProposalSeq, c.getCurrentDecisionsInView()), or
  • sync keeps newDecisionsInView = c.getCurrentDecisionsInView() when the latest decision is at the controller's sequence.

With leader rotation on, getLeaderID can then give this node a different leader from the other nodes, until the next view change resets the count.

This is not a regression: before #681, the send on deliverChan failed in the same case and the increment was skipped too.

Proposed fix: when d.view is the current view but stopped, still call incrementCurrentDecisionsInView, and skip only checkIfRotate/changeView and acquireLeaderToken, the steps that could race the pending view change. Add a test for it.

2. Regression tests that fail on the old code

The tests added in #681 (completion_signal_test.go) pass on the old code as well:

  • decide now only closes d.delivered, and close never blocks;
  • the in-flight tests call decideInFlight and syncInFlight directly, and their non-blocking sends never block either.

Tests that reproduce the original hangs would catch a regression:

  • controller: Decide returns through the view's abort channel while the decision is still buffered in decisionChan, and the run loop must keep processing later events;
  • view changer: commitInFlightProposal times out or is stopped, and the deferred inFlightView.Abort() must not hang on a late Decide or Sync from the in-flight view.
3. Inconsistent nil checks

decide checks d.delivered != nil, but Decide always sets it. Meanwhile a nil d.view panics in isRunningCurrentView. The two fields are always set together, so both checks can be removed.

4. Comments in decide
  • "the current view must not be rotated or lead because of it" is unclear. It could say that the leader token is not acquired.
  • It could also say why skipping acquireLeaderToken is fine for a replaced view: the new view's startView acquires the token when needed.
5. Optional: in-flight attempt result after a late decision

If the in-flight attempt times out or is stopped while the in-flight view is delivering its decision, the deferred Abort() waits for the delivery to finish, so the proposal was committed. commitInFlightProposal still returns false, and the node waits for the next view change. Since decideCh is buffered, a non-blocking receive on it after the abort could return true instead. This only affects how quickly the node recovers, not correctness.

Dominant language
Go
Stars
110
Forks
40
Avg merge
1d 6h
Merged PRs (30d)
14

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from hyperledger/SmartBFT

All issues in hyperledger/SmartBFT

Similar issues

More Go issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.