Copy Edit and Requirements Restructuring by toadlyBroodle · Pull Request #267 · lightning/bolts

toadlyBroodle · 2017-11-05T02:27:46Z

While reading through first few sections, I noticed a number of grammar, punctuation, and style issues. Also, I found it rather difficult to comprehend the requirements sections due to their lack of structure. So thought I'd take a stab at fixing these things. So far, I've gone through sections 00, 01, and 02 copy-editing for correctness, clarity, conciseness, and consistency.

If this is desirable, I can continue on with the remaining sections?

Thanks,
Landon

…tyle Edit 00-introduction copy for clarity (minor rephrasing, punctuation), correctness (grammar, capitalization, punctuation), consision (minimizing wordiness, redundancy), and consistency (document style, e.g. 1 space between sentences, capitalization of headers, etc.)

Edit 01-messaging copy for clarity (minor rephrasing, punctuation), correctness (grammar, capitalization, punctuation), consision (minimizing wordiness, redundancy), and consistency (document style, e.g. 1 space between sentences, capitalization of headers, etc.)

Requirements were difficult to follow in existing sentence form, so I reordered them into hopefully more intuitive groups of unordered lists.

…ches.

rustyrussell

Minor comments, I think the last one is the only significant change.

Thanks, though, this is great!

rustyrussell · 2017-11-08T02:08:35Z

-This semantic allows future incompatible changes, or backward
-compatible changes.  Bits should generally be assigned in pairs, so
+This semantic allows future incompatible changes and/or backward
+compatible changes. Bits should generally be assigned in pairs, so


How about:

This semantic allows both future incompatible changes and future backward compatible changes.

ie. odd bits->cool if you don't understand, even bits->error if you don't understand.

Sounds good.

rustyrussell · 2017-11-08T02:16:02Z

+   - MUST ignore the received message.
+ - otherwise type is even
+   - MUST fail the channels.



I prefer to separate sending and receiving requirements. As an implementer, you implement one of them at a time.

So, when implementing "receive_foo" you look through the "A node receiving foo..." (and, for c-lightning, we include and check quotes from the spec, making it especially powerful).

Alright, I can go through and group requirements according to sender/receiver for sure.

@rustyrussell

Implement changes requested by @rustyrussell: wording change, structure requirements according to whether nodes are sending or receiving; Correct grammar, punctuation, capitalization, and style for correctness, concision, clarity, comprehension, and consistency;

@rustyrussell

…grammar Implement changes requested by @rustyrussell: wording change, structure requirements according to whether nodes are sending or receiving; Correct grammar, punctuation, capitalization, and style for correctness, concision, clarity, comprehension, and consistency;

…htning-rfc

…ency

…bs markdown

toadlyBroodle · 2017-11-10T08:24:19Z

Ok, I should be pretty much done going over BOLT 0 and BOLT 1 now. I've included a temporary stylesheet/checklist that I've been using for copy editing. Thought others may find it useful for reference.
For the remaining BOLTs, should I keep adding commits here, or start new pull requests?

Done first pass copy edit, up to line 279 of BOLT 2, according to .copy-edit-stylesheet-checklist.md guidelines;

Done first pass copy edit, up to line 576 of BOLT 2, according to .copy-edit-stylesheet-checklist.md guidelines;

fix stylesheet formatting; add passive voice item to stylesheet checklist; apply passive voice change to BOLT 1;

add stylesheet item: prefer typed, not written numbers; updated BOLT 0, 1 to reflect change; copy edit BOLT 2 up to line 674;

BOLT 1: minor list formatting fix; BOLT 2: copy edit up to line 955;

complete first pass copy edit, following .copy-edit-stylesheed-checklist guidelines;

rustyrussell

Wow. This is so awesome.

