Page MenuHomePhabricator

test: remove txid caching in CTransaction class
ClosedPublic

Authored by PiRK on Sep 29 2025, 15:33.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC496a019e7180: test: remove txid caching in CTransaction class
Summary

Rather than txids (represented by the fields .sha256 and .hash)
being stateful, simply compute them on-the-fly. This ensures that
the correct values are always returned and takes the burden of
rehashing from test writers, making the code shorter overall.
In a first step, the fields are kept at the same name with @property
functions as drop-in replacements, for a minimal diff. In later commits,
the names are changed to be more descriptive and indicating the return
type of the txid.

This is a partial backport of core#32421
https://github.com/bitcoin/bitcoin/pull/32421/commits/a2724e3ea392cdbd5526735516862b17bf687624

Depends on D18711

Note that contrary to Bitcoin Core, Bitcoin ABC never uses the return value of CTransaction.calc_sha256() so we can remove it immediately.

Test Plan

ninja check-functional-extended

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Sep 29 2025, 15:33
PiRK planned changes to this revision.Sep 29 2025, 19:28

i think i need to check the effect of this on the perf of the feature_block test. Specifically the numerous calls to make_conform_to_ctor may slow down the test now that the txid is no longer cached and the sorting process may need to recompute the txids multiple times.

PiRK requested review of this revision.Sep 29 2025, 19:48

i fixed the issue in D18713, there is no significant problem with the performance because of caching, it was a bug

Fabien added a subscriber: Fabien.
Fabien added inline comments.
test/functional/test_framework/messages.py
445 ↗(On Diff #55944)

can we add type annotations here ? That's better than the docstring

This revision is now accepted and ready to land.Sep 29 2025, 20:02