Skip to content

feat(functional-tests): migrate btcio tests to new framework (STR-2090)#1451

Merged
storopoli merged 7 commits intomainfrom
STR-2090-migrate-btcio-tests
Apr 7, 2026
Merged

feat(functional-tests): migrate btcio tests to new framework (STR-2090)#1451
storopoli merged 7 commits intomainfrom
STR-2090-migrate-btcio-tests

Conversation

@voidash
Copy link
Copy Markdown
Contributor

@voidash voidash commented Mar 3, 2026

Summary

  • Migrates 3 of 6 legacy btcio functional tests to the new functional-tests-new framework
  • All tests use strata_getL1HeaderCommitment as the replacement for removed L1 RPCs (strata_l1connected, strata_l1status, strata_getL1blockHash)

Migrated tests

New test Replaces What it does
test_l1_connected btcio_connect Verifies strata can see pre-existing L1 blocks
test_l1_tracking btcio_read Mines new blocks, checks strata picks them up
test_l1_reorg btcio_read_reorg Invalidates blocks, checks strata handles chain reorg

Not ported (RPCs removed from new binary)

Old test Missing RPC Notes
btcio_broadcast strataadmin_broadcastRawTx Broadcast removed from new binary
btcio_inscriber strataadmin_submitDABlob DA blob submission removed
btcio_resubmit_checkpoint prover infra Requires prover pipeline

Test plan

  • Run cd functional-tests-new && uv run python entry.py -g btcio to verify all 3 tests pass
  • Verify reorg test uses standalone env (does not share bitcoin node with other tests)

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 3, 2026

Codecov Report

❌ Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.04%. Comparing base (5d370cd) to head (0ced17d).
⚠️ Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
crates/asm/worker/src/service.rs 70.58% 5 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5d370cd) and HEAD (0ced17d). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (5d370cd) HEAD (0ced17d)
unit 2 1
functional 1 0
@@            Coverage Diff             @@
##             main    #1451      +/-   ##
==========================================
- Coverage   75.44%   66.04%   -9.40%     
==========================================
  Files         806      819      +13     
  Lines       76648    79186    +2538     
==========================================
- Hits        57825    52302    -5523     
- Misses      18823    26884    +8061     
Flag Coverage Δ
functional ?
unit 66.04% <70.58%> (+0.40%) ⬆️

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

Files with missing lines Coverage Δ
crates/asm/worker/src/service.rs 77.19% <70.58%> (-8.67%) ⬇️

... and 320 files with indirect coverage changes

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 3, 2026

Commit: 0a65799

SP1 Execution Results

program cycles success
EVM EE STF 1,322,399
Checkpoint 4,720
Checkpoint New 881,434

@voidash voidash marked this pull request as ready for review March 17, 2026 09:37
@voidash voidash requested a review from a team as a code owner March 17, 2026 09:37
Copy link
Copy Markdown
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Generally we don't want the wait_until_ functions to be called in tests directly unless there's some really specific condition the test is looking for. We want to expose the high level operation through helpers that make it so that we can change the implementation (like if RPCs get reworked) without having to go and update every test. Here especially, a very similar pattern is repeated 5 times, so it makes sense to consolidate it.

Comment thread functional-tests-new/tests/btcio/test_l1_connected.py Outdated
Comment thread functional-tests-new/tests/btcio/test_l1_reorg.py Outdated
Comment thread functional-tests-new/tests/btcio/test_l1_tracking.py Outdated
Comment thread functional-tests-new/tests/btcio/test_l1_reorg.py Outdated
Comment thread functional-tests-new/tests/btcio/test_l1_tracking.py Outdated
@storopoli
Copy link
Copy Markdown
Member

Agree with @delbonis comments in the wait_until_value calls.
If it helps, you can see how I've did with a wait_for_non_empty_blob helper function in functional-tests-new/common/services/alpen_client.py in #1505 in this git commit

@voidash voidash force-pushed the STR-2090-migrate-btcio-tests branch from 28ab2c0 to 814c0e2 Compare March 24, 2026 12:44
@voidash voidash requested a review from delbonis March 24, 2026 12:44
@delbonis
Copy link
Copy Markdown
Contributor

Just that remaining strata_getL1HeaderCommitment-based wait call.

voidash added 3 commits March 26, 2026 21:12
… (STR-2090)

Migrate 3 of 6 btcio tests that are portable to the new strata binary:
- test_l1_connected: verifies strata sees L1 blocks (replaces btcio_connect)
- test_l1_tracking: mines blocks and checks strata picks them up (replaces btcio_read)
- test_l1_reorg: invalidates blocks and checks strata handles reorg (replaces btcio_read_reorg)

All tests use strata_getL1HeaderCommitment as the replacement for the
removed strata_l1connected/strata_l1status/strata_getL1blockHash RPCs.

Not ported (RPCs removed from new binary):
- btcio_broadcast (strataadmin_broadcastRawTx)
- btcio_inscriber (strataadmin_submitDABlob)
- btcio_resubmit_checkpoint (prover infrastructure)
- Increase wait_until_with_value timeouts from 30s to 60s in
  test_l1_tracking and test_l1_reorg — strata needs more time in CI
  to process L1 blocks, especially with 110 pre-generated blocks
- Run ruff format on both files to fix lint failures
Move inline wait_until_with_value calls for L1 header commitments into
a dedicated StrataService.wait_for_l1_commitment() method, consistent
with existing wait_for_block_height/wait_for_block patterns.
@voidash voidash force-pushed the STR-2090-migrate-btcio-tests branch from 814c0e2 to a6ad430 Compare March 26, 2026 15:27
- SIM103: simplify conditional return in wait_for_l1_commitment
- Format legacy testenv.py
storopoli
storopoli previously approved these changes Mar 26, 2026
Copy link
Copy Markdown
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK ce0a18d

