bitcoin

mirror of https://github.com/bitcoin/bitcoin.git synced 2025-07-30 00:56:29 +02:00

Files

Ava Chow 1473f69924 Merge bitcoin/bitcoin#32421 : test: refactor: overhaul (w)txid determination for CTransaction objects

4ef6253017 test: avoid unneeded (w)txid hex -> integer conversions (Sebastian Falbesoner)
472f3770ae scripted-diff: test: rename CTransaction `.getwtxid()` -> `wtxid_hex` for consistency (Sebastian Falbesoner)
81af4334e8 test: rename CTransaction `.sha256` -> `.txid_int` for consistency (Sebastian Falbesoner)
ce83924237 test: rename CTransaction `.rehash()`/`.hash` -> `.txid_hex` for consistency (Sebastian Falbesoner)
e9cdaefb0a test: introduce and use CTransaction `.wtxid_int` property (Sebastian Falbesoner)
9b3dce24a3 test: remove bare CTransaction `.rehash()`/`.calc_sha256()` calls (Sebastian Falbesoner)
a2724e3ea3 test: remove txid caching in CTransaction class (Sebastian Falbesoner)

Pull request description:

  In the functional test framework, determining a (w)txid for a `CTransaction` instance is currently rather confusing and footgunny due to inconsistent naming/interfaces (see table below) and statefulness involved. This PR aims to improve that by:
  * removing the (w)txid caching mechanism, in order to avoid the need to call additional rehashing functions (`.rehash()`/`.calculate_sha256()`, see first two commits and https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993286997). This change in theory decreases the performance, as the involved serialization and hashing involved might be called more often than previously, but I couldn't find a functional test where this leads to a measurable run-time increase on my machine.
  * introduce consistent naming that shows the type of the returned txid, i.e. hex string vs. test-framework-internal representation [currently integers] (see remaining commits)

  Summary table showing (w)txid determaination before/after this PR:

  | Task                   | master                 | PR           |
  |:-----------------------|:-----------------------|:-------------|
  | get TXID (hex string)  | `.rehash()` / `.hash`[1] | `.txid_hex`  |
  | get TXID (integer)     | `.sha256`[1]             | `.txid_int`  |
  | get WTXID (hex string) | `.getwtxid()`          | `.wtxid_hex` |
  | get WTXID (integer)    | `.calc_sha256(True)`   | `.wtxid_int` |

  Unfortunately, most renames can't be done with a scripted-diff, as the property names (`.hash`, `.sha256`) are also used for blocks and other message types. The PR is rather invasive and touches a lot of files, but I think it's worth to do it, also to make life easier for new contributors. Future tasks like e.g. doing the same overhaul for block (header) objects or getting rid of the integer representation (see https://github.com/bitcoin/bitcoin/pull/32050) become easier should become easier after this one.

  [1] = returned value might be out-of-date, if rehashing function wasn't called after modification

ACKs for top commit:
  maflcko:
    re-ACK 4ef6253017 🏈
  achow101:
    ACK 4ef6253017
  marcofleon:
    code review ACK 4ef6253017

Tree-SHA512: 4b472c31d169966b6f6878911a8404d25bf3e503b6e8ef30f36a7415d21ad4bc1265083af2d3ead6edfcd9fac9ccb0a8be57e1b0739ad431b836413070d7d583