Separate Warning into its own channel, refactor transaction rejection naming
#268
Conversation
This file contains 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
A transaction that is met with a reject message may be for a low fee rate. When connected to multiple peers, it is possible to receive a reject, but the transaction may still propagate. In this sense, "failure" is not the correct term
rustaceanrob
commented
Jan 15, 2025
rustaceanrob
force-pushed
the
warn-1-14
branch
from
895fc6e to
003552c
Compare
rustaceanrob
force-pushed
the
warn-1-14
branch
from
003552c to
9c10396
Compare
rustaceanrob
force-pushed
the
warn-1-14
branch
from
8ddd20d to
94dde90
Compare
nyonson
reviewed
Jan 15, 2025
| @@ -48,7 +48,7 @@ impl Dialog { | |||
| let _ = self.log_tx.send(Log::Dialog(message)).await; | |||
| } | |||
|
|
|||
| pub(crate) async fn send_warning(&self, warning: Warning) { | |||
There was a problem hiding this comment.
I believe the send call is async right? Kinda looks weird dropping the wrapper async if that is the case. Are there any issues in the tokio runtime with this though?
There was a problem hiding this comment.
UnboundedSender<T> is sync. CI would hopefully fail for not awaiting a future
The previous bounded mpsc channel has an async send call.
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.
Previous to this PR, the relationship between
LogandWarningis that aWarningis contained in aLog. Because the log channel had limited capacity, aWarningevent can technically be missed if the receiver is lagging behind. To compensate for this, transaction rejections at the network level were put into theEventenum, which have an unbounded channel to miss anyEvent. In reality though, a user would probably not want to miss anyWarningat all, including transaction rejections.As an aside, the term "failure" in
FailurePayloadis not correct, as a single node may reject a transaction while others accept it. Instead, theFailurePayloadis renamed toRejectPayloadto more accurately reflect the situation.This PR introduces the
RejectPayloadinto the transaction rejection variant onWarning. In addition, theWarningvariant is removed fromLog, andWarningevents are given their own channel. Now there is a clear separation of concerns between aLog,Warning, andEventwithout ambiguity of what data should be in each type.@nyonson the bulk of the refactor are one line changes to account for this new warning channel sender/receiver. I tried to break the commits into easily reviewable parts, minus the head commit that has to do some heavy lifting. Of note in that one, any time the node needs to talk to the client, it must do so through the
Dialogstruct. To redirect theWarningmessages to the new channel instead of withLog, all that changes is the implementation ofDialog::send_warning.