Improve documentation on the `all` module by tcharding · Pull Request #1339 · rust-bitcoin/rust-bitcoin

tcharding · 2022-10-24T22:04:18Z

Improve documentation on the all module by doing:

Document guarantee that all will only ever contain opcode constants
Fix stale/incorrect code comment

Done as follow up to #1295

Kixunil

It's not wrong but if we're explicitly guaranteeing things we should also guarantee beginning with OP_

Kixunil · 2022-10-24T22:40:50Z

bitcoin/src/blockdata/opcodes.rs

And all opcode constants are guaranteed to begin with OP_.

Used as suggested.

Kixunil · 2022-10-24T22:43:02Z

bitcoin/src/blockdata/opcodes.rs

I wonder what's the reason though.

No idea, @apoelstra did you write this originally?

The reason for not having Ord is that there is no "ordering" of opcodes that makes conceptual sense, and I can't think of a reason to put sets of individual opcodes into ordered data structures (or sorting them directly).

The reason I mentioned this is because Bitcoin Core routinely uses < and > on opcodes to effect consensus logic, something which hides implicit assumptions about the opcodes' byte encoding, in code where the exact behavior is critical, and dresses this up in a skinsuit of mathematical notation.

Probably nobody else is aware of, or as offended by, this particular quirk of Core as I was, so we can drop the comment :). Also like, I'm pretty sure we have the same logic, I just had to add some as u8s before the <s.

Also like, I'm pretty sure we have the same logic, I just had to add some as u8s before the <s

Made me laugh :)

tcharding · 2022-10-25T21:59:14Z

Removed code comment as suggested.

apoelstra · 2022-10-25T22:01:58Z

CI failure is in master. I will PR to fix it.

tcharding · 2022-10-25T22:17:27Z

I will PR to fix it.

Woops, I missed that comment :)

tcharding · 2022-10-26T02:02:44Z

I forgot about Kix's suggestion about guaranteeing opcodes start with OP_, now its added.

Kixunil · 2022-10-26T09:38:08Z

Oh, I'd like to keep "Ord and PartialOrd intentionally not implemented." comment and even make it a doc comment.

Kixunil

ACK 74c3727fc78b5359d7b3f92f2decc5cbf7a8f98c

(Correct, I'm not ACKing removal of the comment.)

tcharding · 2022-10-26T23:13:55Z

Added new patch using text from comment above by @apoelstra

Kixunil

ACK c9b0c1d87766b840ea81a1c242cff095b7e8fce7

tcharding · 2022-10-28T00:00:20Z

Rebased on master, no other changes.

Kixunil

ACK c56e54a1b4b9c22d9d4d8a73d7696b9c9544a3d4

apoelstra · 2022-10-28T19:31:09Z

bitcoin/src/blockdata/opcodes.rs

In c56e54a1b4b9c22d9d4d8a73d7696b9c9544a3d4:

I'm happy to ACK this but TBH I don't think it belongs in a doccomment. Core's logic is hidden inside Core, so I think if you care about this you're probably reading the source code, not the API docs, and a regular comment will suffice.

Demoted it back to a code comment. This PR wins this months prize for most iterations on a totally trivial PR :)

I think the part about deliberately not implementing Ord is fine in doc comments so that the users will not ask us to implement it (or find the comment too late). For notes about Core a normal comment is fine. Notice that I wrote only the first part in my suggestion.

@tcharding perhaps you can save some time by waiting for resolution of our discussions before implementing changes when there are different opinions.

@tcharding perhaps you can save some time by waiting for resolution of our discussions before implementing changes when there are different opinions.

Yes, I'll try this and no be so trigger happy, cheers.

@Kixunil what if we said "We do not implement Ord on this type because there is no natural ordering on opcodes, but there may appear to be one (e.g. because all the push opcodes appear in a consecutive block) and we don't want to encourage subtly buggy code. Please use the associated methods on Opcode to distinguish different types of opcodes."

If we wanted to mention Core, we could also point out that Bitcoin Core's IsPushOnly considers OP_RESERVED to be a "push opcode", allowing this opcodes in contexts where only pushes are supposed to be allowed, because of exactly this category of mistakes.

Yes, I like that wording. The core example is a good one but not sure if we should add it to doc comment. Maybe hide it in <details><summary>Example of Core bug caused by assuming ordering</summary> ... </details>.

apoelstra

ACK f492c1d57fbcce0fb79f3eff9ef9fae6d115f349

The `all` module enables usage of a wildcard import statement without muddying the scope with any other types defined in `opcodes`, in other words if one wants to use the `All` type `opcodes::All` is the most clear way to use it, however usage of naked `OP_FOO` types is perfectly clear. Add documentation stating that we guarantee to never put anything else in the `all` module so folks are confident using a wildcard import will not bring any rubbish into scope. Expected usage in downstream applications that need types in `opcodes` as well as the opcodes: ``` use bitcoin::opcodes::all::*; use bitcoin::opcodes; ``` Also, we do no implement `Ord` or `PartialOrd`, document this including HTML tags hiding an example bug from Bitcoin Core that shows why not.

tcharding · 2022-11-05T19:16:09Z

I've force pushed changes that hopefully implement review suggestions. Please review carefully because the Core bug described is totally unknown to me and I did not go digging in Core source code looking for it. Also I re-worded the suggestion to mention All::classify, I believe that is what was meant by "Please use the associated methods on Opcode to distinguish different types of opcodes."

apoelstra · 2022-11-05T23:48:28Z

I'm happy with this. I think maybe Core developers might dispute this being a "bug" but I think it is :) and anyway I don't think it'd be a big deal if we had to change it later.

apoelstra

ACK 2157e69

Kixunil

ACK 2157e69

Kixunil reviewed Oct 24, 2022

View reviewed changes

tcharding force-pushed the 10-25-document-all-module branch from d43c65f to 5ccdfc8 Compare October 25, 2022 21:58

tcharding force-pushed the 10-25-document-all-module branch from 5ccdfc8 to 560be81 Compare October 26, 2022 02:01

Kixunil reviewed Oct 26, 2022

View reviewed changes

tcharding force-pushed the 10-25-document-all-module branch from 560be81 to c9b0c1d Compare October 26, 2022 23:13

Kixunil previously approved these changes Oct 27, 2022

View reviewed changes

Kixunil added documentation trivial Obvious, easy and quick to review (few lines or doc-only...) one ack PRs that have one ACK, so one more can progress them labels Oct 27, 2022

tcharding dismissed Kixunil’s stale review via c56e54a October 28, 2022 00:00

tcharding force-pushed the 10-25-document-all-module branch from c9b0c1d to c56e54a Compare October 28, 2022 00:00

Kixunil previously approved these changes Oct 28, 2022

View reviewed changes

apoelstra reviewed Oct 28, 2022

View reviewed changes

tcharding dismissed Kixunil’s stale review via f492c1d October 28, 2022 20:33

tcharding force-pushed the 10-25-document-all-module branch from c56e54a to f492c1d Compare October 28, 2022 20:33

apoelstra previously approved these changes Oct 28, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 2157e69 November 5, 2022 19:13

tcharding force-pushed the 10-25-document-all-module branch from f492c1d to 2157e69 Compare November 5, 2022 19:13

apoelstra approved these changes Nov 5, 2022

View reviewed changes

Kixunil approved these changes Nov 6, 2022

View reviewed changes

apoelstra merged commit c288141 into rust-bitcoin:master Nov 6, 2022

tcharding deleted the 10-25-document-all-module branch November 7, 2022 20:33

Conversation

tcharding commented Oct 24, 2022

Uh oh!

Kixunil 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 25, 2022

Uh oh!

apoelstra commented Oct 25, 2022

Uh oh!

tcharding commented Oct 25, 2022

Uh oh!

tcharding commented Oct 26, 2022

Uh oh!

Kixunil commented Oct 26, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 26, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 28, 2022

Uh oh!

Kixunil 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Nov 5, 2022

Uh oh!

apoelstra commented Nov 5, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants