This issue tracks the work items identified in a full review of lib.rs and test_emitter.rs on master (de7a89f), separated into 3 broad categories: 1) Internal fixes, 2) API + Documentation, 3) Test coverage.
1. Quirks (needs investigation)
The inner loop in mempool_at calls 5 RPCs per iteration and retries immediately with no sleep, no ceiling on retries, and the HashSet<Txid> allocation is repeated on every failed iteration. It doesn't remove a race condition that exists between the time of getting the raw mempool and the time the full transactions are retrieved from the node.
This pattern is legal but unusual Rust. The loop is really "retry until consistent snapshot"— an fn fetch_consistent_mempool_snapshot() that returns a named struct would be cleaner.
poll responds to AgreementPointNotFound by resetting to genesis and looping. If genesis is somehow inaccessible or the node has a persistent inconsistency, this spins forever. Although it's a pathological case, the recommendation would be to return a hard error after a bounded number of consecutive failures.
The fallback None => self.checkpoint.block_id() (returns itself) is unreachable via the emitter in practice, since every emitted checkpoint gets pushed onto the last cp. For correctness, should use (block_height - 1, block.header.prev_blockhash) directly which is consistent with the intended block-by-block behavior.
2. API Design + Documentation
MempoolEvent::update originally documented the timestamp as a "first-seen unix timestamp". For transactions in mempool_snapshot, the current sync_time is used on every poll — not the time the tx was first observed.
start_height was intended as a birthday/scan-start height: "don't emit blocks before this height." PR #2167 exposed a bug where, after a reorg that drops the agreement point below start_height, the emitter skips directly to start_height rather than re-emitting the invalidated heights. The proposed fix modifies start_height internally at runtime to the agreement height, circumventing the stated purpose of the parameter and making the behavior difficult to explain.
Two options:
-
Remove start_height entirely. The birthday/scan-start is encoded directly in last_cp: callers build a checkpoint containing the birthday height. The emitter scans from there naturally.
-
Add an alternate constructor that does not take start_height and relies solely on last_cp for the start height, keeping the existing constructor for backward compatibility.
mempool_at contains the actual at_tip check and eviction logic, but its doc just says "no-std version of mempool". The eviction-guard semantics (withheld until emitter reaches tip, rationale) belong on mempool_at, with mempool() cross-referencing it.
The constant is a typed alias for core::iter::empty() hardcoded to T = Arc<Transaction>. This causes type-inference friction when callers pass Transaction directly.
BlockEvent<B> exists solely to support the private poll<C, V, F> closure abstraction. next_block is the only public consumer and always returns BlockEvent<Block>.
These are the algorithmic heart of the emitter. Working branch adds state-machine docs explaining entry invariants, what each PollResponse variant means, and the reorg-detection flow.
3. Test Coverage
The test mempool_avoids_re_emission has a doc comment that says "mempool txs should not be re-emitted" but its own assertions loop twice and assert that the same txids appear on both calls. Should be renamed to something like mempool_update_is_full_snapshot and the doc rewritten to describe what it actually tests: "every call to mempool() returns all currently-known unconfirmed transactions, including ones returned on previous calls."
The comment references EmittedUpdate::into_tx_graph_update — a deleted API. The test body is fine. Comment should be rewritten to describe what the test actually covers. Additionally addr_1 and addr_2 are generated and tracked but never funded, adding noise without coverage.
ensure_block_emitted_after_reorg_is_at_reorg_height contains TODO: If the reorg height is lower than the fallback height... — this is not a test concern, it is a design question. Should be removed from the test and tracked as a separate issue.
test_expect_tx_evicted tests eviction only when already at tip. The complementary scenario — evict a tx while the emitter is still behind tip, assert evicted is empty in that window, advance to tip, then assert the eviction is reported — is untested.
The unit test test_expected_mempool_txids_accumulate_and_remove verifies accumulation and removal on confirmation, but there is no test that: (1) passes a known-unconfirmed tx to Emitter::new via expected_mempool_txs, (2) evicts that tx from the node's mempool, and (3) asserts it appears in MempoolEvent::evicted. Edit: Covered by test_expect_tx_evicted.
No test verifies the fundamental birthday guarantee: an emitter with last_cp at height N must not emit any block at or below N.
mempool_at is never called in integration tests to verify that each mempool tx carries the custom sync_time passed into the API.
This issue tracks the work items identified in a full review of lib.rs and test_emitter.rs on master (de7a89f), separated into 3 broad categories: 1) Internal fixes, 2) API + Documentation, 3) Test coverage.
1. Quirks (needs investigation)
mempool_atconsistency loopThe inner
loopinmempool_atcalls 5 RPCs per iteration and retries immediately with no sleep, no ceiling on retries, and theHashSet<Txid>allocation is repeated on every failed iteration. It doesn't remove a race condition that exists between the time of getting the raw mempool and the time the full transactions are retrieved from the node.let mutdeclarations before the loopThis pattern is legal but unusual Rust. The loop is really "retry until consistent snapshot"— an
fn fetch_consistent_mempool_snapshot()that returns a named struct would be cleaner.AgreementPointNotFoundarm creates an unprotected infinite looppollresponds toAgreementPointNotFoundby resetting to genesis and looping. If genesis is somehow inaccessible or the node has a persistent inconsistency, this spins forever. Although it's a pathological case, the recommendation would be to return a hard error after a bounded number of consecutive failures.connected_to()returns itself when checkpoint has no parentThe fallback
None => self.checkpoint.block_id()(returns itself) is unreachable via the emitter in practice, since every emitted checkpoint gets pushed onto the last cp. For correctness, should use(block_height - 1, block.header.prev_blockhash)directly which is consistent with the intended block-by-block behavior.2. API Design + Documentation
mempoolrefers to first-seen unix timestampsMempoolEvent::updateoriginally documented the timestamp as a "first-seen unix timestamp". For transactions inmempool_snapshot, the currentsync_timeis used on every poll — not the time the tx was first observed.start_heightsemanticsstart_heightwas intended as a birthday/scan-start height: "don't emit blocks before this height." PR #2167 exposed a bug where, after a reorg that drops the agreement point belowstart_height, the emitter skips directly tostart_heightrather than re-emitting the invalidated heights. The proposed fix modifiesstart_heightinternally at runtime to the agreement height, circumventing the stated purpose of the parameter and making the behavior difficult to explain.Two options:
Remove
start_heightentirely. The birthday/scan-start is encoded directly inlast_cp: callers build a checkpoint containing the birthday height. The emitter scans from there naturally.Add an alternate constructor that does not take
start_heightand relies solely onlast_cpfor the start height, keeping the existing constructor for backward compatibility.mempool_atdoc should carry the canonical eviction guard explanationmempool_atcontains the actualat_tipcheck and eviction logic, but its doc just says "no-std version ofmempool". The eviction-guard semantics (withheld until emitter reaches tip, rationale) belong onmempool_at, withmempool()cross-referencing it.NO_EXPECTED_MEMPOOL_TXSconstant adds friction, not clarityThe constant is a typed alias for
core::iter::empty()hardcoded toT = Arc<Transaction>. This causes type-inference friction when callers passTransactiondirectly.BlockEvent<B>generic is vestigialBlockEvent<B>exists solely to support the privatepoll<C, V, F>closure abstraction.next_blockis the only public consumer and always returnsBlockEvent<Block>.poll/poll_onceare undocumentedThese are the algorithmic heart of the emitter. Working branch adds state-machine docs explaining entry invariants, what each
PollResponsevariant means, and the reorg-detection flow.CheckPoint should be generalized for any data type Make
bdk_bitcoind_rpcemitters support any checkpoint data type #2105Move away from outdated
bitcoincore_rpc::ClientRefactor: update bdk-bitcoind-rpc to use bdk-bitcoind-client #21193. Test Coverage
mempool_avoids_re_emissiondoc commentThe test
mempool_avoids_re_emissionhas a doc comment that says "mempool txs should not be re-emitted" but its own assertions loop twice and assert that the same txids appear on both calls. Should be renamed to something like mempool_update_is_full_snapshot and the doc rewritten to describe what it actually tests: "every call to mempool() returns all currently-known unconfirmed transactions, including ones returned on previous calls."test_into_tx_graphdoc commentThe comment references
EmittedUpdate::into_tx_graph_update— a deleted API. The test body is fine. Comment should be rewritten to describe what the test actually covers. Additionallyaddr_1andaddr_2are generated and tracked but never funded, adding noise without coverage.ensure_block_emitted_after_reorg_is_at_reorg_heightcontainsTODO: If the reorg height is lower than the fallback height...— this is not a test concern, it is a design question. Should be removed from the test and tracked as a separate issue.test_expect_tx_evictedtests eviction only when already at tip. The complementary scenario — evict a tx while the emitter is still behind tip, assertevictedis empty in that window, advance to tip, then assert the eviction is reported — is untested.expected_mempool_txseviction path is not end-to-end testedThe unit test
test_expected_mempool_txids_accumulate_and_removeverifies accumulation and removal on confirmation, but there is no test that: (1) passes a known-unconfirmed tx toEmitter::newviaexpected_mempool_txs, (2) evicts that tx from the node's mempool, and (3) asserts it appears inMempoolEvent::evicted. Edit: Covered bytest_expect_tx_evicted.start_heightNo test verifies the fundamental birthday guarantee: an emitter with
last_cpat height N must not emit any block at or below N.Missing test coverage for
BlockEvent::connected_to()mempool_atis never called in integration testsmempool_atis never called in integration tests to verify that each mempool tx carries the customsync_timepassed into the API.