concurrency-safe vertex iterators for levelled forest

[AlexHentschel]

Status quo: concurrency limitations of the leveled forest

The levelled forest is an in-memory data structure that stores vertices of a forest-style graph. Internally it uses:

flow-go/module/forest/leveled_forest.go

Lines 24 to 26 in de694d9

	type LevelledForest struct {
	vertices VertexSet
	verticesAtLevel map[uint64]VertexList

utilizing the lower-level structs

• flow-go/module/forest/leveled_forest.go

  Lines 31 to 32 in de694d9

  	type VertexList []*vertexContainer
  	type VertexSet map[flow.Identifier]*vertexContainer

• flow-go/module/forest/leveled_forest.go

  Lines 41 to 48 in de694d9

  	type vertexContainer struct {
  	id flow.Identifier
  	level uint64
  	children VertexList
  	// the following are only set if the block is actually known
  	vertex Vertex
  	}

The forest itself has no internal synchronization and all operations mutate shared state. Hence, concurrent reads and mutations (methods LevelledForest.AddVertex PruneUpToLevel) introduce the usual race conditions

Iterator-specific issue

The core subtlety is that the iterator API reads directly into the internal slice structures of the forest (intentionally avoiding copying for performance). Even with locking of the forest, an iterator may be accessed from a different goroutine than the one holding the mutex. Example:

• Goroutine A holds a mutex protecting the forest and constructs an iterator.
• Goroutine B adds another vertex (acquiring the forest lock after A released it), which is written into the slice that A's iterator reads from.

Specifically, the VertexIterator internally references a VertexList slice, which is owned by the forest. If the forest mutates that slice concurrently, we have to worry that an iterator may:

• read inconsistent data,
• read from a stale slice that was de-referenced by the forest,
• experience index-out-of-bounds panics,
• ignore newly appended elements.

In other words: the concurrency hazard is not the iterator’s logic but the fact that the iterator reads from a slice that can be concurrently mutated by other goroutines updating the forest.

Why the forest itself does not need stronger concurrency guarantees

For performance reasons, the forest is not concurrency safe, since it is very easy for the higher-level logic to provide synchronization if desired (wrapping all forest operations in a mutex, see code snippets below). Because all structural mutations funnel through a narrow API surface, this is marginal extra work.

Exemplary code snippets

forestMutex  sync.RWMutex

// mutations must use write lock
forestMutex.Lock()
forest.AddVertex(...)
forestMutex.Unlock()

forestMutex.Lock()
forest.PruneUpToLevel(...)
forestMutex.Unlock()

// reads acquiring read lock only
forestMutex.RLock()
n := forest.GetNumberOfVerticesAtLevel(...)
forestMutex.RUnlock()

forestMutex.RLock()
chdrn := forest.GetChildren(...)
forestMutex.RUnlock()

Focus of this PR: supportive primitives are required for concurrency-safe iterators

At the hear of the challenge lies the fact that iterator references forest-owned slices. Recalling elements from the iterator is trivially safe if the forest were to never mutate that slice underneath the iterator. However, this would defeat the purpose of a concurrent environment. The iterator therefore requires support from the forest to guarantee that the portion of the data is is reading is not modified concurrently:

1. The iterator holds a stable reference to some backing array.
2. The subset of elements that the iterator is reading from the backing array is immutable.

PR Summary

This PR provides a small wrapper (VertexIteratorConcurrencySafe) hat can be applied to the non-concurrency safe VertexIterator. Algorithmically, the wrapper is very light and efficient, because it does not copy the data. However, the challenge is proving it's concurrency safety, which is the main contribution of this PR.

Extensive documentation and tests validate the concurrency model and ensure the forest's continuing operations do not interfering with iterator.

The forest itself remains intentionally non–thread-safe, since callers can trivially guard forest mutations using a mutex.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[codecov-commenter]

Codecov Report

❌ Patch coverage is 20.00000% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
module/forest/concurrency_helpers.go	20.00%	8 Missing ⚠️

📢 Thoughts on this report? Let us know!

[peterargue]

looks good, thanks for improving these docs.

One general comment, I think the docs as a whole make it clear, but it would be easy to confuse the use case of VertexIteratorConcurrencySafe.

The intention is to allow concurrent use of a single iterator. I could see people making the assumption it was for concurrent access to the forest.

[peterargue]

Suggested change

	// safe. By design, the ResultForest guarantees this. Hence, construction
	// safe. By design, the LevelledForest guarantees this. Hence, construction

[peterargue]

Suggested change

	t.Run(fmt.Sprintf("fully filled non-empty slice (len = 10, cap = 10)"), func(t *testing.T) {
	t.Run("fully filled non-empty slice (len = 10, cap = 10)", func(t *testing.T) {

[peterargue]

Suggested change

	// - we NOT COPY the list's containers for efficiency.
	// - we do NOT COPY the list's containers for efficiency.