<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31596.
<!--021abf342d371248e50ceaed478a90ca-->
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.
ACK 13464c0c44645e0ed88d9d72c3ad697dca800e10
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
* This means, for example, that ArithToUint256(num).GetHex() can be used to
* 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:
/** 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?
/** Get the hash of the wtxids of these transactions, treating each wtxid hash
* as a little-endian number and sorting them in ascending numeric order.
* This is equivalent to reversing the bytes of each wtxid hash and sorting them lexicographically.
*/
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
Could we be more compact here? Personally I'm not a fan of long documentation - though the reason usually is that if we need to explain something heavily it's a sign of poorly written code - not something we can really change here.
I think it's a useful comment.
'can be used to used to display' -> 'can be used to display'
This was already fixed
👍 my bad, github didn't show it as outdated and was looking at the convo tab.
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
{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.
Thanks for improving these! Since we can’t usually change the behavior, it’s crucial to share tribal knowledge. I have a few suggestions for reducing duplication, simplifying sentence structure, and applying these changes to similar places. However, I understand if you prefer to limit the scope
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.
* 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"},
Nit: Call it 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.
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
LGTM ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
Thanks for the reviews. Since there are more followups that could be made here, I created a branch to keep track of them. I don't want to create too much churn with documentation PRs so I won't open another one right away, but I can implement any suggestions made here, open a pr if requested, or wait for a related PR to be opened to attach these changes to.