-
Notifications
You must be signed in to change notification settings - Fork 156
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
Changes from all commits
2024749
4c8ecb2
2c2c287
fac8a1f
8963226
4304326
a9040a9
cc85b4d
195df39
cf044ac
cada867
a868de8
File filter
Filter by extension
Conversations
Jump to
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
Contributor
Author
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. Codex: This matches my original intention when I first created this standalone script for the expensive electrs build step. It looks like we want to avoid building the binary ourselves anyway for easier local development, so I will likely be able to consolidate things again once a binary that supports
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.
Okay, yeah, not too strong of an opinion.
Well, given how close this PR is it would be nice to land it for 0.8 still, and not wait indefinitely. Hmm, given that the Forgejo migration is already around the corner, further CI changes will be necessary anyways soon. Maybe we could at least provide a convenience wrapper script here, that sets up bitcoind/electrsd and runs the tests? Or even better just do #660 (comment), i.e., leave the current test setup as is and only test 0fc in a separate new file and workflow, so we don't require the custom electrs for all local tests?
Contributor
Author
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. See the commit below titled "Isolate testing of 0FC channels to standalone CI" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #!/bin/bash | ||
|
benthecarman marked this conversation as resolved.
|
||
| 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.
Contributor
Author
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. This is indeed my intention, and I believe the script currently does this. See the tag here: https://github.com/tankyleo/blockstream-electrs/releases/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. Ah, sorry, I took
Contributor
Author
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. Done below, the tag remains useful to do a |
||
| 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| { | ||
|
Contributor
Author
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
Contributor
Author
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. See the esplora and electrum chain sources, I prefer to log the full response at trace even in the success case for debugging purposes. We get information on replaced transactions, transactions already in the mempool, and transactions freshly accepted into the mempool. |
||
|
|
||
| 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 | ||
|
|
||
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?