Protocol timeouts #1813

karknu · 2020-03-16T16:21:03Z

Implements #1395, apart from the multi-chunk message timeout.

ouroboros-consensus/ouroboros-consensus-test-infra/src/Test/ThreadNet/Network.hs

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs

coot

@karknu I realised some of my comments are about size limits, while this PR is only about timeouts. I didn't removed them, as we need to discuss these anyway, but we can ignore them to approve this PR.

coot · 2020-03-20T08:23:08Z

ouroboros-network/src/Ouroboros/Network/Protocol/BlockFetch/Codec.hs

+  where
+    stateToLimit :: forall (pr :: PeerRole) (st :: BlockFetch block).
+                    PeerHasAgency pr st -> Word
+    stateToLimit (ClientAgency TokIdle)      = smallByteLimit


@karknu I think it would be good to document why all messages in the TokIdle state are smaller than: smallByteLimits. These are mostly trivial things , but it would be good to have it documented.

If we start using ToCBOR or at least have encodedSizeExpr for all the codecs we could add tests for these limits, let's create an issue for this. The limitation is that one cannot compute encodedSizeExpr of things like block or tx, but it's possible to do that for headers.

coot · 2020-03-20T08:32:04Z

ouroboros-network/src/Ouroboros/Network/Protocol/BlockFetch/Codec.hs

+                    PeerHasAgency pr st -> Word
+    stateToLimit (ClientAgency TokIdle)      = smallByteLimit
+    stateToLimit (ServerAgency TokBusy)      = smallByteLimit
+    stateToLimit (ServerAgency TokStreaming) = largeByteLimit


We have a dependency between these limits and blockFetchProtocolLimits; I was more conservative there: I estimated max message size to 2_097_154 bytes (which is 2MB block + cbor overhead); I think these two limits (mux ingress queue size limits & per protocol state limits) should agree.

Strictly speaking they don't have to agree. The conjunction is enforced. But yes it's more consistent.

ouroboros-network/src/Ouroboros/Network/Protocol/BlockFetch/Codec.hs

coot · 2020-03-20T08:41:22Z

ouroboros-network/src/Ouroboros/Network/Protocol/ChainSync/Codec.hs

+  where
+    stateToLimit :: forall (pr :: PeerRole) (st :: ChainSync header tip).
+                    PeerHasAgency pr st -> Word
+    stateToLimit (ClientAgency TokIdle)                = smallByteLimit


The same here; But we actually know the maximal header size. It's documented in NodeToNode module, so we can give more adequate limits.

ouroboros-network/src/Ouroboros/Network/Protocol/BlockFetch/Codec.hs

coot · 2020-03-20T08:50:02Z

ouroboros-network/src/Ouroboros/Network/Protocol/TxSubmission/Codec.hs

+  where
+    stateToLimit :: forall (pr :: PeerRole) (st :: TxSubmission txid tx).
+                    PeerHasAgency pr st -> Word
+    stateToLimit (ClientAgency (TokTxIds TokBlocking))    = largeByteLimit


Also here we can use estimations which were calculated for mux ingress queue + enhancing the haddocks.

ouroboros-network-framework/src/Ouroboros/Network/Snocket.hs

network-mux/src/Network/Mux/Bearer/Socket.hs

karknu · 2020-03-20T10:32:30Z

@coot I will update the size limit ticket with references to your byteLimits comments to ensure that they aren't forgotten.

coot · 2020-03-20T10:42:28Z

@karknu thanks; then just please update the haddocs in this PR and I'll accept it.

coot

LGTM; just a reminder to append to a comment.

ouroboros-network-framework/src/Ouroboros/Network/Snocket.hs

dcoutts

Looking very good.

A couple questions and comments below.

dcoutts · 2020-03-23T21:41:06Z

ouroboros-network/src/Ouroboros/Network/Protocol/BlockFetch/Codec.hs

+                    PeerHasAgency pr st -> Word
+    stateToLimit (ClientAgency TokIdle)      = smallByteLimit
+    stateToLimit (ServerAgency TokBusy)      = smallByteLimit
+    stateToLimit (ServerAgency TokStreaming) = largeByteLimit


Strictly speaking they don't have to agree. The conjunction is enforced. But yes it's more consistent.

dcoutts · 2020-03-23T21:51:58Z

ouroboros-network/src/Ouroboros/Network/Protocol/TxSubmission/Codec.hs

+    stateToLimit (ClientAgency (TokTxIds TokBlocking))    = largeByteLimit
+    stateToLimit (ClientAgency (TokTxIds TokNonBlocking)) = largeByteLimit


I think these can be the small byte limit. The messages in this state are transaction ids, not transactions. With the 64k limit you could fit 2k tx ids. Our maximum outstanding window size is less than 2k. It's currently 100.

Also, this could be collapsed into one case with a wildcard on the blocking type.

ouroboros-network/src/Ouroboros/Network/Protocol/TxSubmission/Codec.hs

