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.

Sign up for GitHub

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

Jump to bottom

Check for unexpected thunks in TxSubmission.Inbound state #1708

Merged
merged 6 commits into from
Apr 14, 2020
Merged

Check for unexpected thunks in TxSubmission.Inbound state #1708

merged 6 commits into from
Apr 14, 2020

Conversation

intricate
Copy link
Contributor

Closes #1645

@intricate intricate added this to the S7 2020-02-27 milestone Feb 27, 2020
@intricate intricate self-assigned this Feb 27, 2020
intricate
intricate commented Feb 27, 2020
View reviewed changes
stack.yaml Outdated Show resolved Hide resolved
@intricate intricate force-pushed the intricate/1645 branch 3 times, most recently from a09ffb7 to b0b72ab Compare February 27, 2020 05:14
mrBliss
mrBliss reviewed Feb 27, 2020
View reviewed changes
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

Looking good. One thing I noticed: you can easily forget to use continueWithState somewhere where you should have, the types won't help you. In the ChainSyncClient we added Stateful to the type signatures (where possible) for that reason. Can you do the same here?

Also, be sure to run the testsuite with thunk checking enabled, i.e., enable the checktvarinvariant flag in the io-sim-classes. E.g.,

stack test --flag io-sim-classes:checktvarinvariant <testsuite(s)> --ta '--quickcheck-tests=20' |& grep -v closurePtr

I'm hoping you'll some failures, as there are no bangs on the fields of ServerState 🙂

ouroboros-consensus/src/Ouroboros/Consensus/Ledger/Dual.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs Outdated Show resolved Hide resolved
@mrBliss mrBliss requested a review from dcoutts February 27, 2020 08:09
@intricate intricate force-pushed the intricate/1645 branch 4 times, most recently from 6feb137 to 3b9633a Compare March 3, 2020 02:26
@intricate intricate requested a review from mrBliss March 3, 2020 14:58
@intricate intricate force-pushed the intricate/1645 branch 4 times, most recently from 998d85b to ec241cb Compare March 12, 2020 21:05
@mrBliss mrBliss modified the milestones: S8 2020-03-12, S9 2020-03-26 Mar 17, 2020
@intricate intricate force-pushed the intricate/1645 branch 3 times, most recently from 64abb79 to 457afc8 Compare March 27, 2020 17:01
@intricate intricate requested a review from coot March 27, 2020 17:01
@intricate intricate force-pushed the intricate/1645 branch 2 times, most recently from 5c260b1 to 8e396d4 Compare March 30, 2020 17:08
@mrBliss mrBliss modified the milestones: S9 2020-03-26, S10 2020-04-09 Apr 2, 2020
@intricate intricate force-pushed the intricate/1645 branch 2 times, most recently from 652d721 to 84098c6 Compare April 7, 2020 15:16
@intricate intricate added the tx-submission Issues related to tx-submission protocol label Apr 7, 2020
coot
coot approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM

@intricate
Replace usages of unsafeNoThunks with unsafeNoUnexpectedThunks
f931285
@intricate
Check for unexpected thunks in TxSubmission.Inbound state
0d0298b
@intricate
Make all fields strict in TxSubmission.Inbound ServerState
0dc843e
@intricate
Add and utilize Stateful, StatefulM, and StatefulCollect types
5995c3f
@intricate
Refactoring for usage of Stateful, StatefulM, and StatefulCollect
ec9e9e7
@intricate
Force thunk in bufferedTxs' binding in acknowledgeTxIdsInMempool
39f1619 
Apparently, the `Semigroup` operation for `Map` is `union`, which is
actually not implemented in such a way that values of the resulting
`Map` are forced to WHNF.

An odd thing that I noticed is that the `union` function that is exposed
from the `Data.Map.Strict` module is the same as that exposed from the
`Data.Map.Lazy` module. Internally, this function uses lazy `insert`s
and therefore provides no guarantee that the values of the resulting
`Map` will be in WHNF. It seems misleading to me that this function
would be exposed from `Data.Map.Strict` without providing that guarantee
which is promised by the module (see its documentation).
@mrBliss
Copy link
Contributor

mrBliss commented Apr 14, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 14, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 4a8d7e2 into master Apr 14, 2020
@intricate intricate deleted the intricate/1645 branch April 14, 2020 23:30
coot pushed a commit that referenced this pull request May 16, 2022
@iohk-bors @intricate
Merge #1708
d0a952c 
1708: Check for unexpected thunks in TxSubmission.Inbound state r=mrBliss a=intricate

Closes #1645 

Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coot coot coot approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@mrBliss mrBliss Awaiting requested review from mrBliss

Assignees

@intricate intricate

Labels
tx-submission Issues related to tx-submission protocol
Projects
None yet
Milestone
S10 2020-04-09
Development

Successfully merging this pull request may close these issues.

Avoid space leaks in TxSubmission.Inbound
4 participants
@intricate @mrBliss @coot @vhulchenko-iohk