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

Validate connection manager/inbound governor transition order #3360

Merged

bolt12 merged 6 commits into p2p-master from bolt12/p2p-master-validate-transition-order

Nov 2, 2021

Contributor

bolt12 commented Sep 20, 2021

No description provided.

This was linked to issues Sep 20, 2021

Validate order of connection-manager's transitions #3358

Closed

Validate order of inbound-governor's transitions #3359

Closed

coot force-pushed the p2p-master branch from 8a409f3 to 121ddfa Compare

September 22, 2021 07:57

bolt12 linked an issue

that may be closed by this pull request

Make sure transition tracings are well covered. #3367

Closed

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch from b68bb6d to ef2d1fd Compare

September 23, 2021 15:06

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch from 34c7cdc to de9a060 Compare

October 1, 2021 11:30

bolt12 marked this pull request as ready for review

October 1, 2021 11:37

bolt12 requested review from coot, karknu and njd42 as code owners

October 1, 2021 11:37

bolt12 removed the request for review from njd42

October 1, 2021 11:38

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch 3 times, most recently from 47c06ba to 1add932 Compare

October 4, 2021 10:24

coot reviewed

View reviewed changes

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch 2 times, most recently from d8c306a to 15dd65b Compare

October 6, 2021 08:19

bolt12 requested a review from coot

October 6, 2021 08:19

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch 7 times, most recently from 5be3bbc to 4d0cca7 Compare

October 8, 2021 12:02

bolt12 mentioned this pull request

Pruning tests #3309

Merged

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch 4 times, most recently from 48b2528 to 03d9534 Compare

October 12, 2021 13:05

bolt12 removed request for EncodePanda and nfrisby

October 21, 2021 09:29

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch from 634ead5 to fb4eb98 Compare

October 21, 2021 11:54

bolt12 requested a review from coot

October 21, 2021 14:12

coot reviewed

View reviewed changes

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

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

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

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

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

bolt12 linked an issue

that may be closed by this pull request

Flawed accept/connect interaction in Snocket. #3449

Closed

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch 2 times, most recently from 41d299f to 21b9799 Compare

October 25, 2021 10:27

coot force-pushed the p2p-master branch from 81052c0 to 2f311e7 Compare

October 26, 2021 05:58

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch from 21b9799 to 29dcdfe Compare

October 26, 2021 07:39

coot reviewed

View reviewed changes

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

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

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

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

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

coot reviewed

View reviewed changes

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

coot requested changes

View reviewed changes

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

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Outdated Show resolved Hide resolved

ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs Show resolved Hide resolved

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch from 29dcdfe to 26ad1f6 Compare

October 27, 2021 13:53

coot force-pushed the p2p-master branch from bc9a9a5 to cec7933 Compare

October 29, 2021 09:44

coot reviewed

View reviewed changes

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved


Refactored Server2 multinode_Sim tests

7974cd8

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch 3 times, most recently from 6ecf01c to e6c69cc Compare

October 29, 2021 12:18

bolt12 requested a review from coot

October 29, 2021 12:19

coot approved these changes

View reviewed changes

Contributor

coot left a comment

LGTM; I left a minor comment (bang in modifyTMVarSTM), but this can be left for later as well (in that case create a github issue for it).

ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs Outdated Show resolved Hide resolved

bolt12 added 5 commits

November 2, 2021 14:48


Add test to check order of ConnectionManager trans

71dc491

- In unregisterInboundConnectionImpl: OutboundDupState Ticking -> OutboundDupState Expired
- In withConnectionManager cleanup, where we put the state in TerminatedState is missing a trace. However, since this can be async the connState can come out of order and/or not making much sense.
- In requestOutboundConnectionImpl bracketOnError: TerminatedState -> Unknown
- In includeInboundConnectionImpl: on readPromise Left handleError case, where we put the state in either TerminatingState or TerminatedState
- In unregisterInboundConnectionImpl: Only trace TerminatingState -> TerminatedState, after having written to the connVar
- In a possible race between withConnectionManager finally block and forkConnectionHandler cleanup function, where the latter can run first and break an assumption made by withConnectionManager that it is the first to run so a connection shouldn't be in TerminatingState (which is not the case, since there is a race condition). So an out of order and possibly not making much sense transition is logged.
- When accept call returns first than connect and the connVar gets overwritten. In requestOutboundConnectionImpl cleanup function we wrongly trace * -> TerminatedState transition of a removed connVar, we shouldn't care about that connVar anymore.
- In promotedToWarmRemoteImpl, in OutboundIdleState case we are not updating the state correctly.


Reliable tracing with Async exceptions

2e81823

In promotedToWarmRemoteImpl (and family), it is possible that the STM action successfully commits, writing a new state to the connVar, but right after an AsyncCancelled is received making us omit the respective transition tracing.


Added InboundGovernor transition order test

9c7d65b

Fixed issue in inboundGovernor function where in case of Async
exceptions we wouldn't log the final transitions for the connections.


No invalid trace test and monadic assertion error

38fb021

- This way an assertion is not a pure exception evaluated when printing
an `io-sim` test case, but a simulation error. Note that this will hide
such exceptions from simulation. To test that such assertion do not
happen we can analyse trace.

- Check that `TrUnexpectedlyMissingConnectionState` is not logged.
This is an initial part of #3465.

Authors: Marcin Szamotulski


Fix Snocket bug in the accept/connect

94c4b30

Basically, if I connect to someone and someone connects to me, before the connect returns (and before the remote accept returns as well) the local accept can return first masking itself as the remote one because we have no way to distinguish directions.

bolt12 force-pushed the bolt12/p2p-master-validate-transition-order branch from e6c69cc to 94c4b30 Compare

November 2, 2021 14:49

bolt12 merged commit f10648c into p2p-master

iohk-bors bot deleted the bolt12/p2p-master-validate-transition-order branch

November 2, 2021 16:55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet