Skip to content

Conversation

@morehouse
Copy link
Owner

No description provided.

@morehouse
Copy link
Owner Author

@Arvin21M: ready for review!

Copy link

@Arvin21M Arvin21M left a comment

Choose a reason for hiding this comment

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

Thanks for your work to identify this Matt, and a great write up as well. Totally understandable from a "non-developer" pleb perspective, as well as a technical dive for those inclined.

Other than the one hyperlink issue listed below, the majority of the suggested/requested changes are really more like "Nice to Haves", so I didnt want to mark the change requests directly on the md file view this time in case you just wanted to skip right over them quickly. Let me know if you prefer I do that in the md file for the next review.

Title suggestion:
Susceptible to DoS Attack: Channel Open Race in CLN

Line 50 - 58:
Tell the reader what the following 9 bullet points are outlining. Similar to what you have done in the two previous sections about "The Peer Connection Flow" and "The Channel Open Flow" by saying "..the following..." and explaining briefly what the bullet points are in reference to.

Line 16, 32, 43, 60, 70, 108, 118:
Capitalize these H2 and H3 tags as if they were H1 Titles.
(ie: Line 118 "Do Stress Testing" instead of "Do stress testing")

Hyperlinks:
Under the The DoS Attack section, the one hyperlink "fake channel DoS attack" leads to a 404 Page Not Found on GitHub. It currently uses the URL https://github.com/morehouse/morehouse.github.io/blob/cln_channel_open_race/lightning/fake-channel-dos

The rest of the links appear to be valid and pointing to its intended landing/target page.

In Closing:
If justified and/or necessary, perhaps a "Takeaway" summary at the very end with 1-3 bullet points or something like that would be beneficial.

@morehouse morehouse force-pushed the cln_channel_open_race branch from 409a07d to c8cf15c Compare January 8, 2024 18:31
@morehouse
Copy link
Owner Author

Thanks for the review, Arvin!

Thanks for your work to identify this Matt, and a great write up as well. Totally understandable from a "non-developer" pleb perspective, as well as a technical dive for those inclined.

Other than the one hyperlink issue listed below, the majority of the suggested/requested changes are really more like "Nice to Haves", so I didnt want to mark the change requests directly on the md file view this time in case you just wanted to skip right over them quickly. Let me know if you prefer I do that in the md file for the next review.

Either way is fine with me.

Title suggestion: Susceptible to DoS Attack: Channel Open Race in CLN

I think I prefer the current title since it is shorter and fits on one line.

Line 50 - 58: Tell the reader what the following 9 bullet points are outlining. Similar to what you have done in the two previous sections about "The Peer Connection Flow" and "The Channel Open Flow" by saying "..the following..." and explaining briefly what the bullet points are in reference to.

Done.

Line 16, 32, 43, 60, 70, 108, 118: Capitalize these H2 and H3 tags as if they were H1 Titles. (ie: Line 118 "Do Stress Testing" instead of "Do stress testing")

Done.

Hyperlinks: Under the The DoS Attack section, the one hyperlink "fake channel DoS attack" leads to a 404 Page Not Found on GitHub. It currently uses the URL https://github.com/morehouse/morehouse.github.io/blob/cln_channel_open_race/lightning/fake-channel-dos

Yeah, it's a relative link /lightning/fake-channel-dos/ that should fix itself once the real webpage is generated. I'll double-check after merging to make sure.

The rest of the links appear to be valid and pointing to its intended landing/target page.

In Closing: If justified and/or necessary, perhaps a "Takeaway" summary at the very end with 1-3 bullet points or something like that would be beneficial.

I was kind of going for that with the points under Prevention. I'm concerned it would be too repetitive if I added another section that said something like this:

# Takeaways

- Avoid race conditions
- Use regression and stress testing

What do you think?

@Arvin21M
Copy link

Arvin21M commented Jan 8, 2024

Either way is fine with me.

Okay! Will mark file or not mark file accordingly next time.

I think I prefer the current title since it is shorter and fits on one line.

I agree, it will look cleaner. I was trying to me more "provocative" with that edit.

Yeah, it's a relative link /lightning/fake-channel-dos/ that should fix itself once the real webpage is generated. I'll double-check after merging to make sure.

Noted!

I was kind of going for that with the points under Prevention. I'm concerned it would be too repetitive if I added another section that said something like this:

Takeaways

  • Avoid race conditions
  • Use regression and stress testing

What do you think?

Yes! Those two takeaways are great. 🚀

@morehouse morehouse force-pushed the cln_channel_open_race branch from c8cf15c to c0a2e81 Compare January 8, 2024 18:55
@morehouse
Copy link
Owner Author

Thanks, Arvin.

I added a short takeaways section at the end. Will merge now and check the rendered page.

@morehouse morehouse merged commit ff80451 into main Jan 8, 2024
@morehouse morehouse deleted the cln_channel_open_race branch January 8, 2024 19:04
morehouse added a commit that referenced this pull request Dec 4, 2025
morehouse added a commit that referenced this pull request Dec 4, 2025
morehouse added a commit that referenced this pull request Dec 4, 2025
morehouse added a commit that referenced this pull request Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants