Skip to content
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

Update bitcoin + ldk + secpzkp #197

Merged
merged 1 commit into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ members = [
"dlc-sled-storage-provider",
"electrs-blockchain-provider",
]

resolver = "2"
9 changes: 5 additions & 4 deletions bitcoin-rpc-provider/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ name = "bitcoin-rpc-provider"
version = "0.1.0"

[dependencies]
bitcoin = {version = "0.29.2"}
bitcoincore-rpc = {version = "0.16.0"}
bitcoincore-rpc-json = {version = "0.16.0"}
bitcoin = {version = "0.30.2"}
bitcoincore-rpc = {version = "0.17.0"}
bitcoincore-rpc-json = {version = "0.17.0"}
dlc-manager = {path = "../dlc-manager"}
lightning = { version = "0.0.118" }
hex = { package = "hex-conservative", version = "0.1" }
lightning = { version = "0.0.121" }
log = "0.4.14"
rust-bitcoin-coin-selection = { version = "0.1.0", git = "https://github.com/p2pderivatives/rust-bitcoin-coin-selection", rev = "405451929568422f7df809e35d6ad8f36fccce90", features = ["rand"] }
simple-wallet = {path = "../simple-wallet"}
50 changes: 27 additions & 23 deletions bitcoin-rpc-provider/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,27 @@
//! # Bitcoin rpc provider

use std::cmp::max;
use std::collections::HashMap;
use std::sync::atomic::{AtomicU32, Ordering};
use std::sync::{Arc, Mutex};
use std::time::Duration;

