Skip to content

Pruning e2e verification - #404

Open
alrxy wants to merge 16 commits into
mainfrom
prune-verification
Open

Pruning e2e verification#404
alrxy wants to merge 16 commits into
mainfrom
prune-verification

Conversation

@alrxy

@alrxy alrxy commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

PR Type

Tests, Enhancement


Description

  • Add pruning end-to-end verification test

  • Scan bbolt and badger storage

  • Parameterize retention, sync, pruner settings

  • Add CI job and Make target


Diagram Walkthrough

flowchart LR
  A["Pruning E2E test"] -- "creates epoch-bound request" --> B["Wait for signatures and proof"]
  B -- "waits until prune eligibility" --> C["Scan persisted sidecar data"]
  C -- "supports both backends" --> D["bbolt and badger storage"]
  E["Scripts and CI configuration"] -- "inject retention and pruner env" --> A
Loading

File Walkthrough

Relevant files
Tests
3 files
pruning_test.go
Add pruning end-to-end storage verification                           
+419/-0 
tests.yaml
Add pruning E2E CI matrix job                                                       
+89/-0   
Makefile
Add dedicated pruning E2E target                                                 
+4/-0     
Enhancement
1 files
types_test.go
Extract reusable environment loading helper                           
+11/-3   
Configuration changes
3 files
generate_network.sh
Pass pruning settings into generated network                         
+16/-1   
sidecar-start.sh
Make sidecar pruning values environment-driven                     
+10/-5   
setup.sh
Export pruning-related environment variables                         
+6/-1     

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incomplete coverage

The new pruning check only scans sidecarIndex 0, even though the test creates the request across all sidecars and waits for signatures from all of them. If pruning is broken on any non-zero sidecar, this test will still pass and miss the regression.

	before, err = scanSidecarStorage(0, targetEpoch, common.HexToHash(requestID), nil)
	require.NoError(t, err)
}