network-mux/src/Network/Mux/Bearer/Socket.hs

karknu · 2020-03-24T09:45:15Z

bors r+

1813: Protocol timeouts r=karknu a=karknu Implements #1395, apart from the multi-chunk message timeout. Co-authored-by: Karl Knutsson <[email protected]>

karknu · 2020-03-24T10:34:42Z

bors r-

iohk-bors · 2020-03-24T10:34:43Z

Canceled

Implement an optional timeout for reading Mux SDUs. This adds a 30s timeout when reading SDUs over a socketSnocket (IPv4/IPv6). There is no timeout for reading messages over localSnockets.

dcoutts · 2020-03-24T14:25:08Z

bors r+

iohk-bors · 2020-03-24T14:41:03Z

Build succeeded

mrBliss reviewed Mar 16, 2020

View reviewed changes

ouroboros-consensus/ouroboros-consensus-test-infra/src/Test/ThreadNet/Network.hs Show resolved Hide resolved

karknu force-pushed the karknu/moar_timeouts branch from d429ebd to 27a9ee4 Compare March 16, 2020 17:06

mrBliss force-pushed the karknu/moar_timeouts branch from 27a9ee4 to 63b6755 Compare March 17, 2020 09:58

karknu force-pushed the karknu/moar_timeouts branch 10 times, most recently from 388066e to 51c77cd Compare March 19, 2020 09:28

karknu requested review from coot, dcoutts and mrBliss March 19, 2020 10:32

karknu marked this pull request as ready for review March 19, 2020 10:32

mrBliss reviewed Mar 19, 2020

View reviewed changes

ouroboros-consensus/src/Ouroboros/Consensus/NodeNetwork.hs Outdated Show resolved Hide resolved

karknu force-pushed the karknu/moar_timeouts branch from 51c77cd to 87b14bb Compare March 19, 2020 13:56

karknu requested a review from njd42 March 19, 2020 15:25

coot requested changes Mar 20, 2020

View reviewed changes

karknu mentioned this pull request Mar 20, 2020

Add per state size limits in Ouroboros-Network #1727

Closed

karknu force-pushed the karknu/moar_timeouts branch from 87b14bb to e3cab58 Compare March 20, 2020 14:08

karknu requested a review from coot March 20, 2020 14:11

coot approved these changes Mar 23, 2020

View reviewed changes

ouroboros-network-framework/src/Ouroboros/Network/Snocket.hs Show resolved Hide resolved

dcoutts approved these changes Mar 23, 2020

View reviewed changes

karknu force-pushed the karknu/moar_timeouts branch 2 times, most recently from 6393b47 to a3fb47d Compare March 24, 2020 07:08

iohk-bors bot added a commit that referenced this pull request Mar 24, 2020

Merge #1813

d46b045

1813: Protocol timeouts r=karknu a=karknu Implements #1395, apart from the multi-chunk message timeout. Co-authored-by: Karl Knutsson <[email protected]>

karknu force-pushed the karknu/moar_timeouts branch from a3fb47d to 4d72207 Compare March 24, 2020 13:08

karknu added 4 commits March 24, 2020 14:15

Protocol limits for NodeToNode protocols

41caba7

Use timeouts for NodeToNode protocols

ac233a7

Optional timeout for reading Mux SDUs.

ddb840d

Implement an optional timeout for reading Mux SDUs. This adds a 30s timeout when reading SDUs over a socketSnocket (IPv4/IPv6). There is no timeout for reading messages over localSnockets.

Test case for ProtocolTimeLimits

e0445d0

karknu force-pushed the karknu/moar_timeouts branch from 4d72207 to e0445d0 Compare March 24, 2020 13:22

iohk-bors bot merged commit ea9aae7 into master Mar 24, 2020

iohk-bors bot deleted the karknu/moar_timeouts branch March 24, 2020 14:41

dcoutts mentioned this pull request Mar 25, 2020

Assign timeouts to various protocols of Ouroboros-Network #1427

Closed

		stateToLimit (ClientAgency (TokTxIds TokBlocking)) = largeByteLimit
		stateToLimit (ClientAgency (TokTxIds TokNonBlocking)) = largeByteLimit

Protocol timeouts #1813

Protocol timeouts #1813

Uh oh!

Conversation

karknu commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coot left a comment

Choose a reason for hiding this comment

Uh oh!

coot Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

coot Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

dcoutts Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coot Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coot Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karknu commented Mar 20, 2020

Uh oh!

coot commented Mar 20, 2020

Uh oh!

coot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dcoutts left a comment

Choose a reason for hiding this comment

Uh oh!

dcoutts Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

dcoutts Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karknu commented Mar 24, 2020

Uh oh!

karknu commented Mar 24, 2020

Uh oh!

iohk-bors bot commented Mar 24, 2020

Canceled

Uh oh!

dcoutts commented Mar 24, 2020

Uh oh!

iohk-bors bot commented Mar 24, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karknu commented Mar 16, 2020 •

edited

Loading