use bitcoin::address::NetworkUnchecked;
use bitcoin::consensus::encode::Error as EncodeError;
use bitcoin::hashes::hex::ToHex;
use bitcoin::hashes::serde;
use bitcoin::psbt::PartiallySignedTransaction;
use bitcoin::secp256k1::rand::thread_rng;
use bitcoin::secp256k1::{PublicKey, SecretKey};
use bitcoin::{
consensus::Decodable, network::constants::Network, Amount, PrivateKey, Script, Transaction,
Txid,
consensus::Decodable, network::constants::Network, Amount, PrivateKey, Transaction, Txid,
};
use bitcoin::{Address, OutPoint, TxOut};
use bitcoin::{Address, OutPoint, ScriptBuf, TxOut};
use bitcoincore_rpc::jsonrpc::serde_json;
use bitcoincore_rpc::jsonrpc::serde_json::Value;
use bitcoincore_rpc::{json, Auth, Client, RpcApi};
use bitcoincore_rpc_json::AddressType;
use dlc_manager::error::Error as ManagerError;
use dlc_manager::{Blockchain, ContractSignerProvider, SimpleSigner, Utxo, Wallet};
use hex::DisplayHex;
use json::EstimateMode;
use lightning::chain::chaininterface::{ConfirmationTarget, FeeEstimator};
use log::error;
Expand Down Expand Up @@ -108,10 +107,6 @@ impl BitcoinCoreProvider {
let client = Arc::new(Mutex::new(rpc_client));
let mut fees: HashMap<ConfirmationTarget, AtomicU32> = HashMap::with_capacity(7);
fees.insert(ConfirmationTarget::OnChainSweep, AtomicU32::new(5000));
fees.insert(
ConfirmationTarget::MaxAllowedNonAnchorChannelRemoteFee,
AtomicU32::new(25 * 250),
);
fees.insert(
ConfirmationTarget::MinAllowedAnchorChannelRemoteFee,
AtomicU32::new(MIN_FEERATE),
Expand Down Expand Up @@ -184,9 +179,9 @@ impl ContractSignerProvider for BitcoinCoreProvider {
.client
.lock()
.unwrap()
.call::<HashMap<Address, Value>>(
.call::<HashMap<Address<NetworkUnchecked>, Value>>(
"getaddressesbylabel",
&[Value::String(keys_id.to_hex())],
&[Value::String(keys_id.to_lower_hex_string())],
)
.map_err(rpc_err_to_manager_err)?;

Expand All @@ -199,7 +194,7 @@ impl ContractSignerProvider for BitcoinCoreProvider {
.client
.lock()
.unwrap()
.dump_private_key(address)
.dump_private_key(&address.clone().assume_checked())
.map_err(rpc_err_to_manager_err)?;
Ok(SimpleSigner::new(sk.inner))
} else {
Expand All @@ -214,7 +209,7 @@ impl ContractSignerProvider for BitcoinCoreProvider {
network,
inner: sk,
},
Some(&keys_id.to_hex()),
Some(&keys_id.to_lower_hex_string()),
Some(false),
)
.map_err(rpc_err_to_manager_err)?;
Expand Down Expand Up @@ -263,22 +258,26 @@ impl ContractSignerProvider for BitcoinCoreProvider {

impl Wallet for BitcoinCoreProvider {
fn get_new_address(&self) -> Result<Address, ManagerError> {
self.client
Ok(self
.client
.lock()
.unwrap()
.get_new_address(None, Some(AddressType::Bech32))
.map_err(rpc_err_to_manager_err)
.map_err(rpc_err_to_manager_err)?
.assume_checked())
}

fn get_new_change_address(&self) -> Result<Address, ManagerError> {
self.client
Ok(self
.client
.lock()
.unwrap()
.call(
.call::<Address<NetworkUnchecked>>(
"getrawchangeaddress",
&[Value::Null, opt_into_json(Some(AddressType::Bech32))?],
)
.map_err(rpc_err_to_manager_err)
.map_err(rpc_err_to_manager_err)?
.assume_checked())
}

fn get_utxos_for_amount(
Expand All @@ -304,8 +303,16 @@ impl Wallet for BitcoinCoreProvider {
txid: x.txid,
vout: x.vout,
},
address: x.address.as_ref().ok_or(Error::InvalidState)?.clone(),
redeem_script: x.redeem_script.as_ref().unwrap_or(&Script::new()).clone(),
address: x
.address
.as_ref()
.map(|x| x.clone().assume_checked())
Copy link
Collaborator

@luckysori luckysori Feb 9, 2024

Choose a reason for hiding this comment

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

Should we use https://docs.rs/bitcoin/latest/bitcoin/address/struct.Address.html#method.require_network instead? It might be a bit annoying to adapt the code base, but consumers are probably only gonna use an instance of Manager, for example, for one particular Network at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can and I thought of it. But then it'd be like asking the node who just sent you an address to also tell you its network. I guess it'd be a double check that the node is not doing something weird, but it doesn't feel very useful to me. I can add it though let me know if you think it's useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's low priority and I would only do this in a separate PR.

But I do think it can be useful in some spots. What if one side uses a testnet change address by mistake, when they're actually setting up a mainnet DLC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which spots are you referring to? I think in this particular place, it wouldn't change anything. I think all the other places are in test code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right, this is the only place where we receive an Address in production code.

I assumed that there were more spots in the code base 😅

We can and I thought of it. But then it'd be like asking the node who just sent you an address to also tell you its network. I guess it'd be a double check that the node is not doing something weird, but it doesn't feel very useful to me.

Wouldn't it be more like making sure that the address returned by the node corresponds to the expected network? As in, you would receive the address and then check that its Network matches the one stored in BitcoinCoreProvider. This might prevent some weirdness/bugs if someone intends to run this component for mainnet, but they point to a testnet node instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So IIUC what you're proposing is that the constructor takes a Network parameter? I think that's fine but I wonder if then it wouldn't be easier to just check within the constructor that the node corresponds to the expected network and then forget about it.

.ok_or(Error::InvalidState)?,
redeem_script: x
.redeem_script
.as_ref()
.cloned()
.unwrap_or(ScriptBuf::new()),
reserved: false,
}))
})
Expand Down Expand Up @@ -536,9 +543,6 @@ fn poll_for_fee_estimates(
fees.get(&ConfirmationTarget::OnChainSweep)
.unwrap()
.store(fee_rate, Ordering::Release);
fees.get(&ConfirmationTarget::MaxAllowedNonAnchorChannelRemoteFee)
.unwrap()
.store(max(25 * 250, fee_rate * 10), Ordering::Release);
}
Err(e) => {
error!("Error querying fee estimate: {}", e);
Expand Down
6 changes: 3 additions & 3 deletions bitcoin-test-utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ name = "bitcoin-test-utils"
version = "0.1.0"

[dependencies]
bitcoin = { version = "0.29.2", default-features = false }
bitcoincore-rpc = {version = "0.16"}
bitcoincore-rpc-json = {version = "0.16"}
bitcoin = { version = "0.30.2", default-features = false }
bitcoincore-rpc = {version = "0.17"}
bitcoincore-rpc-json = {version = "0.17"}
9 changes: 6 additions & 3 deletions bitcoin-test-utils/src/rpc_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,16 @@ pub fn init_clients() -> (Client, Client, Client) {

let offer_address = offer_rpc
.get_new_address(None, Some(AddressType::Bech32))
.unwrap();
.unwrap()
.assume_checked();
let accept_address = accept_rpc
.get_new_address(None, Some(AddressType::Bech32))
.unwrap();
.unwrap()
.assume_checked();
let sink_address = sink_rpc
.get_new_address(None, Some(AddressType::Bech32))
.unwrap();
.unwrap()
.assume_checked();

sink_rpc.generate_to_address(1, &offer_address).unwrap();
sink_rpc.generate_to_address(1, &accept_address).unwrap();
Expand Down
13 changes: 7 additions & 6 deletions dlc-manager/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,28 +17,29 @@ use-serde = ["serde", "dlc/use-serde", "dlc-messages/serde", "dlc-trie/use-serde

[dependencies]
async-trait = "0.1.50"
bitcoin = { version = "0.29.2", default-features = false }
bitcoin = { version = "0.30.2", default-features = false }
dlc = { version = "0.4.0", default-features = false, path = "../dlc" }
dlc-messages = { version = "0.4.0", default-features = false, path = "../dlc-messages" }
dlc-trie = { version = "0.4.0", default-features = false, path = "../dlc-trie" }
lightning = { version = "0.0.118", default-features = false, features = ["grind_signatures"] }
hex = { package = "hex-conservative", version = "0.1" }
lightning = { version = "0.0.121", default-features = false, features = ["grind_signatures"] }
log = "0.4.14"
rand_chacha = {version = "0.3.1", optional = true}
secp256k1-zkp = {version = "0.7.0"}
secp256k1-zkp = {version = "0.9.2"}
serde = {version = "1.0", optional = true}

[dev-dependencies]
bitcoin-rpc-provider = {path = "../bitcoin-rpc-provider"}
bitcoin-test-utils = {path = "../bitcoin-test-utils"}
bitcoincore-rpc = {version = "0.16.0"}
bitcoincore-rpc-json = {version = "0.16.0"}
bitcoincore-rpc = {version = "0.17"}
bitcoincore-rpc-json = {version = "0.17"}
criterion = "0.4.0"
dlc-manager = { path = ".", default-features = false, features = ["use-serde"] }
dlc-messages = { path = "../dlc-messages", default-features = false, features = ["serde"] }
electrs-blockchain-provider = {path = "../electrs-blockchain-provider"}
env_logger = "0.9.1"
mocks = {path = "../mocks"}
secp256k1-zkp = {version = "0.7.0", features = ["bitcoin_hashes", "rand", "rand-std", "global-context", "use-serde"]}
secp256k1-zkp = {version = "0.9.2", features = ["bitcoin_hashes", "rand", "rand-std", "global-context", "serde"]}
serde = "1.0"
serde_json = "1.0"
simple-wallet = {path = "../simple-wallet"}
Expand Down
6 changes: 3 additions & 3 deletions dlc-manager/src/channel/accepted_channel.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! # Structure and methods for channels that have been accepted.

use bitcoin::{Script, Transaction};
use bitcoin::{ScriptBuf, Transaction};
use dlc_messages::channel::AcceptChannel;
use secp256k1_zkp::{EcdsaAdaptorSignature, PublicKey};

Expand Down Expand Up @@ -29,7 +29,7 @@ pub struct AcceptedChannel {
/// The buffer transaction for the initial contract in the channel.
pub buffer_transaction: Transaction,
/// The script pubkey of the buffer transaction output.
pub buffer_script_pubkey: Script,
pub buffer_script_pubkey: ScriptBuf,
/// The temporary id of the channel.
pub temporary_channel_id: ChannelId,
/// The actual id of the channel.
Expand All @@ -53,7 +53,7 @@ impl AcceptedChannel {
funding_pubkey: contract.accept_params.fund_pubkey,
payout_spk: contract.accept_params.payout_script_pubkey.clone(),
payout_serial_id: contract.accept_params.payout_serial_id,
funding_inputs: contract.funding_inputs.iter().map(|x| x.into()).collect(),
funding_inputs: contract.funding_inputs.clone(),
change_spk: contract.accept_params.change_script_pubkey.clone(),
change_serial_id: contract.accept_params.change_serial_id,
cet_adaptor_signatures: cet_adaptor_signatures.into(),
Expand Down
1 change: 1 addition & 0 deletions dlc-manager/src/channel/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub mod offered_channel;
pub mod party_points;
pub mod ser;
pub mod signed_channel;
mod utils;

/// Enumeration containing the possible state a DLC channel can be in.
#[derive(Clone)]
Expand Down
12 changes: 2 additions & 10 deletions dlc-manager/src/channel/offered_channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,7 @@ impl OfferedChannel {
payout_spk: offered_contract.offer_params.payout_script_pubkey.clone(),
payout_serial_id: offered_contract.offer_params.payout_serial_id,
offer_collateral: offered_contract.offer_params.collateral,
funding_inputs: offered_contract
.funding_inputs_info
.iter()
.map(|x| x.into())
.collect(),
funding_inputs: offered_contract.funding_inputs.clone(),
change_spk: offered_contract.offer_params.change_script_pubkey.clone(),
change_serial_id: offered_contract.offer_params.change_serial_id,
cet_locktime: offered_contract.cet_locktime,
Expand Down Expand Up @@ -123,11 +119,7 @@ impl OfferedChannel {
refund_locktime: offer_channel.refund_locktime,
fee_rate_per_vb: offer_channel.fee_rate_per_vb,
fund_output_serial_id: offer_channel.fund_output_serial_id,
funding_inputs_info: offer_channel
.funding_inputs
.iter()
.map(|x| x.into())
.collect(),
funding_inputs: offer_channel.funding_inputs.clone(),
total_collateral: offer_channel.contract_info.get_total_collateral(),
keys_id,
};
Expand Down
2 changes: 1 addition & 1 deletion dlc-manager/src/channel/party_points.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
//! of states possible. This module contain a structure containing them and methods
//! useful for derivation.

use super::utils::{derive_public_key, derive_public_revocation_key};
use bitcoin::PublicKey as BitcoinPublicKey;
use dlc::channel::RevokeParams;
use lightning::ln::chan_utils::{derive_public_key, derive_public_revocation_key};
use secp256k1_zkp::{All, PublicKey, Secp256k1, Signing, Verification};

/// Base points used by a party of a DLC channel to derive public and private
Expand Down
8 changes: 4 additions & 4 deletions dlc-manager/src/channel/signed_channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//! transaction inputs. This module contains the model for a signed channel,
//! the possible states in which it can be as well as methods to work with it.

use bitcoin::{Script, Transaction, Txid};
use bitcoin::{ScriptBuf, Transaction, Txid};
use dlc::PartyParams;
use dlc_messages::oracle_msgs::OracleAttestation;
use lightning::ln::chan_utils::CounterpartyCommitmentSecrets;
Expand Down Expand Up @@ -230,7 +230,7 @@ typed_enum!(
/// The buffer transaction.
buffer_transaction: Transaction,
/// The buffer transaction script pubkey.
buffer_script_pubkey: Script,
buffer_script_pubkey: ScriptBuf,
/// The adaptor signature for the buffer transaction generated by
/// the accept party.
accept_buffer_adaptor_signature: EcdsaAdaptorSignature,
Expand All @@ -256,7 +256,7 @@ typed_enum!(
/// The buffer transaction.
buffer_transaction: Transaction,
/// The buffer transaction script pubkey.
buffer_script_pubkey: Script,
buffer_script_pubkey: ScriptBuf,
/// The adaptor signature for the buffer transaction generated by
/// the offer party.
offer_buffer_adaptor_signature: EcdsaAdaptorSignature,
Expand Down Expand Up @@ -398,7 +398,7 @@ pub struct SignedChannel {
/// The fund transaction for the channel.
pub fund_tx: Transaction,
/// The script pubkey for the funding output.
pub fund_script_pubkey: Script,
pub fund_script_pubkey: ScriptBuf,
/// The vout of the funding output.
pub fund_output_index: usize,
/// The latest "stable" state in which the channel was (if already in a "stable")
Expand Down
Loading
Loading