Your cleanups made me realize a couple of flawed requirements (ie. "MUST check", but I'm happy to patch those myself after this is merged) so that this is a pure "reformat and tidy" patchset.

That leaves only a couple of minor things I found. If you want to resolve those, I can rebase and apply!

rustyrussell · 2017-11-15T01:49:13Z

+            - SHOULD do this.
+          - otherwise:
+            - MUST do this,
+              - and MUST NOT do this.


Shouldn't "- and MUST NOT do this" be indented less, to line up with the above? I think you only want more indent after a "if" or "otherwise".

I see you used this with a "but MUST NOT" in BOLT 1. See comments there.

ok I've simplified the structure to reflect this.

rustyrussell · 2017-11-15T01:57:25Z

+    - consistent use of _emphasis_, **strong**, `code`, CAPS, 'quotes'
+    - single line separators between paragraphs and page elements
+    - type numbers (e.g. '1', '1s') rather than write them (e.g. 'one', 'ones')
+      - exception: 'non-zero'


Interesting, I was taught small numbers are as words, large or enumerated as digits. So I would say "between two peers" but "version 0". But this is simpler, I guess.

Yes, exactly right, generally small numbers are written out, however, it's largely a stylistic choice depending upon the publication. My thought was due to the many instances of written mathematical statements, this would help to avoid any potential confusion (e.g. "the old one must be equal to 1"). But this does look funny in a non-mathematical sentence.

So how about this: type references to actual digits (e.g. "the number 1"), otherwise write out small amounts (e.g. "there is one instance of the number 2", "there are two number 1s").

rustyrussell · 2017-11-15T02:07:05Z

+  - MUST truncate `len` to the remainder of the packet (if it's larger).
+  - if the string is composed solely of printable ASCII characters (For reference: the printable character set includes byte values 32 through 127, inclusive):
+    - SHOULD only print out `data` verbatim.



This final case has been inverted (though the original was confusing):

if data is not composed solely of printable ASCII characters (For reference: the printable character set includes byte values 32 through 127, inclusive):

SHOULD NOT print out data verbatim.

rustyrussell · 2017-11-15T02:10:07Z

+    - SHOULD include the raw, hex-encoded transaction in reply to a `funding_created`, `funding_signed`, `closing_signed`, or `commitment_signed` message.
+  - when `channel_id` is 0:
+    - MUST fail all channels,
+      - and MUST close the connection.


Could unindent and remove the "and "?

rustyrussell · 2017-11-15T02:12:17Z

+  - SHOULD set `ignored` to 0s,
+    - but MUST NOT set `ignored` to sensitive data such as secrets or portions of initialized
+ memory.
+


Ah, I see how you used an extra indent level here to join them. This could go either way; treat them as independent requirements, or, as here, structurally note that the second is a specialization of the first.

I think you're right, it's simpler to remove the extra indent altogether, except for true sub-items.

rustyrussell · 2017-11-15T02:54:11Z

+A receiving node:
+  - MUST check that `id` corresponds to an HTLC in its current commitment transaction,
+    - and MUST fail the channel if it does not.
+  - MUST check that the `payment_preimage` value in `update_fulfill_htlc`


Again, that's my bad "MUST CHECK" pattern :(

if the id does not correspond to an HTLC in its current commitment transaction:

MUST fail the channel

rustyrussell · 2017-11-15T02:54:46Z

+    - and MUST fail the channel if it does not.
+  - MUST check that the `payment_preimage` value in `update_fulfill_htlc`
+  SHA256 hashes to the corresponding HTLC `payment_hash`,
+    - and MUST fail the channel if it does not.


Again with the MUST check... checking isn't actionable.

rustyrussell · 2017-11-15T02:55:53Z

+    - MUST fail the channel.
+  - MAY check the `sha256_of_onion` in `update_fail_malformed_htlc`,
+    - and if it does not match the onion it sent:
+      - MAY retry or choose an alternate error response.


Again, me with the "must check" nonsense :(

rustyrussell · 2017-11-15T02:57:36Z

-A node which sends `update_fulfill_htlc` before the sender is also
-committed to the HTLC risks losing funds.
+A node which sends `update_fulfill_htlc`, before the sender, is also
+committed to the HTLC and risks losing funds.


"A node that" perhaps here too?

rustyrussell · 2017-11-15T03:04:52Z

-previous commitment transaction, MUST set `next_per_commitment_point` to the values for its next commitment transaction.
+A receiving node:
+  - MUST check that `per_commitment_secret` generates the previous `per_commitment_point`,
+    - and MUST fail the channel if it does not.


Again with the MUST check...

update stylesheet to reflect appropriate structure of Requirement sub-items and format of digits, numerations, and quantities; copy edit BOLTs 0,1,2 to adhere to these changes;

rustyrussell · 2017-11-16T23:54:33Z

FWIW, in future, merges are best done right at the end (in fact, I prefer rebasing).

I'm going to merge this now, simply because it's going to keep getting harder, and I'm going to do so without cleaning up the commits, because otherwise it'd really become almost one big commit.

toadlyBroodle · 2017-11-17T01:27:26Z

Ok, duly noted. Thanks for merging :D

toadlyBroodle added 4 commits November 4, 2017 09:32

Organize 01-messaging requirements into unordered lists

5450ec1

Requirements were difficult to follow in existing sentence form, so I reordered them into hopefully more intuitive groups of unordered lists.

Further clarify 01-messaging requirement structures by deepening bran…

531317b

…ches.

rustyrussell requested changes Nov 8, 2017

View reviewed changes

toadlyBroodle added 9 commits November 9, 2017 13:30

minor grammar, punctuation, and capitalization fixes;

2f6a629

Merge amended branch 'master' of https://github.com/toadlyBroodle/lig…

076ec8a

…htning-rfc

fix 00-introduction, 01-messaging section spacings for format consist…

c3ef4ca

…ency

fix 01-messaging unordered list spacing so formats correctly in githu…

919df99

…bs markdown

fix 01-messaging missed unordered list spacing

20fca8b

BOLT 1: change HTML tags to markdown

e885a63

add copy editing stylesheet temporarily while drafting, for reference;

b5d2d8b

toadlyBroodle added 12 commits November 10, 2017 00:56

BOLT 2: first pass copy edit;

93afa0d

Done first pass copy edit, up to line 279 of BOLT 2, according to .copy-edit-stylesheet-checklist.md guidelines;

BOLT 2: first pass copy edit continued;

1afc143

Done first pass copy edit, up to line 576 of BOLT 2, according to .copy-edit-stylesheet-checklist.md guidelines;

update stylesheet checklist

4e70a8b

fix stylesheet formatting; add passive voice item to stylesheet checklist; apply passive voice change to BOLT 1;

BOLT 0, 1, 2: copy edit

b475639

add stylesheet item: prefer typed, not written numbers; updated BOLT 0, 1 to reflect change; copy edit BOLT 2 up to line 674;

Merge branch 'master' into copy-edit-bolts

441da24

BOLT 1, 2: copy edit

03197f2

BOLT 1: minor list formatting fix; BOLT 2: copy edit up to line 955;

BOLT 2: first pass copy edit done;

adb13bf

complete first pass copy edit, following .copy-edit-stylesheed-checklist guidelines;

update stylesheet checklist

074b62b

amend: update stylesheet checklist

43f4a7e

Merge branch 'master' into master

3d67c79

BOLT 2: copy edit merged changes

d99a410

make spellcheck.sh happy

24896d8

rustyrussell requested changes Nov 15, 2017

View reviewed changes

toadlyBroodle added 2 commits November 15, 2017 02:55

BOLT 0,1,2: fix requested changes;

c89a54a

update stylesheet to reflect appropriate structure of Requirement sub-items and format of digits, numerations, and quantities; copy edit BOLTs 0,1,2 to adhere to these changes;

Merge branch 'master' into master

04301ad

Merge branch 'master' into master

ba4601b

rustyrussell approved these changes Nov 16, 2017

View reviewed changes

rustyrussell merged commit fd560bb into lightning:master Nov 17, 2017

Conversation

toadlyBroodle commented Nov 5, 2017

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toadlyBroodle commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toadlyBroodle Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toadlyBroodle Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Nov 16, 2017

Uh oh!

toadlyBroodle commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

toadlyBroodle commented Nov 10, 2017 •

edited

Loading

toadlyBroodle Nov 15, 2017 •

edited

Loading

toadlyBroodle Nov 15, 2017 •

edited

Loading