docs: the REST guide still tells operators pubsequence doesn't exist - #53
Conversation
Original prompt from a
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe REST interface documentation now describes Core-compatible Changespubsequence documentation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce6d706aec
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bitcoin-rs publishes the Core-compatible `pubsequence` ZMQ topic with block | ||
| connect (`C`) and disconnect (`D`) events. The configured endpoint is reported | ||
| by `getzmqnotifications`, so the unmodified enforcer can discover it through |
There was a problem hiding this comment.
When an operator follows this guide with only rest=1, the node's default zmqpubsequence list is empty (crates/node/src/config.rs), so NodeState installs NoOpZmqPublisher and getzmqnotifications reports no sequence endpoint. Auto-discovery therefore works only after bitcoin-rs is also configured with something like zmqpubsequence=tcp://127.0.0.1:28332; document that prerequisite here, otherwise the setup presented by this guide still leaves the enforcer without a publisher.
Useful? React with 👍 / 👎.
| by `getzmqnotifications`, so the unmodified enforcer can discover it through | ||
| its normal startup path rather than requiring an external publisher or an | ||
| explicit `--node-zmq-addr-sequence`. Mempool `A`/`R` events remain intentionally | ||
| absent until the mempool has per-transaction event sequencing and explicit |
There was a problem hiding this comment.
Even with a configured endpoint, this block-only stream cannot replace the previous no-mempool/bounded guidance when the enforcer tracks bitcoin-rs's mempool. CONCEPTS.md records that the enforcer requires a contiguous transaction-event sequence, but transactions can enter the node while this publisher emits no A/R events, so discovery of the socket does not keep the enforcer's mempool view synchronized. Retain the operational restriction or explicitly scope the no-external-publisher claim to block-only/no-mempool operation.
AGENTS.md reference: AGENTS.md:L3-L7
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rest-interface.md`:
- Around line 41-43: Update the documentation text to distinguish the configured
notification type `pubsequence` from the actual ZMQ wire topic, referring to
`sequence` as the topic clients must subscribe to while retaining `pubsequence`
for configuration and `getzmqnotifications` discovery.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be9c78ea-9e5e-4ae2-bccd-3f98525989e5
📒 Files selected for processing (1)
docs/rest-interface.md
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test
- GitHub Check: bench-smoke
- GitHub Check: wallet-no-seckey
- GitHub Check: fmt
🧰 Additional context used
🔍 Remote MCP Github Grep
Relevant review context
- Bitcoin Core configures this through
-zmqpubsequence=<address>and registers a correspondingpubsequencenotifier. - The actual ZMQ command emitted on the wire is named
sequence, notpubsequence; the latter is the configuration/notifier name. Documentation should distinguish these terms. getzmqnotificationsis an RPC that returns information about active ZMQ notifications, supporting the discoverability claim.- Core’s sequence payload includes the reversed 32-byte hash, an event label, and optionally a little-endian sequence number.
| bitcoin-rs publishes the Core-compatible `pubsequence` ZMQ topic with block | ||
| connect (`C`) and disconnect (`D`) events. The configured endpoint is reported | ||
| by `getzmqnotifications`, so the unmodified enforcer can discover it through |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use sequence for the wire topic.
This wording confuses the notifier name with the ZMQ topic. pubsequence is the configuration and getzmqnotifications type; the wire topic is sequence. A client that subscribes to pubsequence will receive no messages.
Proposed wording
-bitcoin-rs publishes the Core-compatible `pubsequence` ZMQ topic with block
+bitcoin-rs publishes the Core-compatible `sequence` ZMQ topic for the
+`pubsequence` notifier, with block
connect (`C`) and disconnect (`D`) events.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bitcoin-rs publishes the Core-compatible `pubsequence` ZMQ topic with block | |
| connect (`C`) and disconnect (`D`) events. The configured endpoint is reported | |
| by `getzmqnotifications`, so the unmodified enforcer can discover it through | |
| bitcoin-rs publishes the Core-compatible `sequence` ZMQ topic for the | |
| `pubsequence` notifier, with block connect (`C`) and disconnect (`D`) events. The configured endpoint is reported | |
| by `getzmqnotifications`, so the unmodified enforcer can discover it through |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/rest-interface.md` around lines 41 - 43, Update the documentation text
to distinguish the configured notification type `pubsequence` from the actual
ZMQ wire topic, referring to `sequence` as the topic clients must subscribe to
while retaining `pubsequence` for configuration and `getzmqnotifications`
discovery.
Source: MCP tools
1 participant
Summary
docs/rest-interface.mdlanded in #49 whilepubsequencewas still missing, so it tells operators to point the enforcer at an external ZMQ publisher via--node-zmq-addr-sequenceor to run it in a no-mempool mode. #52 shipped the topic, which makes that instruction actively wrong — it sends someone setting up an enforcer down a workaround for a problemmainno longer has, when auto-discovery throughgetzmqnotificationsnow works out of the box.Rewritten to describe what we ship: block
C/Donpubsequence, discoverable viagetzmqnotifications, with mempoolA/Rstill intentionally absent — same framing asREADME.md,docs/getting-started.mdand the reorg-design note, so the docs stop disagreeing with each other. Thegetnetworkinfoversion caveat directly above it is untouched, because that one is still true.Docs only, no code change.
Link to Devin session: https://app.devin.ai/sessions/2cf54a23e1494fd080fa7541dadf06ff
Requested by: @metaphorics