Copy link
Copy Markdown
Contributor

@bewakes bewakes left a comment

Choose a reason for hiding this comment

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

Minor nits. Feel free to resolve them and proceed with merging after the failing functional tests are fixed.

Comment thread functional-tests-new/common/services/strata.py Outdated
Comment thread functional-tests-new/tests/btcio/test_l1_connected.py
delbonis
delbonis previously approved these changes Mar 27, 2026
@storopoli storopoli enabled auto-merge March 27, 2026 15:32
- test_l1_reorg: mine blocks above genesis before reorg (was checking
  heights below genesis where ASM never creates manifests)
- test_l1_tracking: use standalone env to avoid sequencer restart
  interference, increase timeouts
- Disable both tests: there is a race in the ASM worker where the L1
  reader notifies the ASM about a new block before the full block data
  is persisted. The ASM fails with "missing l1 block" and the
  notification is consumed without retry, permanently losing that block.
  This affects any test mining L1 blocks while strata is running.
@voidash voidash dismissed stale reviews from delbonis and storopoli via 3ce4b69 March 30, 2026 07:08
@voidash voidash requested a review from a team as a code owner March 30, 2026 07:08
Comment thread functional-tests-new/entry.py Outdated
- Rename wait_for_l1_commitment -> wait_for_l1_commitment_at (bewakes nit)
- Replace last direct strata_getL1HeaderCommitment call in
  test_l1_tracking with the wait_for_l1_commitment_at helper (delbonis)
- Clarify disabled_tests comment: explain test_l1_connected is
  unaffected, add TODO to re-enable once ASM retry logic is fixed
@voidash voidash requested a review from a team as a code owner March 31, 2026 10:55
The L1 reader notifies the ASM worker as soon as it stores the
height→blockid mapping, but the block may not yet be fetchable (DB
propagation lag, Bitcoin RPC timeout under load).  Previously this
caused the ASM worker to exit permanently, consuming the notification
without retry.

Add get_l1_block_with_retry() with exponential backoff (200ms base,
1.5× growth, 2s cap, 10 retries ≈ 10s total).  Only MissingL1Block
is retried; all other errors propagate immediately.

Re-enable test_l1_tracking and test_l1_reorg which were blocked on
this race condition.
@voidash voidash force-pushed the STR-2090-migrate-btcio-tests branch from abadd33 to 0ced17d Compare March 31, 2026 11:01
Copy link
Copy Markdown
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK 0ced17d

Since we are adding a new function to asm I'd like someone from @alpenlabs/chain-protocol to review this all well

Comment thread crates/asm/worker/src/service.rs
Comment thread crates/asm/worker/src/service.rs
@storopoli storopoli added this pull request to the merge queue Apr 7, 2026
Merged via the queue into main with commit 308211f Apr 7, 2026
68 of 69 checks passed
@storopoli storopoli deleted the STR-2090-migrate-btcio-tests branch April 7, 2026 15:56
storopoli added a commit that referenced this pull request Apr 13, 2026
The btcio tests (added by PR #1451) were added to main after the rename
commit was written. The rebase didn't capture them, leaving orphaned
tracked files under functional-tests-new/tests/btcio/.
storopoli added a commit that referenced this pull request Apr 13, 2026
* chore: delete legacy functional-tests directory (STR-2085)

Remove the entire functional-tests/ directory. All valuable tests have been
migrated to functional-tests-new/ or their invariants tracked in JIRA/Notion.

Sync tests: added test_sync_genesis to functional-tests-new. Fullnode lag
restart test deferred — fullnode sync not yet implemented in new strata binary
(tracked in STR-2091).

Also:
- Fix bug in strata_seq_fullnode.py: pass genesis_l1.blk.height (int) not
  GenesisL1View object to create_node()
- Delete .github/workflows/functional.yml (legacy test workflow)
- Update test-coverage.yml, main-eest.yml, dependabot.yml, CODEOWNERS
- Update AGENTS.md, README.md, CONTRIBUTING.md references

* chore: rename functional-tests-new/ to functional-tests/ (STR-2085)

Now that the legacy functional-tests/ directory is deleted, drop the
-new suffix. Updates all references across CI workflows, justfile,
CODEOWNERS, Dockerfile, docs, and .dockerignore.

* chore: exclude functional-tests/_dd from taplo formatting

Test run data directory contains generated TOML files that should not
be linted.

* chore: clean up stray references after functional-tests rename (STR-2085)

- CODEOWNERS: drop fn_*.py and constants.py patterns that match nothing
  in the new layout.
- dependabot.yml: restore the uv ecosystem entry for functional-tests/.
  It was dropped with the legacy delete, but the path is unchanged after
  the rename so the new framework still needs Python dependency updates.
- functional.yml: filter the coverage-instrumented cargo build to the
  binaries the new tests actually use, and update the which assertion
  from strata-client to strata.
- FUNCTIONAL_TEST_MIGRATION_STATUS.md: delete the migration tracking
  doc now that the legacy tree is gone.

* fix: move btcio tests missed during functional-tests rename (STR-2085)

The btcio tests (added by PR #1451) were added to main after the rename
commit was written. The rebase didn't capture them, leaving orphaned
tracked files under functional-tests-new/tests/btcio/.

---------

Co-authored-by: Jose Storopoli <jose@storopoli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants