New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve documentation on the all
module
#1339
Improve documentation on the all
module
#1339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not wrong but if we're explicitly guaranteeing things we should also guarantee beginning with OP_
bitcoin/src/blockdata/opcodes.rs
Outdated
/// The `all` module is provided so one can use a wildcard import `use bitcoin::opcodes::all::*` to | ||
/// get all the `OP_FOO` opcodes without getting other types defined in `opcodes` (e.g. `All`, `Class`). | ||
/// | ||
/// This module is guaranteed to never contain anything except opcode constants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And all opcode constants are guaranteed to begin with OP_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used as suggested.
bitcoin/src/blockdata/opcodes.rs
Outdated
/// A script Opcode. | ||
// Note: I am deliberately not implementing PartialOrd or Ord. If you want to check ranges of | ||
// opcodes, etc., write an #[inline] helper function which casts to u8s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what's the reason though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea, @apoelstra did you write this originally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 u8
s before the <
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also like, I'm pretty sure we have the same logic, I just had to add some
as u8
s before the<
s
Made me laugh :)
d43c65f
to
5ccdfc8
Compare
Removed code comment as suggested. |
CI failure is in master. I will PR to fix it. |
Woops, I missed that comment :) |
5ccdfc8
to
560be81
Compare
I forgot about Kix's suggestion about guaranteeing opcodes start with |
Oh, I'd like to keep " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 74c3727
(Correct, I'm not ACKing removal of the comment.)
560be81
to
c9b0c1d
Compare
Added new patch using text from comment above by @apoelstra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c9b0c1d
Rebased on master, no other changes. |
c9b0c1d
to
c56e54a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c56e54a
bitcoin/src/blockdata/opcodes.rs
Outdated
/// | ||
/// We deliberately do not implement `PartialOrd` or `Ord` because there is no ordering of opcodes | ||
/// that makes conceptual sense. Note however that Bitcoin Core does use `<` and `>` on opcodes to | ||
/// effect consensus logic, something which hides implicit assumptions about the opcodes' byte encoding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In c56e54a:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Demoted it back to a code comment. This PR wins this months prize for most iterations on a totally trivial PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
.
c56e54a
to
f492c1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f492c1d
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.
f492c1d
to
2157e69
Compare
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2157e69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2157e69
Improve documentation on the
all
module by doing:all
will only ever contain opcode constantsDone as follow up to #1295