Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 161
Zero-fee commitments support#660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
20247494c8ecb22c2c287fac8a1f89632264304326a9040a9cc85b4d195df39cf044accada867a868de8File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| name: CI Checks - 0FC Integration Tests | ||
| on: [push, pull_request] | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
| jobs: | ||
| build-and-test: | ||
| timeout-minutes: 60 | ||
| runs-on: self-hosted | ||
| steps: | ||
| - name: Checkout source code | ||
| uses: actions/checkout@v4 | ||
| - name: Install Rust stable toolchain | ||
| run: | | ||
| curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile=minimal --default-toolchain stable | ||
| - name: Enable caching for bitcoind | ||
| id: cache-bitcoind | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: bin/bitcoind-${{ runner.os }}-${{ runner.arch }} | ||
| key: bitcoind-29.0-${{ runner.os }}-${{ runner.arch }} | ||
| - name: Enable caching for electrs | ||
| id: cache-electrs | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: bin/electrs-${{ runner.os }}-${{ runner.arch }} | ||
| key: electrs-submit-package-${{ runner.os }}-${{ runner.arch }} | ||
| - name: Download bitcoind | ||
| if: "steps.cache-bitcoind.outputs.cache-hit != 'true'" | ||
| run: | | ||
| source ./scripts/download_bitcoind_electrs.sh | ||
| mkdir -p bin | ||
| mv "$BITCOIND_EXE" bin/bitcoind-${{ runner.os }}-${{ runner.arch }} | ||
| - name: Download electrs | ||
| if: "steps.cache-electrs.outputs.cache-hit != 'true'" | ||
| run: | | ||
| source ./scripts/build_electrs.sh | ||
| mkdir -p bin | ||
| mv "$ELECTRS_EXE" bin/electrs-${{ runner.os }}-${{ runner.arch }} | ||
| - name: Set bitcoind/electrs environment variables | ||
| run: | | ||
| echo "BITCOIND_EXE=$( pwd )/bin/bitcoind-${{ runner.os }}-${{ runner.arch }}" >> "$GITHUB_ENV" | ||
| echo "ELECTRS_EXE=$( pwd )/bin/electrs-${{ runner.os }}-${{ runner.arch }}" >> "$GITHUB_ENV" | ||
| - name: Test with 0FC enabled | ||
| run: | | ||
| RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test -- --test-threads=1 | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand on why this needs to be a separate script now? Why not keep using the old script that sets up both
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #!/bin/bash | ||
benthecarman marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| set -eox pipefail | ||
| # Our Esplora-based tests require `electrs` binaries. Here, we | ||
| # download the code, build the binaries, and export their location | ||
| # via `ELECTRS_EXE` which will be used by the `electrsd` crates in | ||
| # our tests. | ||
| HOST_PLATFORM="$(rustc --version --verbose | grep "host:" | awk '{ print $2 }')" | ||
| ELECTRS_GIT_REPO="https://github.com/tankyleo/blockstream-electrs.git" | ||
| ELECTRS_TAG="2026-05-26-electrum-submit-package" | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do this, we should use a specific commit revision, not point to a general branch.
| ||
| ELECTRS_REV="8c06d8010e43f793b1a65f83695ea846e5cd83ed" | ||
| if [[ "$HOST_PLATFORM" != *linux* && "$HOST_PLATFORM" != *darwin* ]]; then | ||
| printf "\n\n" | ||
| echo "Unsupported platform: $HOST_PLATFORM Exiting.." | ||
| exit 1 | ||
| fi | ||
| DL_TMP_DIR=$(mktemp -d) | ||
| trap 'rm -rf -- "$DL_TMP_DIR"' EXIT | ||
| pushd "$DL_TMP_DIR" | ||
| git clone --branch "$ELECTRS_TAG" --depth 1 "$ELECTRS_GIT_REPO" blockstream-electrs | ||
| cd blockstream-electrs | ||
| CURRENT_HEAD=$(git rev-parse HEAD) | ||
| if [ "$CURRENT_HEAD" != "$ELECTRS_REV" ]; then | ||
| echo "ERROR: HEAD does not match expected commit" | ||
| echo "expected: $ELECTRS_REV" | ||
| echo "actual: $CURRENT_HEAD" | ||
| exit 1 | ||
| fi | ||
| RUSTFLAGS="" cargo build | ||
| export ELECTRS_EXE="$DL_TMP_DIR"/blockstream-electrs/target/debug/electrs | ||
| chmod +x "$ELECTRS_EXE" | ||
| popd | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -14,6 +14,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; | ||
| use base64::prelude::BASE64_STANDARD; | ||
| use base64::Engine; | ||
| use bitcoin::transaction::Version; | ||
| use bitcoin::{BlockHash, FeeRate, Network, OutPoint, Transaction, Txid}; | ||
| use lightning::chain::chaininterface::ConfirmationTarget as LdkConfirmationTarget; | ||
| use lightning::chain::{BlockLocator, Listen}; | ||
| @@ -41,6 +42,7 @@ use crate::fee_estimator::{ | ||
| }; | ||
| use crate::io::utils::update_and_persist_node_metrics; | ||
| use crate::logger::{log_bytes, log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; | ||
| use crate::tx_broadcaster::SortedTransactions; | ||
| use crate::types::{ChainMonitor, ChannelManager, DynStore, Sweeper, Wallet}; | ||
| use crate::{Error, PersistedNodeMetrics}; | ||
| @@ -119,6 +121,30 @@ impl BitcoindChainSource { | ||
| self.api_client.utxo_source() | ||
| } | ||
| pub(super) async fn validate_zero_fee_commitments_support(&self) -> Result<(), Error> { | ||
| let node_version_result = tokio::time::timeout( | ||
| Duration::from_secs(CHAIN_POLLING_TIMEOUT_SECS), | ||
| self.api_client.get_node_version(), | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| log_error!(self.logger, "Failed to get node version: {:?}", e); | ||
| Error::ConnectionFailed | ||
| })?; | ||
| let node_version = node_version_result.map_err(|e| { | ||
| log_error!(self.logger, "Failed to get node version: {:?}", e); | ||
| Error::ConnectionFailed | ||
| })?; | ||
| // v26 first shipped the `submitpackage` RPC, but we need v29 to relay ephemeral dust | ||
| if node_version < 290000 { | ||
| log_error!(self.logger, "Bitcoin backend MUST be greater than or equal to v29"); | ||
| return Err(Error::ChainSourceNotSupported); | ||
| } | ||
| Ok(()) | ||
| } | ||
| pub(super) async fn continuously_sync_wallets( | ||
| &self, mut stop_sync_receiver: tokio::sync::watch::Receiver<()>, | ||
| onchain_wallet: Arc<Wallet>, channel_manager: Arc<ChannelManager>, | ||
| @@ -571,46 +597,59 @@ impl BitcoindChainSource { | ||
| Ok(()) | ||
| } | ||
| pub(crate) async fn process_broadcast_package(&self, package: Vec<Transaction>) { | ||
| // While it's a bit unclear when we'd be able to lean on Bitcoin Core >v28 | ||
| // features, we should eventually switch to use `submitpackage` via the | ||
| // `rust-bitcoind-json-rpc` crate rather than just broadcasting individual | ||
| // transactions. | ||
| for tx in &package { | ||
| let txid = tx.compute_txid(); | ||
| let timeout_fut = tokio::time::timeout( | ||
| Duration::from_secs(DEFAULT_TX_BROADCAST_TIMEOUT_SECS), | ||
| self.api_client.broadcast_transaction(tx), | ||
| ); | ||
| match timeout_fut.await { | ||
| Ok(res) => match res { | ||
| Ok(id) => { | ||
| debug_assert_eq!(id, txid); | ||
| log_trace!(self.logger, "Successfully broadcast transaction {}", txid); | ||
| }, | ||
| Err(e) => { | ||
| log_error!(self.logger, "Failed to broadcast transaction {}: {}", txid, e); | ||
| log_trace!( | ||
| self.logger, | ||
| "Failed broadcast transaction bytes: {}", | ||
| log_bytes!(tx.encode()) | ||
| ); | ||
| fn log_broadcast_error( | ||
| &self, e: impl core::fmt::Display, txids: &[Txid], txs: &SortedTransactions, | ||
| ) { | ||
| log_error!(self.logger, "Failed to broadcast transaction(s) {:?}: {}", txids, e); | ||
| log_trace!(self.logger, "Failed broadcast transaction bytes:"); | ||
| for tx in txs.iter() { | ||
| log_trace!(self.logger, "{}", log_bytes!(tx.encode())); | ||
| } | ||
| } | ||
| pub(crate) async fn process_transaction_broadcast(&self, txs: SortedTransactions) { | ||
| let all_txs_are_v3 = txs.iter().all(|tx| tx.version == Version::non_standard(3)); | ||
| match txs.len() { | ||
| 2.. if all_txs_are_v3 => { | ||
| let txids: Vec<_> = txs.iter().map(|tx| tx.compute_txid()).collect(); | ||
| let timeout_fut = tokio::time::timeout( | ||
| Duration::from_secs(DEFAULT_TX_BROADCAST_TIMEOUT_SECS), | ||
| self.api_client.submit_package(&txs), | ||
| ); | ||
| match timeout_fut.await { | ||
| Ok(res) => match res { | ||
| Ok(result) => { | ||
| log_trace!(self.logger, "Successfully broadcast package {:?}", txids); | ||
| log_trace!(self.logger, "Successfully broadcast package {}", result); | ||
| }, | ||
| Err(e) => self.log_broadcast_error(e, &txids, &txs), | ||
| }, | ||
| }, | ||
| Err(e) => { | ||
| log_error!( | ||
| self.logger, | ||
| "Failed to broadcast transaction due to timeout {}: {}", | ||
| txid, | ||
| e | ||
| ); | ||
| log_trace!( | ||
| self.logger, | ||
| "Failed broadcast transaction bytes: {}", | ||
| log_bytes!(tx.encode()) | ||
| Err(e) => self.log_broadcast_error(e, &txids, &txs), | ||
| } | ||
| }, | ||
| _ => { | ||
| for tx in txs.iter() { | ||
| let txid = tx.compute_txid(); | ||
| let timeout_fut = tokio::time::timeout( | ||
| Duration::from_secs(DEFAULT_TX_BROADCAST_TIMEOUT_SECS), | ||
| self.api_client.broadcast_transaction(tx), | ||
| ); | ||
| }, | ||
| } | ||
| match timeout_fut.await { | ||
| Ok(res) => match res { | ||
| Ok(id) => { | ||
| debug_assert_eq!(id, txid); | ||
| log_trace!( | ||
| self.logger, | ||
| "Successfully broadcast transaction {}", | ||
| txid | ||
| ); | ||
| }, | ||
| Err(e) => self.log_broadcast_error(e, &[txid], &txs), | ||
| }, | ||
| Err(e) => self.log_broadcast_error(e, &[txid], &txs), | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| @@ -748,6 +787,31 @@ impl BitcoindClient { | ||
| } | ||
| } | ||
| pub(crate) async fn get_node_version(&self) -> Result<u64, BitcoindClientError> { | ||
| match self { | ||
| BitcoindClient::Rpc { rpc_client, .. } => { | ||
| Self::get_node_version_inner(Arc::clone(rpc_client)) | ||
| .await | ||
| .map_err(BitcoindClientError::Rpc) | ||
| }, | ||
| BitcoindClient::Rest { rpc_client, .. } => { | ||
| // Bitcoin Core's REST interface does not support `getnetworkinfo` | ||
| // so we use the RPC client. | ||
| Self::get_node_version_inner(Arc::clone(rpc_client)) | ||
| .await | ||
| .map_err(BitcoindClientError::Rpc) | ||
| }, | ||
| } | ||
| } | ||
| async fn get_node_version_inner(rpc_client: Arc<RpcClient>) -> Result<u64, RpcClientError> { | ||
| rpc_client.call_method::<serde_json::Value>("getnetworkinfo", &[]).await.and_then(|value| { | ||
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can also look into properly parsing the return value here like how we did for | ||
| value["version"].as_u64().ok_or(RpcClientError::InvalidData(String::from( | ||
| "The version field in the `getnetworkinfo` response should be a u64", | ||
| ))) | ||
| }) | ||
| } | ||
| /// Broadcasts the provided transaction. | ||
| pub(crate) async fn broadcast_transaction( | ||
| &self, tx: &Transaction, | ||
| @@ -776,6 +840,38 @@ impl BitcoindClient { | ||
| rpc_client.call_method::<Txid>("sendrawtransaction", &[tx_json]).await | ||
| } | ||
| /// Submits the provided package | ||
| pub(crate) async fn submit_package( | ||
| &self, package: &SortedTransactions, | ||
| ) -> Result<String, BitcoindClientError> { | ||
| match self { | ||
| BitcoindClient::Rpc { rpc_client, .. } => { | ||
| Self::submit_package_inner(Arc::clone(rpc_client), package) | ||
| .await | ||
| .map_err(BitcoindClientError::Rpc) | ||
| }, | ||
| BitcoindClient::Rest { rpc_client, .. } => { | ||
| // Bitcoin Core's REST interface does not support submitting packages | ||
| // so we use the RPC client. | ||
| Self::submit_package_inner(Arc::clone(rpc_client), package) | ||
| .await | ||
| .map_err(BitcoindClientError::Rpc) | ||
| }, | ||
| } | ||
| } | ||
| async fn submit_package_inner( | ||
| rpc_client: Arc<RpcClient>, package: &SortedTransactions, | ||
| ) -> Result<String, RpcClientError> { | ||
| let package_serialized: Vec<_> = | ||
| package.iter().map(|tx| bitcoin::consensus::encode::serialize_hex(tx)).collect(); | ||
| let package_json = serde_json::json!(package_serialized); | ||
| rpc_client | ||
| .call_method::<SubmitPackageResponse>("submitpackage", &[package_json]) | ||
| .await | ||
| .map(|response| response.0) | ||
| } | ||
| /// Retrieve the fee estimate needed for a transaction to begin | ||
| /// confirmation within the provided `num_blocks`. | ||
| pub(crate) async fn get_fee_estimate_for_target( | ||
| @@ -1327,6 +1423,23 @@ impl TryInto<GetMempoolEntryResponse> for JsonResponse { | ||
| } | ||
| } | ||
| pub struct SubmitPackageResponse(String); | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we even return the full response string here rather than
| ||
| impl TryInto<SubmitPackageResponse> for JsonResponse { | ||
| type Error = String; | ||
| fn try_into(self) -> Result<SubmitPackageResponse, String> { | ||
| let response = self.0.to_string(); | ||
| let res = self.0.as_object().ok_or("Failed to parse submitpackage response".to_string())?; | ||
| match res["package_msg"].as_str() { | ||
| Some("success") => Ok(SubmitPackageResponse(response)), | ||
| Some(_) | None => { | ||
| return Err(response); | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| #[derive(Debug, Clone)] | ||
| pub(crate) struct MempoolEntry { | ||
| /// The transaction id | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Maybe we could also add interop test coverage as a follow-up? Presumably with Eclair it should work?