feat: add stacks testcontainer and deposit e2e test#1906
feat: add stacks testcontainer and deposit e2e test#1906
Conversation
| # If the snapshot is older than 2 hours the stacks node will complain. To | ||
| # prevent it, we generate a bitcoin block on startup. | ||
| bitcoin-new-block: |
There was a problem hiding this comment.
We should probably note the importance of sleep infinity at the end of the script.
sbtc/src/testing/containers.rs
Outdated
| let miner_faucet = | ||
| Faucet::new(STACKS_MINER_PRIVATE_KEY, AddressType::P2pkh, &rpc_client); | ||
| miner_faucet.track_address(None); | ||
| let amount = Amount::from_int_btc(49); | ||
| // Prepare multiple UTXOs for the faucet just in case | ||
| for _ in 0..5 { | ||
| miner_faucet.send_to(amount.to_sat(), &faucet.address); | ||
| } | ||
| faucet.generate_block(); |
There was a problem hiding this comment.
We spoke about this online and realized that there is a race here. Because we are using the stacks miner's UTXOs here, there is a possibility that there is a race between our code and the stacks-core. Stacks-core should be getting UTXO for PoX-commit transactions and we are sending stuff to our faucet. There are two ways to fix this:
- Sleep a little! Basically, have this code wait for some time, under the assumption that the stacks miner will be done by the time we resume.
- Have the bitcoin node mine some blocks to our faucet during the initialization phase.
The second one is cleaner but it might be more annoying than it seems. Good luck!
There was a problem hiding this comment.
I went with (2) since it was pretty easy and the less we mess with the Stacks miner the better, given it's already quite moody.
| for _ in 0..2 { | ||
| let tx = fund_stx(&stacks_client, &recipient, 1_000_000).await; |
There was a problem hiding this comment.
Can we check the account balance at the end too?
There was a problem hiding this comment.
Reworked a bit the test to only use balances
There was a problem hiding this comment.
Pull request overview
This PR adds Stacks testcontainer support for integration tests, enabling testing against a real Stacks node running in Epoch 3.0. The implementation uses pre-generated chainstate snapshots to avoid the time-consuming process of progressing through earlier epochs during test execution.
Changes:
- Added Docker Compose configurations for running Stacks node with pre-snapshotted chainstate
- Created bash script to generate chainstate snapshots at a specific block height
- Implemented helper functions and extension traits for Stacks and Bitcoin container interactions in tests
- Added end-to-end integration test demonstrating deposit flow with real Stacks node
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| docker/tests/docker-compose.stacks.yml | Main compose file for Stacks testing with snapshot loading |
| docker/tests/docker-compose.stacks.build.yml | Build-time compose for generating new snapshots |
| docker/tests/generate_snapshot.sh | Script to generate chainstate snapshots at target block height |
| docker/tests/README.md | Documentation for chainstate snapshot generation and usage |
| docker/tests/.gitignore | Ignores transient snapshot directory |
| docker/docker-compose.yml | Updates Stacks image version to 3.3.0.0.4 |
| docker/bitcoin/miner.sh | Adds configurable faucet address for snapshot generation |
| docker/stacks/stacks-test-miner.toml | Stacks node configuration for test environment |
| docker/tests/docker-compose.bitcoin.yml | Renames service from "bitcoind" to "bitcoin" for consistency |
| sbtc/src/testing/containers.rs | Adds StacksContainer type and helper methods for container interaction |
| sbtc/src/testing/regtest.rs | Adds stacks_address() method and async fee data generation |
| signer/tests/integration/containers.rs | Adds extension traits and tests for Bitcoin and Stacks containers |
| signer/tests/integration/stacks.rs | Helper functions for Stacks transaction creation and waiting |
| signer/tests/integration/e2e.rs | End-to-end deposit test with real Stacks node |
| signer/tests/integration/main.rs | Registers new test modules |
| signer/tests/integration/utxo_construction.rs | Refactors deposit creation to support custom recipients and handle dust |
| signer/tests/integration/transaction_coordinator.rs | Generalizes wait_for_signers function signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }]; | ||
|
|
||
| let change = utxo.amount() - Amount::from_sat(amount + fee); | ||
| if change.to_sat() > 546 { |
There was a problem hiding this comment.
The dust threshold value of 546 is hardcoded here. Consider defining this as a constant (e.g., DUST_THRESHOLD or MIN_CHANGE_OUTPUT) at the module level or importing it from the bitcoin crate if available, to make the code more maintainable and the magic number more self-documenting.
| let polling_fut = async { | ||
| while !predicate(stacks_client.get_account(address).await.unwrap().balance) { | ||
| tokio::time::sleep(STACKS_NODE_POLLING).await; | ||
| } | ||
| }; | ||
| polling_fut | ||
| .with_timeout(STACKS_NODE_TIMEOUT) | ||
| .await | ||
| .expect("failed to wait for stx balance"); | ||
| } | ||
|
|
||
| // Wait until the nonce of an address changes, or panic on timeout | ||
| pub async fn wait_for_new_nonce( | ||
| stacks_client: &StacksClient, | ||
| address: &StacksAddress, | ||
| old_nonce: u64, | ||
| ) { | ||
| let polling_fut = async { | ||
| while stacks_client.get_account(address).await.unwrap().nonce <= old_nonce { | ||
| tokio::time::sleep(STACKS_NODE_POLLING).await; | ||
| } |
There was a problem hiding this comment.
These functions use .unwrap() on await results without proper error handling. While this is test code, panicking with unclear error messages makes debugging difficult. Consider using .expect() with descriptive messages like "failed to get STX account balance" to provide better context when these operations fail.
| let change = utxo.amount() - Amount::from_sat(amount + fee); | ||
| if change.to_sat() > 546 { | ||
| tx_outs.push(TxOut { | ||
| value: change, | ||
| script_pubkey: depositor.address.script_pubkey(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The change handling for deposits is only included if it exceeds the dust limit (546 sats). However, there's no handling for the case where the change is positive but below the dust limit. In this case, those sats will be lost as fees. Consider adding a check that ensures the UTXO amount is sufficient to cover both the deposit amount, fee, and minimum dust threshold, or documenting this behavior clearly.
| assert_ge!(sbtc_balance.to_sat(), deposit_amount - max_fee); | ||
| assert_le!(sbtc_balance.to_sat(), deposit_amount); |
There was a problem hiding this comment.
Maybe add check that btc balance went down and sbtc balance didn't incresed before it should be increased ?
Description
Scaffolding and some tests for #1334
Changes
Add docker compose for stacks (and its bitcoin), and the scaffolding to use it.
The chainstate is generated via a bash script that runs a simil devenv compose stack and zip the volumes at the end. The chainstate zip is currently committed (it's relatively small, <5 MB); there's a readme with more info.
Most of the changes come from the new compose files and config, and the new e2e test.
Testing Information
Added a test checking that stacks does accept transactions, and an end to end test performing a deposit via Emily.
Checklist