Pull known logs from witness network #52
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the witness implementation to support dynamic log discovery by pulling logs from a network log list instead of requiring static configuration.
- Key changes include storing witness configuration (log origins and verifier keys) in the backend, implementing a
PullLogListmethod that periodically fetches and parses log lists to add new logs dynamically, and improving metrics collection - The database schema is updated to use
BLOB NOT NULLinstead ofTEXTfor checkpoint storage with proper NULL handling - The witness configuration now takes a log list URL instead of a static list of known logs
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/witness/witness.go | Major refactoring to support dynamic log list loading from a URL, store witness config in backend, defer checkpoint fetching, add RWMutex for thread-safe log access, implement parseLogList parser, and improve metrics |
| internal/witness/metrics.go | Add new metrics for known logs count, log size per origin, and add-checkpoint request counts with error/origin/progress labels |
| internal/ctlog/sqlite.go | Update schema to BLOB NOT NULL, add NULL-to-empty-slice conversion for consistency in Create and Replace methods |
| internal/ctlog/cache.go | Update cache schema to use NOT NULL constraints and STRICT mode |
| cmd/sunlight/sunlight.go | Replace static KnownLogs config with LogList URL, add periodic log list pulling with SIGHUP reload support, refactor home page data structure |
| cmd/sunlight/home.html | Move witness information section and add log list source display |
| README.md | Update database creation command to reflect new BLOB NOT NULL STRICT schema |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+250
to
+253
| } | ||
| w.verifiers = verifiers | ||
| w.checkpoints = checkpoints | ||
| w.m.KnownLogs.Set(float64(len(w.verifiers))) |
There was a problem hiding this comment.
The logsMu lock is held while performing multiple blocking I/O operations (Backend.Create, Backend.Fetch at lines 213-221, and Backend.Fetch/Replace at lines 227-249). This can block concurrent read operations (verifiersForOrigin, checkpointForOrigin, Logs) for an extended period. Consider releasing the lock before the backend operations and re-acquiring it only for the final update of w.verifiers and w.checkpoints at lines 251-253.
Suggested change
| } | |
| w.verifiers = verifiers | |
| w.checkpoints = checkpoints | |
| w.m.KnownLogs.Set(float64(len(w.verifiers))) | |
| } | |
| w.logsMu.Lock() | |
| w.verifiers = verifiers | |
| w.checkpoints = checkpoints | |
| w.m.KnownLogs.Set(float64(len(w.verifiers))) | |
| w.logsMu.Unlock() |
FiloSottile
force-pushed
the
push-qzzyxxprnvpp
branch
from
c6eb6cb to
dc74159
Compare
When passing nil to Create or Replace, it was stored as NULL, but then
was fetched as []byte("") by Fetch, which then didn't compare equal to
NULL in Replace, making it always fail.
This was made harder to debug by two things:
1. in the CLI, NULL and empty are both shown as empty by default;
2. I tried to fix it by running
"UPDATE checkpoints SET body = '' WHERE body IS NULL;"
which put empty TEXT in those columns, which does not compare equal
to empty BLOB. They were both allowed in the same column by the
non-STRICT table.
FiloSottile
force-pushed
the
push-qzzyxxprnvpp
branch
from
dc74159 to
8f00ab4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.