Port #2525 and #2531 to master #2548
Conversation
mrBliss
force-pushed
the
mrBliss/port-2525-2531-to-master
branch
from
9b4ea28
to
bf98f28
Compare
|
@coot @dcoutts @nfrisby Can somebody approve/review this PR? It ports two already reviewed PRs to master. However, because the branch in which they were merged had build errors in CI, we never detected a bug in the original PR, which @nfrisby and I fixed. @coot This means your |
Fixes #2499 for the ChainSyncClient.
Now a directed edge in the topology (ie a bundle of mini-protocol instances) restarts for two reasons. 1) The ChainSync client and/or server terminated *without* an exception. It restarts at the next slot onset. 2) The node was scheduled to restart at this slot onset. It restarts immediately.
This allows us to later easily add arguments to an `App`, e.g., `NodeToClientVersion` or `ControlMessageSTM m`.
The control message isn't used yet.
mrBliss
force-pushed
the
mrBliss/port-2525-2531-to-master
branch
from
bf98f28
to
8be435e
Compare
| -- | Applications for the node-to-node protocols | ||
| -- | ||
| -- See 'Network.Mux.Types.MuxApplication' | ||
| data Apps m peer bCS bBF bTX bKA a = Apps { | ||
| -- | Start a chain sync client that communicates with the given upstream | ||
| -- node. | ||
| aChainSyncClient :: NodeToNodeVersion -> peer -> Channel m bCS -> m (a, Maybe bCS) | ||
| aChainSyncClient :: App m peer bCS a |
There was a problem hiding this comment.
This means we'll not see the actual return value of the client, even though all the upstream code changes now make it available. (The a is shared by the other protocols, and none of those return anything. So it's ()/Void/void/etc or at the very least a bit misleading.) The CS return value is recorded in a tracer invoked within the client, so it'll at least be logged, which seems like the primary use-case, though.
Just an observation.
There was a problem hiding this comment.
Yeah, that's indeed not so nice. That Apps business is actually written by the network team. Unfortunately it lives in the consensus layer 😬.
|
bors merge |
#2525 and #2531 were merged in the https://github.com/input-output-hk/ouroboros-network/tree/coot/connection-manager. However, that branch won't be merged for another few weeks. In the meantime, the merge conflicts with the changes made in those two PRs will accumulate. For example, I know that #2546 will cause a merge conflict.
Now that @coot has kindly merged #2541 (which those PRs depended on) in master, we can port #2525 and #2531 to master so we can avoid the upcoming merge conflicts. After this PR is merged, the
coot/connection-managerbranch should be rebased onto master.