test: avoid treating hash results as integers #32050

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202503-test-avoid_hash_as_int changing 5 files +22 −28
  1. theStack commented at 0:37 am on March 13, 2025: contributor

    In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and there is usually no need to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (<) operator, or interpreting the byte-string as scalar in the EC-context for e.g. key derivation.

    I’d hence argue that most uses of ser_uint256/uint256_from_str and txid conversions via int(txid/blockhash, 16) are potential code smells and should be reduced to a minimum long-term if possible. This PR is a first step into this direction, intentionally kept small with (what I think) uncontroversial changes for demonstration purposes, to check out if other contributors are interested in this. A next step could be to change the classes of primitives (CTransaction, CBlock etc.) and network messages (msg_) to store hash results as actual bytes (maybe in a class wrapping the bytes that offers conversion from/to human-readable strings [1], for easier interaction with RPC calls and debug outputs) rather than ints. But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.

    [1] unfortunately, txids and block hashes are shown to user in reverse byte order, so e.g. a txid_bytes->txid_str conversion is not just a simple txid_bytes.hex(), but a txid_bytes[::-1].hex()

  2. DrahtBot commented at 0:37 am on March 13, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32050.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, rkrux, ryanofsky
    Concept ACK laanwj, l0rinc, yancyribbens, BrandonOdiwuor

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Mar 13, 2025
  4. test: avoid unneeded hash -> uint256 -> hash roundtrips
    In the functional test framework, we often treat hashes
    as uint256 integers, which seems to be confusing and for no
    good reason, as hashes are just sequences of bytes. This commit
    gets rid of obvious internal instances of that where individual
    functional tests are not affected. In the long-term, it might make
    sense to store other hashes (mostly txids) as actual bytes to
    avoid annoying conversions and improve code readability.
    346a099fc1
  5. test: simplify (w)txid checks by avoiding .calc_sha256 calls
    Calls on the tx.calc_sha256 method can be confusing, as they return
    the result (either txid or wtxid, depending on the with_witness
    boolean parameter) as integer rather than as actual (w)txid. Use
    .rehash() and .getwtxid() instead to improve readability and in some
    cases avoid a conversion from string-txid to an integer.
    a82829f37e
  6. theStack force-pushed on Mar 13, 2025
  7. DrahtBot added the label CI failed on Mar 13, 2025
  8. DrahtBot commented at 0:43 am on March 13, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38676500414

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  9. DrahtBot removed the label CI failed on Mar 13, 2025
  10. laanwj commented at 10:04 am on March 13, 2025: member
    Concept ACK, this has been a source of frustration and misunderstanding in bitcoin’s code since the beginning. Hashes are byte blobs, not numbers, and unless there’s a specific good reason to interpret them as a number (can’t think of more cases than those you already mention) they shouldn’t be.
  11. in test/functional/feature_segwit.py:270 in a82829f37e
    266@@ -267,7 +267,7 @@ def run_test(self):
    267         tx1 = tx_from_hex(tx1_hex)
    268 
    269         # Check that wtxid is properly reported in mempool entry (txid1)
    270-        assert_equal(int(self.nodes[0].getmempoolentry(txid1)["wtxid"], 16), tx1.calc_sha256(True))
    271+        assert_equal(self.nodes[0].getmempoolentry(txid1)["wtxid"], tx1.getwtxid())
    


    maflcko commented at 11:05 am on March 13, 2025:

    would be nice to remove calc_sha256(True) completely in a follow-up, or at least force named args.

    Also, if the wtxid is never cached, I wonder what the point is to cache the txid. Either both or none are cached.

    But this can be done in a follow-up.


    theStack commented at 2:32 pm on March 13, 2025:
    Agree. My gut-feeling preference would be to remove the caching both for txid and wtxids, as I guess it doesn’t make a notable performance difference (likely most of the time in functional tests is spent waiting for RPC results), but have to verify that assumption first.
  12. maflcko approved
  13. maflcko commented at 11:05 am on March 13, 2025: member

    review ACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc 🐘

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: review ACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc 🐘
    3RWIFBCsUQhJrwwGbP2FK9owr89qLyKJcC68ZX7FTHO770qoPXSHo3z6b1zPaFYZ8Bb4Dou5qXBZZPJoKZOIaDA==
    
  14. DrahtBot requested review from laanwj on Mar 13, 2025
  15. l0rinc commented at 11:08 am on March 13, 2025: contributor
    Concept ACK
  16. yancyribbens commented at 12:21 pm on March 13, 2025: contributor
    Concept ACK. It looks like the result of the sample changes is less casting which is a good sign.
  17. in test/functional/test_framework/blocktools.py:114 in a82829f37e
    109@@ -111,8 +110,8 @@ def create_block(hashprev=None, coinbase=None, ntime=None, *, version=None, tmpl
    110     return block
    111 
    112 def get_witness_script(witness_root, witness_nonce):
    113-    witness_commitment = uint256_from_str(hash256(ser_uint256(witness_root) + ser_uint256(witness_nonce)))
    114-    output_data = WITNESS_COMMITMENT_HEADER + ser_uint256(witness_commitment)
    115+    witness_commitment = hash256(ser_uint256(witness_root) + ser_uint256(witness_nonce))
    116+    output_data = WITNESS_COMMITMENT_HEADER + witness_commitment
    


    rkrux commented at 1:17 pm on March 13, 2025:

    bytes -> int and then int -> bytes

    type conversion is distracting anyway, all the more if not necessary

    going away with this change, +1

  18. rkrux approved
  19. rkrux commented at 1:38 pm on March 13, 2025: contributor

    Concept and utACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc

    the only exceptions I could think of is PoW-verification of block hashes with the less-than (<) operator

    As soon as I read the PR title, I immediately thought of this^ use case but I didn’t know there were not more such use cases like this.

    But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.

    Agree, good decision to clean up only in the tests first.

  20. DrahtBot requested review from l0rinc on Mar 13, 2025
  21. ryanofsky commented at 4:04 pm on March 13, 2025: contributor

    Changes in this PR seem reasonable to me although I don’t agree with premise that it is an improvement to treat hashes as blobs, but then arbitrarily decide to display those blobs as reverse-byte hex strings, when there is probably not other software that displays blobs as reverse-byte hex strings.

    Status quo where hashes are simply interpreted as little endian numbers, and those numbers are represented as hex strings in the standard way python and other software represents numbers represented as strings makes more sense conceptually and technically, IMO.

    I also think the part of the PR description that says it “almost never makes sense to apply integer operations” to hashes and then immediately lists examples where it makes sense to apply integer operations to hashes is a little hard to take seriously. Would be better to say something like converting hashes to numbers is usually not necessary, so it is better to avoid doing it when not needed.

    At the end of the day, I don’t think it matters too much which approach is taken, and this code change does seem to remove some blob > number > blob round trips, which is nice.

    My only concern would be that this PR is labeled “part 1” so I’d be worried about how many more parts there will be and whether code will be in left in a confusing intermediate state while it’s being converted from one approach to the other.

  22. rkrux commented at 5:26 pm on March 13, 2025: contributor

    Re: #32050 (comment)

    These are quite subtle points that are raised, but important IMO.

    Even if for some reason later it’s decided to not do the cpp changes as suggested in the description, the functional test changes as done in this PR are good enough in their own right, and hence, I agree that “part 1” can be dropped from the title unless the author intends to do more type-conversion related functional test changes, which one can argue can be added in this PR itself because there appears to be a conceptual agreement for these changes in the tests now. Some portion of the language can also be altered in the description like suggested in the comment if the author agrees.

  23. theStack renamed this:
    test: avoid treating hash results as integers (part 1)
    test: avoid treating hash results as integers
    on Mar 13, 2025
  24. theStack commented at 6:40 pm on March 13, 2025: contributor

    @ryanofsky @rkrux: Thanks for the feedback! Removed the “(part 1)” from the PR title as suggested and adapted the strong “almost never makes sense” wording in the description. I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in-reverse-order).

    Status quo where hashes are simply interpreted as little endian numbers, and those numbers are represented as hex strings in the standard way python and other software represents numbers represented as strings makes more sense conceptually and technically, IMO.

    I wouldn’t agree to that statement, especially the “simply” part. For starters, many programming languages don’t even offer support for larger numbers in the standard library, and it seems unnecessary to artificially involve an additional data type in the hash -> string conversion chain if that type is not even used for what it’s intended to. Note that the changes proposed in this PR have been done for the main codebase already more than ten years ago: #5490 (even though arguably, the name uint256 might still suggest otherwise). For the few cases where treating a hash as an integer does indeed make sense, an explicit type arith_uint256 has been introduced.

    My only concern would be that this PR is labeled “part 1” so I’d be worried about how many more parts there will be and whether code will be in left in a confusing intermediate state while it’s being converted from one approach to the other.

    Fair point, I agree that we shouldn’t end up in an intermediate state. But these worries can be expressed in upcoming PRs that might create such a state (if there is any); whether there will be additional PRs or not is obviously not so much dependend on the title of a PR, but on whether other contributors signal interest in follow-ups (one has been expressed e.g. in #32050 (review)).

  25. BrandonOdiwuor commented at 6:09 pm on March 20, 2025: contributor
    Concept ACK
  26. in test/functional/test_framework/script.py:712 in 346a099fc1 outdated
    708@@ -711,41 +709,42 @@ def sign_input_segwitv0(tx, input_index, input_scriptpubkey, input_amount, privk
    709 # Note that this corresponds to sigversion == 1 in EvalScript, which is used
    710 # for version 0 witnesses.
    711 def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount):
    712+    ZERO_HASH = bytes([0]*32)
    


    ryanofsky commented at 1:21 am on March 27, 2025:

    In commit “test: avoid unneeded hash -> uint256 -> hash roundtrips” (346a099fc1e282c8b53d740d65999d8866d17953)

    Could simplify this to ZERO_HASH = bytes(32)


    theStack commented at 5:34 am on March 27, 2025:
    Will do if I have to retouch.
  27. ryanofsky assigned ryanofsky on Mar 27, 2025
  28. in test/functional/p2p_segwit.py:339 in a82829f37e
    335@@ -336,8 +336,7 @@ def test_unnecessary_witness_before_segwit_activation(self):
    336 
    337         # Verify the hash with witness differs from the txid
    338         # (otherwise our testing framework must be broken!)
    339-        tx.rehash()
    340-        assert tx.sha256 != tx.calc_sha256(with_witness=True)
    341+        assert tx.rehash() != tx.getwtxid()
    


    ryanofsky commented at 1:41 am on March 27, 2025:

    In commit “test: simplify (w)txid checks by avoiding .calc_sha256 calls” (a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc)

    It seems like there might be a test bug here, and this check will no longer fail even if the hash is the same because rehash returns an int while getwtxid returns a hex str


    theStack commented at 5:33 am on March 27, 2025:

    Note that both the rehash and getwtxid methods return a hex string. The former returns self.hash, which is set in .calc_sha256 as follows: https://github.com/bitcoin/bitcoin/blob/f1d129d96340503ec5f6b347c2fdf6a6901b1f7e/test/functional/test_framework/messages.py#L682

    Another example where both of these methods are called and directly compared: https://github.com/bitcoin/bitcoin/blob/f1d129d96340503ec5f6b347c2fdf6a6901b1f7e/test/functional/mempool_accept.py#L416

    (Something for a different PR, but I think rehash should either not return anything, or be renamed to gettxid in the future if we decide to remove the caching, see e.g. #32050 (review)).


    ryanofsky commented at 1:11 pm on March 27, 2025:

    re: #32050 (review)

    Note that both the rehash and getwtxid methods return a hex string.

    Oops, sorry I misread the code. I saw rehash was setting the sha256 member, and didn’t realize it was actually returning a different member.

  29. ryanofsky changes_requested
  30. ryanofsky commented at 1:44 am on March 27, 2025: contributor
    Code review a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc and thanks for the updates! It looks like this might introduce a minor bug though (see comment below)
  31. ryanofsky approved
  32. ryanofsky commented at 1:13 pm on March 27, 2025: contributor
    Code review ACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc. Nice changes, and sorry about the false bug report
  33. DrahtBot requested review from BrandonOdiwuor on Mar 27, 2025
  34. ryanofsky merged this on Mar 27, 2025
  35. ryanofsky closed this on Mar 27, 2025

  36. theStack deleted the branch on Mar 27, 2025
  37. ryanofsky commented at 3:09 pm on March 27, 2025: contributor

    re: #32050 (comment)

    I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in-reverse-order).

    I think my point was that it wasn’t an arbitrary decision but an intentional one, even if it didn’t lead a positive outcome.

    Bitcoin proof of work compares hashes with native uint32_t comparisons rather than memcmp when memcmp would be more straightforward. Maybe this was done because it was thought these comparisons would be faster on little endian machines. Or maybe this was done to try to be consistent with other logic already using little endian. But there was clearly some explicit choice to interpret hashes as little endian numbers, and to display them the way they are interpreted: as little endian numbers, not as blobs. The code goes out of its way to do this, it can’t really be characterized as an arbitrary choice.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-28 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me