Detect new mempool txs #1988
Merged
Detect new mempool txs #1988
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evanlinjin
force-pushed
the
fix/bitcoind_rpc_mempool
branch
from
c494aef to
7b010af
Compare
There was a problem hiding this comment.
cACK
I think the new simplified approach looks better.
I'm still in the process of a deeper review. However, when I'm trying to run the detect_new_mempool_txs, on both commits, it takes more than 60 seconds and becomes stale, still unsure if it's a local issue.
|
@oleonardolima are you on Linux or OSX? |
FWIW, the |
tnull
reviewed
Jul 7, 2025
This comment was marked as resolved.
This comment was marked as resolved.
Instead of having an avoid-reemission logic based on timestamps returned by bitcoind RPC, we wrap all transactions in `Arc` and always emit the entire mempool. This makes emission cheap while avoiding the flawed avoid-reemission logic.
This is the more performant method. The downside is that mempool txids are not returned to the seen-in-mempool timestamps so the caller either needs to provide this, or we need to rely on std to get the current timestamp. * `Emitter::mempool` method now requires the `std` feature. A non-std version of this is added: `Emitter::mempool_at` which takes in a `sync_time` parameter. Additional changes: * `NO_EXPECTED_MEMPOOL_TXIDS` is renamed to `NO_EXPECTED_MEMPOOL_TXS`. * Updated documentation. * Fix imports so that `bdk_bitcoind_rpc` compiles without `std`.
evanlinjin
force-pushed
the
fix/bitcoind_rpc_mempool
branch
from
16199c2 to
7de3b71
Compare
evanlinjin
force-pushed
the
fix/bitcoind_rpc_mempool
branch
from
b09a2d9 to
73ab1eb
Compare
oleonardolima
approved these changes
Jul 18, 2025
There was a problem hiding this comment.
tACK 73ab1eb
It looks good. I've tested in regtest with the bitcoind_rpc example, and it worked just fine.
|
cACK 51ee99a |
8 tasks
16 tasks
kwsantiago
pushed a commit
to kwsantiago/bdk
that referenced
this pull request
Aug 5, 2025
51ee99a docs(bitcoind_rpc): fixed typo in docs (Wei Chen) 73ab1eb chore(bitcoind_rpc): Make clippy happy (志宇) 7e894f4 feat(bitcoind_rpc)!: Use `getrawmempool` without verbose (志宇) 05464ec fix(bitcoind_rpc)!: Simplify emitter (志宇) 67dfb0b test(bitcoind_rpc): Detect new mempool txs (志宇) Pull request description: ### Description There is a bug in `bdk_bitcoind_rpc` where some new mempool transactions will not be emitted at all. This problem exists because the avoid-re-emission logic depends on rounded-to-nearest-second timestamps. The fix is to just emit all mempool transactions but wrap them in `Arc`s so that emission becomes cheap. **Background:** I tried using `bdk_bitcoind_rpc` as the chain-source to write an example to showcase the [`IntentTracker`](bitcoindevkit/bdk_wallet#257). However, `bdk_bitcoind_rpc` failed to emit some mempool transactions. ### Notes to the reviewers The test added in c22c68f fails without these fixes. Some tests are removed as they are no longer relevant. ### Changelog notice ```md Fixed: - Some mempool transactions not being emitted at all. The fix is to replace the avoid-re-emission-logic with one which emits all mempool transactions. ``` ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo +nightly fmt` and `cargo clippy` before committing #### Bugfixes: * [x] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing ~* [ ] I'm linking the issue being fixed by this PR~ ACKs for top commit: nymius: cACK 51ee99a LagginTimes: Re-ACK 51ee99a Tree-SHA512: 04e180e1d28c3f4c581a61ccac95e8e7e6927123d272ed07eae0ae51bf70799df44298b47ba0e49a309fd76366875e8d18d73478252931713137844857b8ed5a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There is a bug in
bdk_bitcoind_rpcwhere some new mempool transactions will not be emitted at all.This problem exists because the avoid-re-emission logic depends on rounded-to-nearest-second timestamps.
The fix is to just emit all mempool transactions but wrap them in
Arcs so that emission becomes cheap.Background: I tried using
bdk_bitcoind_rpcas the chain-source to write an example to showcase theIntentTracker. However,bdk_bitcoind_rpcfailed to emit some mempool transactions.Notes to the reviewers
The test added in c22c68f fails without these fixes.
Some tests are removed as they are no longer relevant.
Changelog notice
Fixed: - Some mempool transactions not being emitted at all. The fix is to replace the avoid-re-emission-logic with one which emits all mempool transactions.Checklists
All Submissions:
cargo +nightly fmtandcargo clippybefore committingBugfixes:
* [ ] I'm linking the issue being fixed by this PR