The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31596.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, Sjors, BrandonOdiwuor, achow101 |
| Concept ACK | l0rinc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
86+ * last), the GetHex() output will match the way the number would normally
87+ * be written in base-16 (with most significant digits first and least
88+ * significant digits last).
89+ *
90+ * This means, for example, that ArithToUint256(num).GetHex() can be used to
91+ * used to display an arith_uint256 num value as a number, because
0 * This means, for example, that ArithToUint256(num).GetHex() can be used to
1 * display an arith_uint256 num value as a number, because
re: #31596 (review)
Thanks, took suggestion
25@@ -26,6 +26,7 @@ class base_uint
26 protected:
27 static_assert(BITS / 32 > 0 && BITS % 32 == 0, "Template parameter BITS must be a positive multiple of 32.");
28 static constexpr int WIDTH = BITS / 32;
29+ /** Number represented in least-significant-first base 2^32 digits. */
I had to read this several times (didn’t understand what a “least-significant-first base” is at first), consider the following:
0 /** Integer with 32-bit digits, least-significant first. */
re: #31596 (review)
Thanks, agree that is clearer, took the suggestion but kept “represented”
88@@ -89,8 +89,9 @@ bool IsChildWithParents(const Package& package);
89 */
90 bool IsChildWithParentsTree(const Package& package);
91
92-/** Get the hash of these transactions' wtxids, concatenated in lexicographical order (treating the
93- * wtxids as little endian encoded uint256, smallest to largest). */
94+/** Get the hash of these transactions' wtxids, with wtxids treated as little
95+ * endian numbers and sorted in increasing numeric order (i.e. with byte-reversed
96+ * wtxid hashes sorted lexicographically.) */
According to Wiki little endian is preferred with a dash: https://en.wikipedia.org/wiki/Endianness
But the part I find confusing is rather expressing 3 concepts in a single sentence - can we break it up a bit?
0/** Get the hash of the wtxids of these transactions, treating each wtxid hash
1 * as a little-endian number and sorting them in ascending numeric order.
2 * This is equivalent to reversing the bytes of each wtxid hash and sorting them lexicographically.
3 */
re: #31596 (review)
Thanks I basically just took your first sentence. I dropped the second sentence, because I think it’s not essential, and the phrasing is potentially misleading because the the wtxid bytes are only reversed when they are sorted, not when they hashed after being sorted.
Should be ok not to mention here that you can sort little endian numbers by reversing the bytes.
88+ * significant digits last).
89+ *
90+ * This means, for example, that ArithToUint256(num).GetHex() can be used to
91+ * used to display an arith_uint256 num value as a number, because
92+ * ArithToUint256() converts the number to a blob in little-endian format,
93+ * so the arith_uint256 class doesn't need to have its own number parsing
632@@ -633,8 +633,8 @@ static RPCHelpMan getblocktemplate()
633 {RPCResult::Type::OBJ, "", "",
634 {
635 {RPCResult::Type::STR_HEX, "data", "transaction data encoded in hexadecimal (byte-for-byte)"},
636- {RPCResult::Type::STR_HEX, "txid", "transaction id encoded in little-endian hexadecimal"},
637- {RPCResult::Type::STR_HEX, "hash", "hash encoded in little-endian hexadecimal (including witness data)"},
638+ {RPCResult::Type::STR_HEX, "txid", "txid hash (excluding witness data) with hash bytes reversed, encoded as hex"},
documenting txid as “it’s the txid hash” is a bit redundant and confusing.
In https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L135 (and a few other places) the description is "The transaction hash in hex".
Could we unify the rest as well to something simple like
0 {RPCResult::Type::STR_HEX, "txid", "transaction id hash (without witness) shown in byte-reversed hex"},
re: #31596 (review)
Thanks, I like “shown in byte-reversed hex” as a shorter alternative, and tried to incorporate your other ideas while not sounding ambiguous. If reviewers are happy with the new phrasing I’ll add new commit changing other RPCs to use it. The other RPCs do seem ok as-is because they don’t mention “little-endian hexadecimal”, but it’s good to be consistent.
re: #31596 (review)
Could we unify the rest as well
To follow up, I looked at occurrences of "txid", "wtxid", and "hash" (quoted strings) in the RPC code and found dozens of fields that could be updated to be consistent. It seems more challenging than I expected to update all the documentation, and now I’m also wondering if might actually be harmful to mention reversed bytes in some cases but not others, because this might give the wrong impression that bytes are not always reversed for every hash field.
I wonder if maybe a better approach to provide more consistency could be to introduce new RPC types like STR_TXID, and STR_WTXID and use them instead of the more generic STR_HEX type, so specific documentation about the meanings of these hashes could be incorporated into the type documentation and not repeated in field descriptions.
This is a documentation-only change following up on suggestions made in the
#30526 review.
Motivation for this change is that I was recently reviewing #31583, which
reminded me how confusing the arithmetic blob code was and made me want to
write better comments.
Updated 13464c0c44645e0ed88d9d72c3ad697dca800e10 -> 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40 (pr/docblob.1 -> pr/docblob.2, compare) with suggestions
Thanks for the reviews!
88@@ -89,8 +89,9 @@ bool IsChildWithParents(const Package& package);
89 */
90 bool IsChildWithParentsTree(const Package& package);
91
92-/** Get the hash of these transactions' wtxids, concatenated in lexicographical order (treating the
93- * wtxids as little endian encoded uint256, smallest to largest). */
94+/** Get the hash of the concatenated wtxids of transactions, with wtxids
95+ * treated as a little-endian numbers and sorted in ascending numeric order.
0 * treated as little-endian numbers and sorted in ascending numeric order.
re: #31596 (review)
Thanks, fixed in a followup branch.
632@@ -633,8 +633,8 @@ static RPCHelpMan getblocktemplate()
633 {RPCResult::Type::OBJ, "", "",
634 {
635 {RPCResult::Type::STR_HEX, "data", "transaction data encoded in hexadecimal (byte-for-byte)"},
636- {RPCResult::Type::STR_HEX, "txid", "transaction id encoded in little-endian hexadecimal"},
637- {RPCResult::Type::STR_HEX, "hash", "hash encoded in little-endian hexadecimal (including witness data)"},
638+ {RPCResult::Type::STR_HEX, "txid", "transaction hash excluding witness data, shown in byte-reversed hex"},
639+ {RPCResult::Type::STR_HEX, "hash", "transaction hash including witness data, shown in byte-reversed hex"},
hexadecimal instead of hex like done in the "data" field for better consistency?
re: #31596 (review)
Nit: Call it
hexadecimalinstead ofhexlike done in the"data"field for better consistency?
I would probably prefer to go the other way since the term “hex” occurs more frequently than “hexadecimal” in RPC code and documentation (115 vs 11 times). I’m hoping we might be able to improve consistency by moving this information from descriptions of RPC fields to descriptions of RPC types and left another comment about this.