t.Log("Step 5: Waiting for pruning and verifying that no non-excluded entities remain...")
require.NoError(t, waitForErrorIsNil(ctx, pruningTimeout(), func() error {
	remaining, err := scanSidecarStorage(0, targetEpoch, common.HexToHash(requestID), excluded)

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Verify every sidecar storage

The test only inspects sidecarIndex 0, so it can pass even when pruning is broken on
the other sidecars. Iterate over every configured sidecar during verification;
otherwise this e2e check misses cluster-wide pruning regressions.

e2e/tests/pruning_test.go [87-103]

+requestHash := common.HexToHash(requestID)
+
 require.NoError(t, waitForErrorIsNil(ctx, pruningTimeout(), func() error {
-		remaining, err := scanSidecarStorage(0, targetEpoch, common.HexToHash(requestID), excluded)
-		if err != nil {
-			return err
+		var allRemaining []remainingEntity
+
+		for i := range envInfo.GetSidecarConfigs() {
+			remaining, err := scanSidecarStorage(i, targetEpoch, requestHash, excluded)
+			if err != nil {
+				return err
+			}
+			for _, entity := range remaining {
+				entity.Location = fmt.Sprintf("sidecar-%02d/%s", i+1, entity.Location)
+				allRemaining = append(allRemaining, entity)
+			}
+			if debugScan && i == 0 {
+				after, scanErr := scanSidecarStorage(i, targetEpoch, requestHash, nil)
+				if scanErr != nil {
+					return scanErr
+				}
+				finalAfter = after
+			}
 		}
-		if debugScan {
-			after, scanErr := scanSidecarStorage(0, targetEpoch, common.HexToHash(requestID), nil)
-			if scanErr != nil {
-				return scanErr
-			}
-			finalAfter = after
-		}
-		if len(remaining) > 0 {
-			return fmt.Errorf("found unpruned entities:\n%s", formatRemainingEntities(remaining))
+
+		if len(allRemaining) > 0 {
+			return fmt.Errorf("found unpruned entities:\n%s", formatRemainingEntities(allRemaining))
 		}
 		return nil
 	}))
Suggestion importance[1-10]: 8

__

Why: This is a correct and important gap: TestPruningE2E_RemovesAllNonExcludedEntities currently verifies pruning only on sidecar 0, so failures on other sidecars would go undetected. The suggested loop is consistent with how the request is created across all sidecars and materially improves the test's coverage.

Medium
Match both hex ID formats

requestID matching only checks raw bytes and the "0x"-prefixed hex form, so any
entries stored as plain hex will be missed and the test can report a false pass.
Match both prefixed and non-prefixed hex encodings to avoid silently skipping
unpruned records.

e2e/tests/pruning_test.go [336-347]

 func matchesPrunedEntity(key, value, epochBytes, requestIDBytes []byte, requestIDHex string) bool {
+	requestIDHexNoPrefix := strings.TrimPrefix(requestIDHex, "0x")
+
 	if bytes.Contains(key, requestIDBytes) || bytes.Contains(value, requestIDBytes) {
 		return true
 	}
 	if bytes.Contains(key, []byte(requestIDHex)) || bytes.Contains(value, []byte(requestIDHex)) {
+		return true
+	}
+	if bytes.Contains(key, []byte(requestIDHexNoPrefix)) || bytes.Contains(value, []byte(requestIDHexNoPrefix)) {
 		return true
 	}
 	if bytes.Contains(key, epochBytes) || bytes.Contains(value, epochBytes) {
 		return true
 	}
 	return false
 }
Suggestion importance[1-10]: 5

__

Why: This is a plausible correctness improvement because matchesPrunedEntity currently checks raw bytes and only the 0x-prefixed hex form from requestID.Hex(). If some stored keys or values use hex without the prefix, the scan could miss leftover records and falsely pass.

Low
General
Wait for healthy services

A fixed sleep 30 makes the pruning job flaky because slower runners can still be
starting containers when the tests begin. Wait for the compose services to become
healthy instead of assuming startup always fits in 30 seconds.

.github/workflows/tests.yaml [422-427]

 - name: "Start Pruning Test Network"
   run: |
     cd e2e/temp-network
     docker compose up -d
-    sleep 30
 
+    timeout 180 bash -c '
+      until ! docker compose ps | grep -Eq "starting|unhealthy|created"; do
+        sleep 5
+      done
+    '
+
Suggestion importance[1-10]: 6

__

Why: Replacing a fixed sleep 30 with an actual readiness wait is a relevant improvement for CI stability, especially since container startup time varies across runners. The proposed change aligns with the defined healthchecks and reduces flaky failures before make e2e-pruning-test runs.

Low

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6df10e3415

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread e2e/tests/pruning_test.go
Message: []byte(msg),
RequiredEpoch: (*uint64)(&epoch),
})
require.NoErrorf(t, err, "failed to sign message on sidecar %d", i)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Retry SignMessage when creating pruning test requests

createPruningRequest performs a single SignMessage call per sidecar and immediately fails on any transient RPC/storage error. This makes the new pruning test flaky under the same transaction-conflict conditions already handled elsewhere in this suite (for example, sign_test.go retries this API). A bounded retry loop here would prevent intermittent CI failures that are unrelated to pruning behavior.

Useful? React with 👍 / 👎.

Comment thread e2e/scripts/sidecar-start.sh Outdated
RETENTION_SIGNATURE_EPOCHS=${RETENTION_SIGNATURE_EPOCHS:-1000}
RETENTION_PROOF_EPOCHS=${RETENTION_PROOF_EPOCHS:-1000}
PRUNER_INTERVAL=${PRUNER_INTERVAL:-1m}
SYNC_EPOCHS=${SYNC_EPOCHS:-1000}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Derive sync epochs from retention defaults

SYNC_EPOCHS defaults to 1000 even when retention is overridden to a much smaller value. With this change, users can now set RETENTION_VALSET_EPOCHS via env, but if they forget to also set SYNC_EPOCHS, the generated config becomes invalid (sync.epochs > retention.valset-epochs) and relay startup fails due to config validation. The default should be capped/derived from retention to avoid this regression when enabling pruning settings.

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Mar 25, 2026

Copy link
Copy Markdown

🧪 Test Coverage Report

Coverage: 50.7%

@alrxy
alrxy requested a review from oxsteins April 2, 2026 07:00
# Conflicts:
#	.github/workflows/tests.yaml
#	e2e/scripts/generate_network.sh
@alrxy
alrxy requested a review from ilyasymbiotic May 15, 2026 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant