Skip to content

refactor(tests): move TxTemplate from chain to testenv#2177

Open
tvpeter wants to merge 4 commits into
bitcoindevkit:masterfrom
tvpeter:refactor/mv-txtemplate-to-testenv
Open

refactor(tests): move TxTemplate from chain to testenv#2177
tvpeter wants to merge 4 commits into
bitcoindevkit:masterfrom
tvpeter:refactor/mv-txtemplate-to-testenv

Conversation

@tvpeter

@tvpeter tvpeter commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR moves the TxTemplate from the chain crate into the testenv. It also updates the TxTemplate to use the Cow smart pointer, adds method implementations to TxTemplate, and migrates tests to the builder API.

This PR addresses the first and second todos on #1936.

Changelog notice

  • moved TxTemplate from chain to testenv
  • added builder API methods to TxTemplate

Checklists

All Submissions:

@oleonardolima oleonardolima left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK a5d9f41

You can rebase to get the latest toolchain updated CI. Overall looks good, I only would like to take a deeper look into the last commit.

@oleonardolima

Copy link
Copy Markdown
Collaborator

cACK a5d9f41

You can rebase to get the latest toolchain updated CI. Overall looks good, I only would like to take a deeper look into the last commit.

Also, I'd avoid mixing renaming and moving code in the same commits.

@tvpeter
tvpeter force-pushed the refactor/mv-txtemplate-to-testenv branch from a5d9f41 to 6d5cac1 Compare April 30, 2026 02:58
@oleonardolima

Copy link
Copy Markdown
Collaborator

@tvpeter Oh, there are some conflicts 😅

@tvpeter

tvpeter commented May 1, 2026

Copy link
Copy Markdown
Collaborator Author

@tvpeter Oh, there are some conflicts 😅

Yeah, from the recent merge. I will rebase and resolve

@tvpeter
tvpeter force-pushed the refactor/mv-txtemplate-to-testenv branch 2 times, most recently from e381444 to 49ba5a5 Compare May 2, 2026 12:46
@oleonardolima
oleonardolima force-pushed the refactor/mv-txtemplate-to-testenv branch from 49ba5a5 to 067245e Compare June 15, 2026 16:55
@oleonardolima

Copy link
Copy Markdown
Collaborator

@tvpeter I've rebased it on top of master to solve the conflicts, let me know if it's all good or I've messed something.

@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.32%. Comparing base (1acb4d0) to head (16619d4).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2177   +/-   ##
=======================================
  Coverage   78.32%   78.32%           
=======================================
  Files          30       30           
  Lines        5934     5934           
  Branches      281      281           
=======================================
  Hits         4648     4648           
  Misses       1210     1210           
  Partials       76       76           
Flag Coverage Δ
rust 78.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tvpeter

tvpeter commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

@tvpeter I've rebased it on top of master to solve the conflicts, let me know if it's all good or I've messed something.

At a glance, it looks just fine. Thank you.

@evanlinjin

evanlinjin commented Jun 16, 2026

Copy link
Copy Markdown
Member

Remember that PR descriptions need to explain why things are done.

It also updates the TxTemplate to use the Cow smart pointe

What is the rationale for this?

Edit: I see that it is my suggestion in the ticket. I was wrong. Cow only makes sense if the type will start as a reference and may need to be mutated later on.

@tvpeter

tvpeter commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Edit: I see that it is my suggestion in the ticket. I was wrong. Cow only makes sense if the type will start as a reference and may need to be mutated later on.

Alright, should I revert the aspect that updates the TxTemplate struct using Cow?

@tvpeter
tvpeter force-pushed the refactor/mv-txtemplate-to-testenv branch 3 times, most recently from 5ee920d to ee76753 Compare June 30, 2026 02:46
Comment thread crates/testenv/src/utils.rs
Comment thread crates/testenv/src/tx_template.rs
Comment thread crates/testenv/src/tx_template.rs Outdated
Comment on lines +63 to +65
/// If `Some(index)`, the output will use the script pubkey at that index
/// from the test descriptor set. If `None`, a random (empty) script is used.
pub spk_index: Option<u32>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 This will cause a panic on L214 if the spk index wasn't inserted into the index.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored init_graph fn to prevent panics, check the outputs of each TxTemplate, derive and inserts spk that the indexer has not yet seen just-in-time before the transaction is constructed and scanned.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, also spk_index can't be in the hardened derivation range either (>= 2^31), worth a comment somewhere in the docs.

Comment thread crates/testenv/src/tx_template.rs
Comment thread crates/testenv/src/lib.rs
tvpeter added 3 commits July 20, 2026 20:51
- Move the tx template helpers (TxTemplate,
TxInTemplate,TxOutTemplate, init_graph) out of
crates/chain/tests/common into the testenv.
Move shared test/bench helpers into testenv utils.
Update benches and the affected tests to use them
- Remove changeset macro because it is no longer
used
- Merged `new_standard_tx`` into `new_tx`` to have
 a uniform helper for creating standard tx and
 updated call sites across tests
- Removed the static loop in init_graph that
pre-populated the SpkTxOutIndex and replaced it
with a check in the TxTemplate outputs, that
derives and populate empty the script pub key for
 indexers without spk  before a transaction is
 constructed and scanned. This refactor removes
 the 0-9  limitation of having just 10 outputs.
 Users can use any spk_index without panic.
 Also removed generating spks that may not be used
- Renamed `txid_to_name` field to `txids` in
TxTemplateEnv and update usage across tests
- Remove dead_code flag
@tvpeter
tvpeter force-pushed the refactor/mv-txtemplate-to-testenv branch from ee76753 to 16619d4 Compare July 21, 2026 15:35

@ValuedMammal ValuedMammal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 16619d4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

5 participants