Fix typos and inaccuracies in comments of the ChainSync client #233

facundominguez · 2023-07-14T15:56:46Z

This PR only modifies comments.

amesgen

Thanks, only a minor comment below ❤️

amesgen · 2023-07-14T17:46:35Z

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs

@@ -171,7 +171,7 @@ bracketChainSyncClient tracer ChainDbView { getIsInvalidBlock } varCandidates
 -- We must keep the candidate chain synchronised with the corresponding
 -- upstream chain. The upstream node's chain might roll forward or
 -- backwards, and they will inform us about this. When we get these
-- messages, we will replicate these actions on our candidate chain.
+-- messages, we will replicate these actions on our current chain.


I think this is already an improvement; but maybe using "our fragment" is even slightly better as it is called ourFrag in the code and "current chain" often refers to our selection in other places (i.e. the method of the ChainDB to get the current selection is called getCurrentChain).

Adopted in a38ce67.

amesgen · 2023-07-14T17:47:12Z

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs

+            -- The @intersection@ is not on our current chain, even though
+            -- we sent only points from our current chain to find an


Same point about "our current chain" -> "our fragment" as above.

Adopted in a38ce67.

nfrisby

Thank you for opening this!

I'm Requesting Changes because I think "our fragment" is correct in one of the places you wrote it but incorrect in the other.

nfrisby · 2023-07-17T21:53:53Z

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs

@@ -171,7 +171,7 @@ bracketChainSyncClient tracer ChainDbView { getIsInvalidBlock } varCandidates
 -- We must keep the candidate chain synchronised with the corresponding
 -- upstream chain. The upstream node's chain might roll forward or
 -- backwards, and they will inform us about this. When we get these
-- messages, we will replicate these actions on our candidate chain.
+-- messages, we will replicate these actions on our fragment.


The phrase "candidate chain" features prominently in the preceding paragraphs of this comment (starting with "Our task:"), so it is somewhat discontinuous to change only this occurrence.

Would you rather change every occurrence of "candidate chain" to "candidate fragment"?

Adopted in 93395a3.

I'm sorry this didn't occur to me earlier, but: there are plenty of other occurrences of "candidate chain" in the rest of the comments in this source file.

Ultimately this is because engineers on this project eventually realize that "fragment" and "chain" are essentially interchangeable. It seems that whoever originally wrote this module was favoring "chain" at the time. Some other comments do already say "candidate fragment". I would guess that---if anything---it's most correlated to how broad the context of the comment is: "chains" in the broad cases, "fragment" in the very specific cases.

Usually, I wouldn't object to your PR as-is now: you've ensured the wording is at least coherent within its local context.

However, in this case, that local context happens to effectively be the binding site of "candidate chain" as it occurs throughout the rest of the module.

However however, the module was already using both "candidate chains" and "candidate fragment"... and it doesn't really matter which is used where. The only possible downside to your PR is that it inverts my hypothesized correlation between scope and chain-versus-fragment word choice 🤷.

Thanks for paying attention @nfrisby; in my initial review, I somehow thought that the context was the same for both changes in 982fc1c. I agree that the choice of "candidate chain" vs "candidate fragment" doesn't matter that much.

I am both fine with reverting the change in this block comment, or keeping it, in that case probably adding "candidate" again

Suggested change

-- messages, we will replicate these actions on our fragment.

-- messages, we will replicate these actions on the candidate fragment.

to make it clear that this is in fact not referring to the ourFrag variable.

@facundominguez wrote:

I'm finding useful to talk about "candidate fragment" when referring to the local anchored fragment, and "candidate chain" to refer to the "upstream chain". My last commit 26c999a makes this story consistent in Client.hs.

To make this even more obvious, we could rename "candidate chain" to "upstream chain" if you want.

Update: I think considering "candidate fragment" and "candidate chain" interchangeable would really hide whether the text is referring to the upstream chain or to "their fragment" which is locally kept.

I haven't fully processed this. But my first knee-jerk reaction is that it seems while to point out: from a practical perspective, the upstream chain is almost always irrelevant to the code, simply because we have no way of observing it. The only thing we can observe is the candidate fragment, ie the prefix of the upstream chain that they have sent so far. The only knowledge we have about the upstream chain is that it is some (perhaps non-proper) extension of the candidate fragment. (Exception: they also send the tip point annotation and MsgAwaitReply, but those merely determine whether it's a proper or non-proper extension.)

the upstream chain is almost always irrelevant to the code, simply because we have no way of observing it.

In that case, I invite you to check the remaining references to "candidate chain". I think they all refer to the upstream chain despite of lacking a way to observe it.

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs

nfrisby

As I explain verbosely below: I think this is unblocked, now that the changes are coherent in their local context.

HOWEVER, if you're interested, there are other occurrences of "candidate chain" in this module. Feel free to hunt them down as well. But please do read my longer comment first, so that you know what swamp you're wading into.

I'm not going to Approve, because I want someone else's opinion first. (Perhaps even just yours.)

nfrisby · 2023-07-17T22:48:03Z

