Skip to content
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

Secondary mechanism to trigger watches for transactions from past blocks #3002

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Feb 6, 2025

When a new block is found, we want to check its confirmed transactions to potentially trigger watches. This is especially important when a channel is spent and we haven't seen the spending transaction in our mempool before receiving it in a block.

This is already handled through the ZMQ rawtx topic, where bitcoind sends us every transaction it receives (either in the mempool or in a block). But when using remote bitcoind instances, ZMQ seems to sometimes be unreliable and silently drop some events (mostly when the connection is unstable with the bitcoind instance). That's why we add another mechanism for extra safety, where whenever a new block is found, we fetch the last N blocks and re-process their transactions. We keep a cache of the processed blocks to ensure that we don't needlessly re-process them multiple times.

@t-bast t-bast requested review from sstone and pm47 February 6, 2025 14:58
When a new block is found, we want to check its confirmed transactions
to potentially trigger watches. This is especially important when a
channel is spent and we haven't seen the spending transaction in our
mempool before receiving it in a block. This is already supposed to be
handled through the ZMQ `rawtx` topic, where bitcoind should send us
every transaction it receives (either in the mempool or in a block).
But when using remote `bitcoind` instances, ZMQ seems to sometimes be
unreliable and silently drop some events. That's why we add another
mechanism for extra safety, where whenever a new block is found, we
fetch the last `N` blocks and re-process their transactions. We keep
a cache of the processed blocks to ensure that we don't needlessly
re-process them multiple times.
@t-bast t-bast force-pushed the zmq-check-latest-blocks branch from d7d00fb to d32a21a Compare February 6, 2025 15:18
@pm47
Copy link
Member

pm47 commented Feb 10, 2025

But when using remote bitcoind instances, ZMQ seems to sometimes be unreliable and silently drop some events (mostly when the connection is unstable with the bitcoind instance)

Did you notice that all ZMQ messages contain a sequence specifically for the purpose of detecting lost messages? For example hashblock.

Also, I wonder if the cause of unreliability could be that we are using the same ZMQ adress for both transactions and blocks. Could we run into high watermarks due to a burst of transactions (maybe unrelayed txs directly found in a new block), and drop blocks?

Finally, the implementation looks fine to me, but I wonder if we should instead rely on sequence messages and make the process a little more blockchainy: something like storing the last analyzed blockId and make sure that the new one is its direct child. It's not explicitly recommended by the doc but they do mention it in a section about reorgs.

@t-bast
Copy link
Member Author

t-bast commented Feb 10, 2025

Did you notice that all ZMQ messages contain a sequence specifically for the purpose of detecting lost messages? For example hashblock.

Yes, but what would you do when you detect that you missed events? There is no mechanism to ask for a retransmission of a past event.

Also, I wonder if the cause of unreliability could be that we are using the same ZMQ adress for both transactions and blocks.

According to bitcoind, this shouldn't matter.

Could we run into high watermarks due to a burst of transactions (maybe unrelayed txs directly found in a new block), and drop blocks?

We've already disabled the high watermark (see ZmqActor.scala), so this shouldn't be related.

The main case where this issue can happen is when we get disconnected or if eclair or bitcoind restarts: bitcoind won't store events that happen while disconnected/offline to retransmit them on reconnection (because in most cases, this isn't the expected behavior of the connecting client and is impossible to fully guarantee anyway).

Finally, the implementation looks fine to me, but I wonder if we should instead rely on sequence messages and make the process a little more blockchainy: something like storing the last analyzed blockId and make sure that the new one is its direct child. It's not explicitly recommended by the doc but they do mention it in a section about reorgs.

I think this would be much more complex to implement correctly than the simple behavior of this PR?

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough! Just a nit

@t-bast t-bast merged commit bc44808 into master Feb 11, 2025
1 check passed
@t-bast t-bast deleted the zmq-check-latest-blocks branch February 11, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants