node: publish Core's sequence ZMQ topic for block connects and disconnects - #52
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:
|
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe node adds a Core-compatible ZMQ ChangesZMQ pubsequence
Sequence Diagram(s)sequenceDiagram
participant BlockTransition
participant ZmqPublisher
participant SocketZmqPublisher
participant ZMQSubscriber
BlockTransition->>ZmqPublisher: publish_sequence(Connected or Disconnected)
ZmqPublisher->>SocketZmqPublisher: Encode block hash, label, and sequence
SocketZmqPublisher->>ZMQSubscriber: Publish pubsequence notification
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/node/src/zmq_publisher.rs (1)
322-340: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAllocate sequence numbers per endpoint and advance them only after a successful send.
fetch_addconsumes the value beforesend_multipartsucceeds. Bitcoin Core increments its notifier counter only after a successful multipart send. One shared topic counter also creates gaps when one endpoint fails and another succeeds. Concurrent calls can deliver allocated values out of order. Serialize allocation with each endpoint’s publication and increment only on success.🤖 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 `@crates/node/src/zmq_publisher.rs` around lines 322 - 340, Update publish so sequence allocation is maintained separately for each endpoint rather than using the shared counters indexed by topic. Serialize each endpoint’s sequence allocation and send through its publication path, incrementing that endpoint’s counter only after send_multipart succeeds; failed sends must not consume values, and concurrent publishes must preserve per-endpoint ordering.Source: MCP tools
🤖 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 `@crates/node/src/apply.rs`:
- Around line 2328-2330: Move the Connected publish call in the connection flow
to after handles.applied_tip.store(...) completes, ensuring subscribers observe
the new tip when handling the event. Add a publisher test that reads applied_tip
during publish_sequence and verifies it contains the updated tip.
In `@crates/node/src/config.rs`:
- Around line 405-410: Update the sequence allocation flow in
push_zmq_publications and SocketZmqPublisher so the shared sequence counter
advances only after send_multipart succeeds; failed sends must not consume a
sequence number or create gaps. Preserve reuse of one sequence value across all
sequence endpoints and Connected/Disconnected events. Add a failure-path test
verifying the next successfully delivered event receives the undelivered event’s
sequence number.
In `@docs/solutions/architecture-patterns/node-reorg-execution-design.md`:
- Line 158: Update the “Open” table row currently labeled “Disconnect
notification” to describe only the missing mempool A/R notifications, removing
the completed ZMQ pubsequence connect/disconnect behavior from its text.
---
Outside diff comments:
In `@crates/node/src/zmq_publisher.rs`:
- Around line 322-340: Update publish so sequence allocation is maintained
separately for each endpoint rather than using the shared counters indexed by
topic. Serialize each endpoint’s sequence allocation and send through its
publication path, incrementing that endpoint’s counter only after send_multipart
succeeds; failed sends must not consume values, and concurrent publishes must
preserve per-endpoint ordering.
🪄 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: fe4f947d-05c2-46fe-8a83-fdd4916463bc
📒 Files selected for processing (13)
CONCEPTS.mdREADME.mdcrates/node/src/apply.rscrates/node/src/bitcoin_conf_compat.rscrates/node/src/config.rscrates/node/src/lib.rscrates/node/src/zmq_publisher.rscrates/node/tests/bitcoin_conf_compat.rscrates/node/tests/config_layered.rscrates/node/tests/rpc_wiring.rsdocs/README.mddocs/getting-started.mddocs/solutions/architecture-patterns/node-reorg-execution-design.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: bench-smoke
- GitHub Check: test
🧰 Additional context used
🔍 Remote MCP Github Grep
Additional review context
- Bitcoin Core exposes both
-zmqpubsequenceand-zmqpubsequencehwm; the topic is registered as a ZMQ notifier. - Core’s wire format is three frames: topic, body, and a per-topic 4-byte little-endian message counter. Hashes use reversed byte order.
- Core emits sequence
CandDevents through callbacks covering every block connection and disconnection—not only active-tip changes. - Core’s block payload is the 32-byte hash followed by the event label; transaction
A/Rpayloads additionally include an 8-byte little-endian mempool sequence. Thus this PR provides only partial Corepubsequencecompatibility while omitting transaction events. - Core’s publisher increments its message counter only after a successful send, and the counter is stored on the publisher notifier. Verify the PR’s shared-counter and failure-handling behavior against this detail.
🔇 Additional comments (6)
crates/node/src/config.rs (1)
154-157: LGTM!Also applies to: 218-219, 268-269, 368-368, 445-445, 532-534, 547-549, 649-650, 659-660, 737-739, 752-754
crates/node/src/bitcoin_conf_compat.rs (1)
71-76: LGTM!Also applies to: 201-203, 216-218
crates/node/tests/bitcoin_conf_compat.rs (1)
74-75: LGTM!Also applies to: 92-96
crates/node/tests/config_layered.rs (1)
123-124: LGTM!Also applies to: 154-157, 175-189
docs/getting-started.md (1)
8-11: LGTM!docs/solutions/architecture-patterns/node-reorg-execution-design.md (1)
56-59: LGTM!
| push_zmq_publications( | ||
| &mut publications, | ||
| crate::zmq_publisher::ZmqTopic::Sequence, | ||
| &self.zmqpubsequence, | ||
| self.zmqpubsequencehwm, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 10 \
'ZmqPublication|ZmqTopic::Sequence|Sequence|next_sequence|counter|send(_multipart)?|publish' \
crates/node/src/zmq_publisher.rs \
crates/node/src/lib.rsRepository: gosuda/bitcoin-rs
Length of output: 41396
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- publish implementation and sequence call sites ---'
sed -n '221,390p' crates/node/src/zmq_publisher.rs
rg -n -C 8 'publish_sequence|SequenceEvent::(Connected|Disconnected)' crates/node/src
printf '%s\n' '--- static order check ---'
python3 - <<'PY'
from pathlib import Path
text = Path("crates/node/src/zmq_publisher.rs").read_text()
increment = text.index("self.counters[topic.index()].fetch_add")
send = text.index("socket.send_multipart")
assert increment < send, "counter increment is not before send"
print("counter increment precedes socket.send_multipart")
print("counter scope: SocketZmqPublisher.counters[topic.index()]")
print("sequence endpoints receive the same sequence value from one publish() call")
PYRepository: gosuda/bitcoin-rs
Length of output: 20614
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -i -C 5 \
'sequence topic|sequence number|sequence counter|pubsequence|sequence_payload|SequenceEvent|failed.*send|send.*fail|notification.*sequence' \
README.md docs crates .github 2>/dev/null || true
printf '%s\n' '--- publisher state and test definitions ---'
rg -n -C 8 \
'struct .*Publisher|next_sequence|AtomicU32|counters|events:|publish_sequence' \
crates/node/src/apply.rs crates/node/src/zmq_publisher.rsRepository: gosuda/bitcoin-rs
Length of output: 50373
Do not consume sequence numbers on failed sends.
SocketZmqPublisher correctly shares one counter for Connected and Disconnected events and reuses one value across all sequence endpoints. However, fetch_add runs before send_multipart, so a failed send advances the counter. Change the allocation logic and add a failure test so an undelivered event does not create a sequence gap.
🤖 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 `@crates/node/src/config.rs` around lines 405 - 410, Update the sequence
allocation flow in push_zmq_publications and SocketZmqPublisher so the shared
sequence counter advances only after send_multipart succeeds; failed sends must
not consume a sequence number or create gaps. Preserve reuse of one sequence
value across all sequence endpoints and Connected/Disconnected events. Add a
failure-path test verifying the next successfully delivered event receives the
undelivered event’s sequence number.
Source: MCP tools
There was a problem hiding this comment.
Keeping the current design here, deliberately, on both halves of this.
Increment on a dropped send. The counter's purpose in this protocol is loss detection: our sockets send with DONTWAIT, so a send that fails at HWM means the message is gone, and the subscriber never sees it either way. Consuming the number is what makes that loss visible as a gap — which for the enforcer is a loud fatal error instead of a silently missing block connect. Holding the number back would make a dropped C indistinguishable from no event at all, which is the worse failure for a validator driven off this stream. That is also what a subscriber observes from Core when its ZMQ layer drops at HWM.
Per-endpoint counters. All four existing topics share one counter per topic across that topic's endpoints, and one publish() sends the same number to all of them, so each subscriber sees a contiguous stream in the ordinary single-endpoint deployment. Splitting the counter per endpoint is a rework of SocketZmqPublisher's counter model affecting every topic — out of scope for adding one topic, and I would rather not change the semantics of hashblock/rawblock as a side effect of this PR. Noted as a real divergence if someone binds one topic to several endpoints with differing failure behavior.
Both properties were exercised in the live run: contiguous per-topic counters across 200 blocks and the reorg, and independent counters across all five topics on one node.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
![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: 3e646cee35
ℹ️ 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".
|
|
||
| pub(crate) fn sequence_payload(event: SequenceEvent) -> [u8; 33] { | ||
| let mut body = [0_u8; 33]; | ||
| body[..32].copy_from_slice(&hash_body_from_hash(event.hash())); |
There was a problem hiding this comment.
When a Core-compatible subscriber decodes any C or D event, this reuses hash_body_from_hash, which reverses Hash256's consensus little-endian bytes into display order. Core's sequence payload carries the raw 32-byte hash in little-endian order, so standard subscribers that reverse the payload for display will derive the wrong block hash and fail to resolve the event; copy event.hash().to_le_bytes() directly before appending the label.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one — it would break the parity it is trying to protect, and I have wire evidence.
I ran bitcoin-rs alongside Bitcoin Core 27.0 on regtest with both nodes publishing sequence, and diffed the captured frames: for 200 blocks plus a reorg, frame[1][0..32] from our publisher was byte-identical to Core's own payload for the same block (core sequence payload list identical to rs: True in the runtime comment), and equal to getblockhash's display form. Core's ZMQ layer serializes hashes reversed relative to their internal order, so display order is the wire order here — same as our existing hashblock topic, which is why the helper is shared.
The consumer confirms it independently: cusf-enforcer-mempool's parser does hash.reverse() on those 32 bytes before BlockHash::from_byte_array, i.e. it expects display order on the wire. Sending to_le_bytes() directly would leave every hash reversed for both Core-compatible subscribers and the enforcer.
| handles | ||
| .zmq_publisher | ||
| .publish_sequence(crate::zmq_publisher::SequenceEvent::Connected(tip.hash)); |
There was a problem hiding this comment.
When a subscriber reacts immediately to a C event and queries getblockcount, getbestblockhash, or getblockchaininfo, the event is sent before applied_tip is updated at line 2345, so RPC can still report the preceding block. The window can include all subsequent raw-block and per-transaction notifications, and it is inconsistent with disconnect events, which are emitted after their tip store; publish the connect event only after the new applied tip becomes visible.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the same point CodeRabbit raised — fixing it. The window you describe is the one that matters in practice: the enforcer's C handler immediately calls back into the node, so it can act on a tip the RPC surface has not published yet. Publishing after applied_tip is stored also makes C symmetric with D, which already waits for its rollback.
Runtime verification of the
|
|
@devin Resolve all the feedbacks if relevent. |
|
Went through all four:
Pushing the two fixes shortly. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17ea5c2eeb
ℹ️ 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".
| if handles.zmq_publisher.wants_notifications() { | ||
| handles | ||
| .zmq_publisher | ||
| .publish_sequence(crate::zmq_publisher::SequenceEvent::Disconnected( |
There was a problem hiding this comment.
When complete_disconnect returns an error (for example, a backend write or flush failure), applied_tip has already moved to the parent and the error is explicitly classified as MarkerStuck, meaning the rollback completed, but this later publication block is skipped by ?. A pubsequence subscriber therefore remains on the disconnected block even though RPC exposes the parent tip; publish D immediately after the tip store, before this fallible marker cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and it's the mirror image of the connect bug you caught earlier — fixing it the same way.
complete_disconnect runs after applied_tip has already been stored to the parent, which is precisely why its failure is classified MarkerStuck rather than a rollback failure: the rollback happened. Propagating with ? before the emission means a subscriber is left believing it is still on a block the node has dropped, while RPC reports the parent — the enforcer would keep building on a tip that no longer exists.
Moving the D emission to immediately after the applied-tip store, before the fallible marker cleanup, so both labels are published at the point the mutation becomes visible. The reorg ordering test still guards that every D precedes the replacement branch's Cs.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Adds Bitcoin Core's
sequenceZMQ topic, carrying block events only (Con connect,Don disconnect). This is the last piece a remotebip300301_enforcerneeds to drive its validator off bitcoin-rs: it subscribes to exactly one topic (sequence,lib/cli.rs), and when--node-zmq-addr-sequenceis omitted it discovers the endpoint by looking forpubsequenceingetzmqnotificationsand hard-errors if it is absent. Complements #49 (REST) — no BIP300/301 logic is ported, and none of Core's other notification surface is added.Frame layout is the three-frame shape its parser requires (
cusf-enforcer-mempool/lib/zmq.rs), and the topic gets its own counter, which is what makes the counter unified across event kinds — one stream, one monotonicu32:The publisher API takes a
SequenceEventenum rather than a method per label, so adding the mempool variants later doesn't reshapeZmqPublisher. The four existing topics and their independent counters are untouched.Why
A/Rare deliberately absentCore also multiplexes mempool add/remove onto this topic. Emitting them now would be worse than silence: the consumer treats a mempool-sequence gap as fatal, but never checks tx-sequence continuity if no tx events arrive — so C/D-only leaves the validator path fully working and degrades the enforcer's mempool mode to an empty mempool instead of a hard error. Our mempool cannot currently produce a gapless per-transaction sequence:
remove_entriesbumps once per call rather than per transaction,prioritisebumps with no membership change,insert_entrydiscards the IDs of the evictions it triggers, and reorg reconsideration is still unfinished (docs/solutions/architecture-patterns/node-reorg-execution-design.md), so no legitimateAcan follow aDyet. That needs a per-transaction event-sequence and a removal-reason model incrates/mempool— a separate change, documented rather than faked.Emission points
Both labels are published where the chainstate mutation lands, which is what makes the stream safe to react to:
Two ordering bugs are fixed by that placement, both of which a subscriber can actually observe:
Cbeforeapplied_tip.storelet a subscriber act on a connect while RPC still reported the parent as the tip — and the enforcer'sChandler calls straight back into the node.Cs before the old branch'sDs, so the enforcer would have calledconnect_blockfor a block whose parent was not its tip. Emitting at the success path ofdisconnect_block_admittedmakes publication order equal occurrence order for every caller and removes the deferred list.Both are pinned by tests: an executor-driven reorg asserts the exact
D…C…sequence with a contiguous counter, and a publisher that readsapplied_tipduring emission asserts the announced block is already the visible tip.Configured like the existing topics (
--zmqpubsequence,BITCOIN_RS_ZMQPUBSEQUENCE,zmqpubsequenceinbitcoin.conf, HWM sibling, layered config), andgetzmqnotificationsreportspubsequenceso endpoint auto-discovery works.Docs that claimed disconnects are silent (
README.md,docs/getting-started.md,docs/README.md, the reorg-design table) are reconciled, andCONCEPTS.mdgains the sequence-stream term.Verified live against Bitcoin Core 27.0 on regtest — frames diffed against Core's own
sequencestream over 200 blocks and a reorg (evidence).Link to Devin session: https://app.devin.ai/sessions/2cf54a23e1494fd080fa7541dadf06ff
Requested by: @metaphorics