Skip to content

docs: fix minor issues in protobuf documentation#119

Merged
torrejonv merged 1 commit into
bsv-blockchain:mainfrom
torrejonv:docs/protobuf-documentation-fixes
Nov 5, 2025
Merged

docs: fix minor issues in protobuf documentation#119
torrejonv merged 1 commit into
bsv-blockchain:mainfrom
torrejonv:docs/protobuf-documentation-fixes

Conversation

@torrejonv

Copy link
Copy Markdown
Contributor

Fixed documentation issues identified in protobuf documentation audit:

  • alertProto.md: Removed test comment that was accidentally left in documentation
  • blockchainProto.md: Removed duplicate TOC entry for GetBlockHeadersFromCommonAncestorRequest

Note: The audit also identified 3 undocumented API services that need documentation:

  • services/legacy/peer_api/peer_api.proto (PeerService with 7 RPC methods)
  • services/p2p/p2p_api/p2p_api.proto (PeerService with 9 RPC methods)
  • services/asset/asset_api/asset_api.proto (appears incomplete, needs investigation)

Fixed documentation issues identified in protobuf documentation audit:
- alertProto.md: Removed test comment that was accidentally left in documentation
- blockchainProto.md: Removed duplicate TOC entry for GetBlockHeadersFromCommonAncestorRequest

Note: The audit also identified 3 undocumented API services that need documentation:
- services/legacy/peer_api/peer_api.proto (PeerService with 7 RPC methods)
- services/p2p/p2p_api/p2p_api.proto (PeerService with 9 RPC methods)
- services/asset/asset_api/asset_api.proto (appears incomplete, needs investigation)
@torrejonv torrejonv enabled auto-merge (squash) November 5, 2025 17:02
@github-actions

github-actions Bot commented Nov 5, 2025

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete

No issues found.

This PR correctly removes a test comment and duplicate TOC entry from protobuf documentation. The changes are straightforward cleanup with no impact on functionality.

@torrejonv torrejonv merged commit ee814e9 into bsv-blockchain:main Nov 5, 2025
1 check passed
@torrejonv torrejonv deleted the docs/protobuf-documentation-fixes branch November 15, 2025 03:13
icellan pushed a commit that referenced this pull request May 13, 2026
Round-trip changes from the review comments left after the initial
atomic-publication PR:

- CodeQL #119: route the post-Del hash-prefix directory removal
  through os.Root so every path operation in this file stays inside
  the store sandbox.
- renameTempFile: cross-platform semantics tightened. The
  ErrBlobAlreadyExists branch is documented as non-POSIX only
  (rename(2) atomically replaces on POSIX), and an allowOverwrite
  parameter lets non-POSIX callers explicitly remove the destination
  and retry instead of receiving a spurious already-exists error
  when they opted in to overwriting.
- syncParentDirBestEffort: doc clarifies that the directory fsync
  is correctness-critical on ext4/xfs/btrfs after rename, not an
  optional "nice to have". "Best-effort" applies to failure handling,
  not to whether the call should run.
- createTempSibling: replace big.NewInt(1<<63-1) with
  big.NewInt(math.MaxInt64) for readability.
- fsyncMode opt-out via the ?fsyncMode=full|data|none URL parameter.
  Default remains full (current behaviour, safe on local disks).
  Operators running the file store over NFS or other network-attached
  storage can drop to data (skip the directory fsync) or none (skip
  both) to avoid the ~10-20 ms per-write cost; the trade-off is
  weakened crash safety as documented at the fsyncMode declaration.

Tests added:

- TestSetFromReader_ConcurrentWritersNeverExposePartialFinal: eight
  concurrent SetFromReader calls on the same key with AllowOverwrite.
  A reader poller asserts that any successful read of the final
  filename is byte-identical to the expected header+body payload,
  never a partial prefix.
- TestRenameTempFile_OverwriteAndRejectSemantics: documents the
  observable POSIX behaviour (rename replaces on AllowOverwrite=true,
  errorOnOverwrite stops the call earlier when false) so a regression
  on either branch shows up in CI.
- TestParseFsyncMode / TestNewWithFsyncMode: cover the new URL
  parameter (parsing, end-to-end Set/Get on each mode, invalid value
  rejection).
- BenchmarkSetFromReader_FsyncModes: measures the cost of the
  atomic-publication path across three payload sizes and the three
  fsync modes. Local-disk numbers on an Apple M3 Max show
  fsyncModeFull at ~20 ms/op, fsyncModeData at ~10 ms/op, fsyncModeNone
  at <1 ms/op — gives operators a concrete frame for evaluating the
  trade-off on their target filesystem.
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.

2 participants