...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ChainSync/Client.hs

@@ -171,7 +171,7 @@ bracketChainSyncClient tracer ChainDbView { getIsInvalidBlock } varCandidates
 -- We must keep the candidate chain synchronised with the corresponding
 -- upstream chain. The upstream node's chain might roll forward or
 -- backwards, and they will inform us about this. When we get these
-- messages, we will replicate these actions on our candidate chain.
+-- messages, we will replicate these actions on our fragment.


I'm sorry this didn't occur to me earlier, but: there are plenty of other occurrences of "candidate chain" in the rest of the comments in this source file.

Ultimately this is because engineers on this project eventually realize that "fragment" and "chain" are essentially interchangeable. It seems that whoever originally wrote this module was favoring "chain" at the time. Some other comments do already say "candidate fragment". I would guess that---if anything---it's most correlated to how broad the context of the comment is: "chains" in the broad cases, "fragment" in the very specific cases.

Usually, I wouldn't object to your PR as-is now: you've ensured the wording is at least coherent within its local context.

However, in this case, that local context happens to effectively be the binding site of "candidate chain" as it occurs throughout the rest of the module.

However however, the module was already using both "candidate chains" and "candidate fragment"... and it doesn't really matter which is used where. The only possible downside to your PR is that it inverts my hypothesized correlation between scope and chain-versus-fragment word choice 🤷.

98% addressed

facundominguez · 2023-07-19T13:37:00Z

I'm finding useful to talk about "candidate fragment" when referring to the local anchored fragment, and "candidate chain" to refer to the "upstream chain". My last commit 26c999a makes this story consistent in Client.hs.

To make this even more obvious, we could rename "candidate chain" to "upstream chain" if you want.

Update: I think considering "candidate fragment" and "candidate chain" interchangeable would really hide whether the text is referring to the upstream chain or to "their fragment" which is locally kept.

…nSync client

…nt.hs Co-authored-by: amesgen <amesgen@amesgen.de>

facundominguez · 2023-07-20T19:27:47Z

I'm merging given that the approvals stood for a while and we all seem to agree that the changes are better resting in main than here. But @nfrisby, let's continue the conversation on the need to refer to the upstream chain in comments.

facundominguez requested review from nfrisby and amesgen July 14, 2023 15:56

facundominguez requested a review from a team as a code owner July 14, 2023 15:56

amesgen approved these changes Jul 14, 2023

View reviewed changes

nfrisby previously requested changes Jul 17, 2023

View reviewed changes

nfrisby reviewed Jul 17, 2023

View reviewed changes

facundominguez enabled auto-merge July 20, 2023 19:24

facundominguez and others added 7 commits July 20, 2023 16:24

Fix comment in chainSyncClient

3ce981f

Remove potentially confusing uses of candidate

a02f009

Fix typos in comments

b34ba27

In comments: our current chain -> our fragment

1de59e8

Use fragment instead of chain in the strategy description of the Chai…

831ecc9

…nSync client

Make comment of intersectFound more precise

f791e3a

Fix incorrect reference to our fragment in comments of ChainSync/Clie…

3f45b95

…nt.hs Co-authored-by: amesgen <amesgen@amesgen.de>

facundominguez force-pushed the fd/comment-fixes-chain-sync branch from ff8ea73 to 3f45b95 Compare July 20, 2023 19:24

facundominguez added this pull request to the merge queue Jul 21, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 21, 2023

facundominguez added this pull request to the merge queue Jul 21, 2023

Merged via the queue into main with commit 08b37e1 Jul 21, 2023

facundominguez deleted the fd/comment-fixes-chain-sync branch July 21, 2023 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typos and inaccuracies in comments of the ChainSync client #233

Fix typos and inaccuracies in comments of the ChainSync client #233

facundominguez commented Jul 14, 2023 •

edited

Loading

amesgen left a comment

amesgen Jul 14, 2023

facundominguez Jul 14, 2023

amesgen Jul 14, 2023

facundominguez Jul 14, 2023

nfrisby left a comment

nfrisby Jul 17, 2023

facundominguez Jul 17, 2023

nfrisby Jul 17, 2023

amesgen Jul 18, 2023

nfrisby Jul 20, 2023

facundominguez Jul 20, 2023

nfrisby left a comment

nfrisby Jul 17, 2023

facundominguez commented Jul 19, 2023 •

edited

Loading

facundominguez commented Jul 20, 2023

		-- The @intersection@ is not on our current chain, even though
		-- we sent only points from our current chain to find an

	-- messages, we will replicate these actions on our fragment.
	-- messages, we will replicate these actions on the candidate fragment.

Fix typos and inaccuracies in comments of the ChainSync client #233

Fix typos and inaccuracies in comments of the ChainSync client #233

Conversation

facundominguez commented Jul 14, 2023 • edited Loading

amesgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfrisby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfrisby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundominguez commented Jul 19, 2023 • edited Loading

facundominguez commented Jul 20, 2023

facundominguez commented Jul 14, 2023 •

edited

Loading

facundominguez commented Jul 19, 2023 •

edited

Loading