-
Notifications
You must be signed in to change notification settings - Fork 86
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
Trace transaction requests and replies in txSubmissionOutbound #1688
Conversation
ouroboros-network/src/Ouroboros/Network/TxSubmission/Outbound.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/src/Ouroboros/Network/TxSubmission/Outbound.hs
Outdated
Show resolved
Hide resolved
data TraceTxSubmissionOutbound txid tx = TraceTxSubmissionOutbound --TODO | ||
data TraceTxSubmissionOutbound txid tx | ||
= TraceTxSubmissionOutboundRecvMsgRequestTxs | ||
![txid] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bang on a list doesn't help that much: just the head of the list is forced to WHNF, not the tail. To be sure the list is forced, you could use forceElemsToWHNF
, but I would not bother with that. I'd just leave out the bangs, they give a false impression of what's going on.
It would be more important if the txids
are computed lazily, e.g., map txId txs
, but that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I've gotten myself into the habit of forcing all fields to WHNF. Of course, as you mentioned, this does not guarantee that the elements of a container are in WHNF, but I've found that it's generally not a bad idea to ensure strictness at the field-level unless laziness would be particularly useful. We also do the same thing for fields of TraceEventMempool
.
However, I'm not particularly committed to this idea unless it actually helps to force to WHNF, so I'll remove the bangs and push.
c9106de
to
17dd08b
Compare
17dd08b
to
f12e7c2
Compare
bors r+ |
Related to IntersectMBO/cardano-node#527.
More specifically, see IntersectMBO/cardano-node#584 (review).