Skip to content

testonly + reftests - #14

Open
phbnf wants to merge 2 commits into
mainfrom
conf-reftests
Open

testonly + reftests#14
phbnf wants to merge 2 commits into
mainfrom
conf-reftests

Conversation

@phbnf

@phbnf phbnf commented Jun 23, 2026

Copy link
Copy Markdown
Owner

tests: better comments, remove prints

@phbnf

phbnf commented Jun 23, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces subtree consistency proof functionality to the Merkle tree implementation, adding a reference implementation for cross-checking, the SubtreeConsistencyProof method on the Tree struct, and corresponding unit tests. The review feedback suggests correcting minor typos in the reference implementation comments and expanding the unit tests to cover additional error cases for invalid subtree ranges.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread testonly/reference_test.go Outdated
Comment on lines +128 to +130
// The subtree is on the left of split. Prove that the subtree is consistent
// with the subtree on the left of split, and record the root of the right
//subtree.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a minor typo in the comment: //subtree. is missing a space after the comment marker.

Suggested change
// The subtree is on the left of split. Prove that the subtree is consistent
// with the subtree on the left of split, and record the root of the right
//subtree.
// The subtree is on the left of split. Prove that the subtree is consistent
// with the subtree on the left of split, and record the root of the right
// subtree.

Comment on lines +142 to +145
// Otherwise, split is between start and end.
// This means that start is 0.
// Prove that the subtree is consistent with the subtree on right of split,
// and record the root of the left subtree.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a minor grammatical typo in the comment: on right of split should be on the right of split.

Suggested change
// Otherwise, split is between start and end.
// This means that start is 0.
// Prove that the subtree is consistent with the subtree on right of split,
// and record the root of the left subtree.
// Otherwise, split is between start and end.
// This means that start is 0.
// Prove that the subtree is consistent with the subtree on the right of split,
// and record the root of the left subtree.

Comment thread testonly/tree_test.go
Comment on lines +204 to +206
if _, err := mt.SubtreeConsistencyProof(0, 6, 3); err == nil {
t.Error("SubtreeConsistencyProof(0, 6, 3) succeeded unexpectedly")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It would be beneficial to also test that SubtreeConsistencyProof correctly returns an error when provided with an invalid subtree range (e.g., where start is not a multiple of bitCeil(end - start)), in addition to testing the end > size error case.

	if _, err := mt.SubtreeConsistencyProof(0, 6, 3); err == nil {
		t.Error("SubtreeConsistencyProof(0, 6, 3) succeeded unexpectedly")
	}
	if _, err := mt.SubtreeConsistencyProof(1, 3, 4); err == nil {
		t.Error("SubtreeConsistencyProof(1, 3, 4) succeeded unexpectedly")
	}

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces subtree consistency proof functionality by adding a reference implementation (refSubtreeConsistencyProof), a SubtreeConsistencyProof method to the Tree struct, and corresponding tests. The review feedback suggests correcting a mathematical proof in the comments of the reference implementation, expanding test coverage to include more invalid subtree boundary conditions, and fixing a minor comment formatting issue.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread testonly/reference_test.go Outdated
Comment on lines +147 to +156
// Proof that start is 0:
// With C = bitCeil(len([start, end))):
// - C is the largest power of 2 within [start, end). Otherwise
// [start, end) would be at least 2C long, which contradicts
// C's definition.
// - `split` is a power of two smaller than end. Since C is the largest
// power of 2 within [start, end), split must be smaller than C.
// - This gives us: `start` < split < C.
// - Since `start` is a multiple of C, and `start` is smaller than C,
// `start` MUST be 0.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The mathematical proof in the comment contains a slight inaccuracy regarding $C = \text{bitCeil}(\text{len})$. Since $C$ is the smallest power of 2 greater than or equal to the length, it is not necessarily "within" the interval (for example, if the interval is $[0, 3)$, then $C = 4$, which is outside the interval).

We can simplify and correct the proof using the properties of powers of 2:

  1. Let $C = \text{bitCeil}(\text{end} - \text{start})$. By definition, $\text{end} - \text{start} \le C$.
  2. Since the subtree is valid, $\text{start}$ is a multiple of $C$ (i.e., $\text{start} = k \cdot C$ for some integer $k \ge 0$).
  3. In this default case, we have $\text{start} &lt; \text{split} &lt; \text{end}$.
  4. This implies $\text{start} &lt; \text{split} &lt; \text{start} + C$.
  5. Since both $\text{split}$ and $C$ are powers of 2:
    • If $\text{split} &lt; C$, then if $k \ge 1$, we would have $\text{split} &lt; C \le \text{start}$, contradicting $\text{start} &lt; \text{split}$. Thus $k$ must be $0$.
    • If $\text{split} \ge C$, then $\text{split}$ must be a multiple of $C$. But there is no multiple of $C$ strictly between $k \cdot C$ and $(k+1) \cdot C$.
  6. Therefore, $k$ must be $0$, which means $\text{start} = 0$.
Suggested change
// Proof that start is 0:
// With C = bitCeil(len([start, end))):
// - C is the largest power of 2 within [start, end). Otherwise
// [start, end) would be at least 2C long, which contradicts
// C's definition.
// - `split` is a power of two smaller than end. Since C is the largest
// power of 2 within [start, end), split must be smaller than C.
// - This gives us: `start` < split < C.
// - Since `start` is a multiple of C, and `start` is smaller than C,
// `start` MUST be 0.
// Proof that start is 0:
// With C = bitCeil(len([start, end))):
// - By definition, end - start <= C.
// - Since the subtree is valid, start is a multiple of C (start = k * C).
// - In this case, start < split < end <= start + C.
// - Since split and C are both powers of 2:
// - If split < C, then if k >= 1, split < C <= start, contradicting start < split.
// - If split >= C, split must be a multiple of C, but no multiple of C lies
// strictly between k * C and (k + 1) * C.
// - Thus, k must be 0, meaning start is 0.

Comment thread testonly/tree_test.go
Comment on lines +204 to +206
if _, err := mt.SubtreeConsistencyProof(0, 6, 3); err == nil {
t.Error("SubtreeConsistencyProof(0, 6, 3) succeeded unexpectedly")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure comprehensive test coverage of the new SubtreeConsistencyProof API, we should also explicitly test other invalid subtree boundary conditions, such as when start >= end or when start is not properly aligned.

	if _, err := mt.SubtreeConsistencyProof(0, 6, 3); err == nil {
		t.Error("SubtreeConsistencyProof(0, 6, 3) succeeded unexpectedly")
	}
	if _, err := mt.SubtreeConsistencyProof(3, 3, 8); err == nil {
		t.Error("SubtreeConsistencyProof(3, 3, 8) succeeded unexpectedly (start >= end)")
	}
	if _, err := mt.SubtreeConsistencyProof(1, 3, 8); err == nil {
		t.Error("SubtreeConsistencyProof(1, 3, 8) succeeded unexpectedly (invalid alignment)")
	}

Comment thread testonly/reference_test.go Outdated
switch {
// The subtree is on the left of split. Prove that the subtree is consistent
// with the subtree on the left of split, and record the root of the right
//subtree.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add a space after the comment prefix // to adhere to standard Go formatting guidelines.

Suggested change
//subtree.
// subtree.

@phbnf
phbnf force-pushed the conf-reftests branch 2 times, most recently from 1f0d59a to 795a4d4 Compare June 23, 2026 11:17
phbnf added 2 commits June 23, 2026 17:40
tests: better comments, remove prints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant