-
Notifications
You must be signed in to change notification settings - Fork 0
node: publish Core's sequence ZMQ topic for block connects and disconnects
#52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1047,7 +1047,9 @@ pub fn disconnect_block( | |
| let transition = handles | ||
| .begin_chain_transition() | ||
| .map_err(|error| crate::DisconnectError::Refused(Box::new(error)))?; | ||
| disconnect_block_admitted(handles, block, &transition) | ||
| let result = disconnect_block_admitted(handles, block, &transition); | ||
| drop(transition); | ||
| result | ||
| } | ||
|
|
||
| /// Disconnects one block while the caller holds admission and `chain_transition`. | ||
|
|
@@ -1217,6 +1219,14 @@ pub(crate) fn disconnect_block_admitted( | |
| }) | ||
| })?; | ||
|
|
||
| if handles.zmq_publisher.wants_notifications() { | ||
| handles | ||
| .zmq_publisher | ||
| .publish_sequence(crate::zmq_publisher::SequenceEvent::Disconnected( | ||
| block_hash, | ||
| )); | ||
| } | ||
|
|
||
| // The marker deliberately stays set here. | ||
| // | ||
| // Every mutation landed, but only some of them are durable. The index | ||
|
|
@@ -2315,6 +2325,9 @@ fn apply_block_admitted( | |
| // Best-effort ZMQ event emission. Failures must not propagate per the | ||
| // ZmqPublisher contract; the trait's methods return `()`. | ||
| handles.zmq_publisher.publish_hashblock(tip.hash); | ||
| handles | ||
| .zmq_publisher | ||
| .publish_sequence(crate::zmq_publisher::SequenceEvent::Connected(tip.hash)); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a subscriber reacts immediately to a Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, and the same point CodeRabbit raised — fixing it. The window you describe is the one that matters in practice: the enforcer's |
||
| if wants_rawblock { | ||
| handles.zmq_publisher.publish_rawblock(&block_bytes); | ||
| } | ||
|
|
@@ -8678,6 +8691,128 @@ mod consensus_rule_tests { | |
| Ok(()) | ||
| } | ||
|
|
||
| #[derive(Debug, Default)] | ||
| struct RecordingSequencePublisher { | ||
| events: Mutex<Vec<(Hash256, u8, u32)>>, | ||
| next_sequence: Mutex<u32>, | ||
| } | ||
|
|
||
| impl crate::ZmqPublisher for RecordingSequencePublisher { | ||
| fn publish_hashblock(&self, _hash: Hash256) {} | ||
|
|
||
| fn publish_hashtx(&self, _txid: bitcoin::Txid) {} | ||
|
|
||
| fn publish_rawblock(&self, _bytes: &[u8]) {} | ||
|
|
||
| fn publish_rawtx(&self, _bytes: &[u8]) {} | ||
|
|
||
| fn publish_sequence(&self, event: crate::SequenceEvent) { | ||
| let (hash, label) = match event { | ||
| crate::SequenceEvent::Connected(hash) => (hash, b'C'), | ||
| crate::SequenceEvent::Disconnected(hash) => (hash, b'D'), | ||
| }; | ||
| let mut next_sequence = self.next_sequence.lock(); | ||
| self.events.lock().push((hash, label, *next_sequence)); | ||
| *next_sequence += 1; | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn reorg_sequence_events_disconnect_old_tip_before_connecting_new_branch() | ||
| -> Result<(), Box<dyn std::error::Error>> { | ||
| let utxo = Arc::new(UtxoSet::new()); | ||
| let publisher = Arc::new(RecordingSequencePublisher::default()); | ||
| let publisher_handle: Arc<dyn crate::ZmqPublisher> = publisher.clone(); | ||
| let mut handles = apply_handles_without_tx_index(Network::Regtest, Arc::clone(&utxo)) | ||
| .with_zmq_publisher(publisher_handle); | ||
| let bodies = Arc::new(MapBodyStore::default()); | ||
| let body_handle: Arc<dyn crate::apply::PruneBodyStore> = bodies.clone(); | ||
| handles.block_body_store = Some(body_handle); | ||
|
|
||
| let genesis = bitcoin::blockdata::constants::genesis_block(bitcoin::Network::Regtest); | ||
| let genesis_hash = Hash256::from_le_bytes(genesis.block_hash().as_byte_array()); | ||
| let genesis_tip = applied_header_tip(&handles, genesis_hash, &genesis, 0)?; | ||
| handles.applied_tip.store(Some(Arc::new(genesis_tip))); | ||
|
|
||
| let old_one = mined_block_with_prev_hash_and_transactions( | ||
| genesis.block_hash(), | ||
| vec![coinbase_transaction(1)], | ||
| )?; | ||
| let old_one_raw = bytes::Bytes::from(bitcoin::consensus::encode::serialize(&old_one)); | ||
| let old_one_tip = apply_block_with_serialized(&handles, &old_one, old_one_raw.clone())?; | ||
| bodies | ||
| .bodies | ||
| .write() | ||
| .insert((old_one_tip.height, old_one_tip.hash), old_one_raw.to_vec()); | ||
|
|
||
| let old_two = mined_block_with_prev_hash_and_transactions( | ||
| old_one.block_hash(), | ||
| vec![coinbase_transaction(2)], | ||
| )?; | ||
| let old_two_raw = bytes::Bytes::from(bitcoin::consensus::encode::serialize(&old_two)); | ||
| let old_two_tip = apply_block_with_serialized(&handles, &old_two, old_two_raw.clone())?; | ||
| bodies | ||
| .bodies | ||
| .write() | ||
| .insert((old_two_tip.height, old_two_tip.hash), old_two_raw.to_vec()); | ||
| publisher.events.lock().clear(); | ||
| *publisher.next_sequence.lock() = 0; | ||
|
|
||
| let new_one = mined_block_with_prev_hash_and_transactions( | ||
| genesis.block_hash(), | ||
| vec![coinbase_transaction(3)], | ||
| )?; | ||
| let new_two = mined_block_with_prev_hash_and_transactions( | ||
| new_one.block_hash(), | ||
| vec![coinbase_transaction(4)], | ||
| )?; | ||
| let target = { | ||
| let mut tree = handles.block_tree.write(); | ||
| let mut target = None; | ||
| for (height, block) in [(1_u32, &new_one), (2_u32, &new_two)] { | ||
| target = Some(tree.insert_header(block.header, NodeStatus::HeaderValid)?); | ||
| bodies.bodies.write().insert( | ||
| ( | ||
| height, | ||
| Hash256::from_le_bytes(block.block_hash().as_byte_array()), | ||
| ), | ||
| bitcoin::consensus::encode::serialize(block), | ||
| ); | ||
| } | ||
| target.ok_or_else(|| anyhow::anyhow!("new branch has no target"))? | ||
| }; | ||
|
|
||
| crate::reorg::switch_to_branch(&handles, target, |_| None, |_| {})?; | ||
|
|
||
| let events = publisher.events.lock().clone(); | ||
| assert_eq!( | ||
| events, | ||
| vec![ | ||
| ( | ||
| Hash256::from_le_bytes(old_two.block_hash().as_byte_array()), | ||
| b'D', | ||
| 0 | ||
| ), | ||
| ( | ||
| Hash256::from_le_bytes(old_one.block_hash().as_byte_array()), | ||
| b'D', | ||
| 1 | ||
| ), | ||
| ( | ||
| Hash256::from_le_bytes(new_one.block_hash().as_byte_array()), | ||
| b'C', | ||
| 2 | ||
| ), | ||
| ( | ||
| Hash256::from_le_bytes(new_two.block_hash().as_byte_array()), | ||
| b'C', | ||
| 3 | ||
| ), | ||
| ] | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_disconnect_body_store_failure_moves_nothing() -> Result<(), Box<dyn std::error::Error>> { | ||
| let ReorgBodyLoadingFixture { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,6 +151,10 @@ pub struct Config { | |
| pub zmqpubrawblockhwm: Option<u32>, | ||
| /// Optional `rawtx` PUB socket high-water mark. | ||
| pub zmqpubrawtxhwm: Option<u32>, | ||
| /// ZMQ `sequence` PUB bind endpoints. | ||
| pub zmqpubsequence: Vec<String>, | ||
| /// Optional `sequence` PUB socket high-water mark. | ||
| pub zmqpubsequencehwm: Option<u32>, | ||
| /// Block height at or below which script verification is skipped during block apply. | ||
| /// | ||
| /// On mainnet the default is the hash-pinned assume-valid anchor | ||
|
|
@@ -211,6 +215,8 @@ impl fmt::Debug for Config { | |
| .field("zmqpubhashtxhwm", &self.zmqpubhashtxhwm) | ||
| .field("zmqpubrawblockhwm", &self.zmqpubrawblockhwm) | ||
| .field("zmqpubrawtxhwm", &self.zmqpubrawtxhwm) | ||
| .field("zmqpubsequence", &self.zmqpubsequence) | ||
| .field("zmqpubsequencehwm", &self.zmqpubsequencehwm) | ||
| .field("assume_valid_height", &self.assume_valid_height) | ||
| .finish_non_exhaustive() | ||
| } | ||
|
|
@@ -259,6 +265,8 @@ impl Config { | |
| zmqpubhashtxhwm: None, | ||
| zmqpubrawblockhwm: None, | ||
| zmqpubrawtxhwm: None, | ||
| zmqpubsequence: Vec::new(), | ||
| zmqpubsequencehwm: None, | ||
| assume_valid_height: network | ||
| .assume_valid_anchor() | ||
| .map_or(0, |(height, _)| height), | ||
|
|
@@ -357,6 +365,7 @@ impl Config { | |
| ("zmqpubhashtxhwm", self.zmqpubhashtxhwm), | ||
| ("zmqpubrawblockhwm", self.zmqpubrawblockhwm), | ||
| ("zmqpubrawtxhwm", self.zmqpubrawtxhwm), | ||
| ("zmqpubsequencehwm", self.zmqpubsequencehwm), | ||
| ] { | ||
| if hwm.is_some_and(|value| value > 2_147_483_647) { | ||
| bail!("{name} exceeds libzmq SNDHWM range"); | ||
|
|
@@ -393,6 +402,12 @@ impl Config { | |
| &self.zmqpubrawtx, | ||
| self.zmqpubrawtxhwm, | ||
| ); | ||
| push_zmq_publications( | ||
| &mut publications, | ||
| crate::zmq_publisher::ZmqTopic::Sequence, | ||
| &self.zmqpubsequence, | ||
| self.zmqpubsequencehwm, | ||
| ); | ||
|
Comment on lines
+405
to
+410
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ 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.
🤖 Prompt for AI AgentsSource: MCP tools There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Per-endpoint counters. All four existing topics share one counter per topic across that topic's endpoints, and one 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| publications | ||
| } | ||
|
|
||
|
|
@@ -427,6 +442,7 @@ impl Config { | |
| Ok(config) | ||
| } | ||
|
|
||
| #[allow(clippy::too_many_lines)] | ||
| fn apply_layer(&mut self, layer: &ConfigLayer) { | ||
| if let Some(network) = layer.network { | ||
| self.network = network; | ||
|
|
@@ -513,6 +529,9 @@ impl Config { | |
| if let Some(endpoints) = &layer.zmqpubrawtx { | ||
| self.zmqpubrawtx.clone_from(endpoints); | ||
| } | ||
| if let Some(endpoints) = &layer.zmqpubsequence { | ||
| self.zmqpubsequence.clone_from(endpoints); | ||
| } | ||
| if let Some(hwm) = layer.zmqpubhashblockhwm { | ||
| self.zmqpubhashblockhwm = Some(hwm); | ||
| } | ||
|
|
@@ -525,6 +544,9 @@ impl Config { | |
| if let Some(hwm) = layer.zmqpubrawtxhwm { | ||
| self.zmqpubrawtxhwm = Some(hwm); | ||
| } | ||
| if let Some(hwm) = layer.zmqpubsequencehwm { | ||
| self.zmqpubsequencehwm = Some(hwm); | ||
| } | ||
| if let Some(height) = layer.assume_valid_height { | ||
| self.assume_valid_height = height; | ||
| } | ||
|
|
@@ -624,6 +646,8 @@ pub(crate) struct ConfigLayer { | |
| pub(crate) zmqpubrawblock: Option<Vec<String>>, | ||
| #[arg(long = "zmqpubrawtx", value_delimiter = ',')] | ||
| pub(crate) zmqpubrawtx: Option<Vec<String>>, | ||
| #[arg(long = "zmqpubsequence", value_delimiter = ',')] | ||
| pub(crate) zmqpubsequence: Option<Vec<String>>, | ||
| #[arg(long = "zmqpubhashblockhwm")] | ||
| pub(crate) zmqpubhashblockhwm: Option<u32>, | ||
| #[arg(long = "zmqpubhashtxhwm")] | ||
|
|
@@ -632,6 +656,8 @@ pub(crate) struct ConfigLayer { | |
| pub(crate) zmqpubrawblockhwm: Option<u32>, | ||
| #[arg(long = "zmqpubrawtxhwm")] | ||
| pub(crate) zmqpubrawtxhwm: Option<u32>, | ||
| #[arg(long = "zmqpubsequencehwm")] | ||
| pub(crate) zmqpubsequencehwm: Option<u32>, | ||
| #[arg(long = "assume-valid-height")] | ||
| pub(crate) assume_valid_height: Option<u32>, | ||
| } | ||
|
|
@@ -708,6 +734,9 @@ impl ConfigLayer { | |
| "BITCOIN_RS_ZMQPUBRAWTX" => { | ||
| layer.zmqpubrawtx = Some(parse_string_list(value)); | ||
| } | ||
| "BITCOIN_RS_ZMQPUBSEQUENCE" => { | ||
| layer.zmqpubsequence = Some(parse_string_list(value)); | ||
| } | ||
| "BITCOIN_RS_ZMQPUBHASHBLOCKHWM" => { | ||
| layer.zmqpubhashblockhwm = Some(value.parse()?); | ||
| } | ||
|
|
@@ -720,6 +749,9 @@ impl ConfigLayer { | |
| "BITCOIN_RS_ZMQPUBRAWTXHWM" => { | ||
| layer.zmqpubrawtxhwm = Some(value.parse()?); | ||
| } | ||
| "BITCOIN_RS_ZMQPUBSEQUENCEHWM" => { | ||
| layer.zmqpubsequencehwm = Some(value.parse()?); | ||
| } | ||
| "BITCOIN_RS_ASSUME_VALID_HEIGHT" => { | ||
| layer.assume_valid_height = Some(value.parse()?); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
complete_disconnectreturns an error (for example, a backend write or flush failure),applied_tiphas already moved to the parent and the error is explicitly classified asMarkerStuck, meaning the rollback completed, but this later publication block is skipped by?. Apubsequencesubscriber therefore remains on the disconnected block even though RPC exposes the parent tip; publishDimmediately after the tip store, before this fallible marker cleanup.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid, and it's the mirror image of the connect bug you caught earlier — fixing it the same way.
complete_disconnectruns afterapplied_tiphas already been stored to the parent, which is precisely why its failure is classifiedMarkerStuckrather 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
Demission 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 everyDprecedes the replacement branch'sCs.