chainntnfs: fix missing notifications #9258
Conversation
yyforyongyu
added
notifications
bug fix
flake fix
size/micro
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
chainntnfs/txnotifier.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| // Deliver the details to the conf set. | ||
| for _, ntfn := range confSet.ntfns { |
There was a problem hiding this comment.
nit: don't shadow the outer ntfn name to make it more clear what notification we're notifying on?
Also, didn't look super deep into this, but could this lead to duplicate notifications? Probably not a big issue as we should always be able to handle duplicates.
There was a problem hiding this comment.
Or is there only ever one notification being in rescan? My assumption is multiple of the set could be and then they would all notify. Perhaps I'm also misunderstanding things here.
There was a problem hiding this comment.
nit: don't shadow the outer ntfn name to make it more clear what notification we're notifying on?
Done.
Or is there only ever one notification being in rescan? My assumption is multiple of the set could be and then they would all notify. Perhaps I'm also misunderstanding things here.
Yeah the water is a bit deep here, also not very confident about this package either😂 - I think the rescan is for every notification, which can def be optimized. As for duplicates, because the ntfn will be marked as dispatched=true and we will decide to notify or not by checking dispatched, I don't think we will see duplicates.
There was a problem hiding this comment.
turns out the dispatched=true only protects the Confirmed notification. For the Updates notification, which sends the num of confs left (I think we never use it?), we may still have duplicates. To avoid the dups, I created a commit in the end to skip if the same update has already been sent before.
yyforyongyu
force-pushed
the
fix-notification
branch
from
66f0251 to
8445c0a
Compare
chainntnfs/interface.go
Outdated
| // We cannot rely on the subscriber to immediately read from | ||
| // the channel so we need to create a larger buffer to avoid | ||
| // blocking the notifier. | ||
| Confirmed: make(chan *TxConfirmation, numConfs), |
There was a problem hiding this comment.
Why do we need a buffer of up to numConfs? Is this for some degenerate case where re-orgs constantly happen each time after a confirmation is notified?
There was a problem hiding this comment.
Turns out the ntfn.Event.Confirmed is fine as we only send out the confirmed event once, although the pattern is still dangerous as one single failure in sending to the Confirmed channel can lead to blocking other notifications.
The channel in question is the ntfn.Event.Updates, and as it's not filtered by the dispatched variable, we may send duplicate notifications there. To fix this, I added a new commit at the end to skip sending the duplicates.
This commit changes the order of notifications when a relevant tx is found in a block and now we will always notify the tx subscribers before notifying the block, which has implications in the upcoming blockbeat. When a block notification is subscribed via `RegisterBlockEpochNtfn` and a confirm or spend is subscribed via `RegisterConfirmationsNtfn` or `RegisterSpendNtfn`, we would always notify the block first before the tx, causing the subsystem to think there's no relevant txns found in this block, while the notifications are sent later. We now fix it by always sending the txns notifications first, so the subsystem can receive the txns, process them, then attempt to advance its state based on the block received.
This commit fixes a bug where the confirmation details may be missed. When the same tx is subscribed via `RegisterConfirmationsNtfn`, we will put them into the same set and notify the whole set. However, this logic is missing when performing the rescan - once the confirmation detail is found, we only notify the current subscriber. Later we will skip notifying other subscribers in `UpdateConfDetails` due to the `confSet.details != nil` check. We now fix it by immediately notify all the subscribers when the confirmation detail is found during the rescan.
So it's easier to handle the following commit where we start skipping duplicate notifications.
yyforyongyu
force-pushed
the
fix-notification
branch
from
5a6264b to
db7ca2b
Compare
This commit adds a new state to the `ConfNtfn` struct to start tracking the number of confs left to be notified to avoid sending duplicate notifications.
yyforyongyu
force-pushed
the
fix-notification
branch
from
db7ca2b to
db6901c
Compare
|
I think this has some diff marks in the release docs. |
Cherry-picked commits from the final
blockbeatto reduce the PR size.This PR made two minor changes,