Ecdsa adaptor dlc #2

Tibo-lg · 2020-08-27T06:52:00Z

This PR implements function for creating, signing and verifying signature for ecdsa adaptor based DLC.

First commit is the interesting part, second commit adds some integration tests.

nkohen

Looking good!

nkohen · 2020-08-31T19:21:59Z

dlc/Cargo.toml

+bitcoin = {version="0.23.0-adaptor.0", git = "https://github.com/tibo-lg/rust-bitcoin", branch = "ecdsa-adaptor", package="bitcoin"}
+secp256k1 = {version="0.17.3-adaptor.0", git = "https://github.com/tibo-lg/rust-secp256k1", branch = "ecdsa-adaptor", package="secp256k1"}


Do you think it would be worth forking these two repos to be under p2pderivatives?

I moved all the repos under p2pderivatives now.

nkohen · 2020-08-31T19:23:37Z

dlc/src/util.rs

+use bitcoin::{PrivateKey, Script, SigHashType, Transaction};
+use secp256k1::{Message, Secp256k1, Signature, Signing};
+
+pub(crate) fn get_sig_hash_msg(


some comment docs for functions in this file would be nice, even just one-liners. I'm able to figure it out without too much effort but my rust isn't that great so I think it would be easier for people like me with a little extra help :)

Added some comments I hope it helps.

Comment certainly helps, though I think it might even be nice to link out to like https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki or whatever spec this follows

nkohen · 2020-08-31T19:24:58Z

integration_test/Cargo.toml

@@ -0,0 +1,12 @@
+[package]
+name = "integration_test"


I don't know much about cargo but perhaps this would be better named dlc_integration_test? Unless Rust figures out on its own what this is testing

Not sure what you mean here? I don't think Rust cares what this is testing. It's not used with cargo test, this is just a separate crate that generates a binary that is then run in the CI. I don't mind renaming if you think it is better but just wanted to make sure I understand why :)

nkohen · 2020-08-31T19:40:15Z

dlc/src/lib.rs

+const P2WPKH_INPUT_SIZE: usize = 31;
+const P2WSH_INPUT_SIZE: usize = 43;


I'm a little confused here, these both seem to be the spks plus 11 bytes but I can't figure out what the 11 bytes are

Wow I'm quite confused as well I have no idea what I had done there or were these numbers came from (I refactored fee computation at the last minutes, was probably not a good idea...). I changed to simplify and only handle P2WPKH for now, I hope it makes more sense.

nkohen · 2020-08-31T19:43:27Z

dlc/src/lib.rs

+/// Contains the parameters required for creating DLC transactions for a single
+/// party
+pub struct PartyParams {


nit: I think this comment could benefit from the addition: "Specifically these are the common fields between Offer and Accept messages."

nkohen · 2020-08-31T19:57:47Z

dlc/src/lib.rs

+    pub local: u64,
+    /// Payout for the remote party
+    pub remote: u64,


Probably worth a comment on this file somewhere saying that local is initiator and remote is accepter (seeing as this is currently a requirement for ordering purposes and such in the spec)

nkohen · 2020-08-31T19:58:38Z

dlc/src/lib.rs

+            local_change_output,
+            remote_change_output,
+        ];
+        outs.into_iter().filter(|o| o.value >= DUST_LIMIT).collect()


For CPFP reasons, I believe this must actually be a requirement, not a filter

I believe it is still the case that to be compatible with the current spec (subject to change) all parties must have change outputs

I thought quite a bit about that, but I prefer not to enforce this at that level while I don't have any way to deal with it at the higher level.

Perhaps this can be dealt with at the coin selection layer

Yes I think so too, and maybe once it's dealt with there maybe I can enforce it here.

nkohen · 2020-08-31T20:01:26Z

dlc/src/lib.rs

+    Ok(secp.adaptor_sign(&sig_hash, &funding_sk.key, &adaptor_point))
+}
+
+/// Sign the given ce using own private key, adapt the counter party signature


nit: ce -> cet

Thanks fixed

weirdly enough, it still shows up as ce for me?

oups probably a rebase gone wrong

nkohen · 2020-08-31T20:02:57Z

dlc/src/lib.rs

+) -> Result<(), Error> {
+    let (_, adaptor_secret) = oracle_signature.decompose()?;
+
+    let other_sig = secp.adaptor_adapt(&adaptor_secret, adaptor_signature);


nit: complete_sig might be a nicer name?

complete_full_sig sounded weird to me, so I went with adapted_sig does that sound alright?

nkohen · 2020-08-31T20:08:25Z

integration_test/src/main.rs

+
+    generate_blocks(1);
+
+    assert!(local_rpc.send_raw_transaction(&dlc_txs.cets[0]).is_ok());


might be worthwhile to test the timelock enforcement on this one by setting it to something specific and then generating to that height (making a call to this with .expect first and then an assert after)

Yeah timelock was definitely not working thanks... Fixed and added a test for that.

ariard

I think you can verify the code related to nLocktime/nSequence. I'm not sure if it enforces the wished semantic.

ariard · 2020-09-08T01:10:24Z

dlc/src/lib.rs

+
+pub mod util;
+
+const DUST_LIMIT: u64 = 1000;


Can you add a generic comment on constant origin from dlcspecs to hint the newcomer on their meaning ?

I added comments for these consts. Since the descriptions in the specs are not merged yet I just added a TODO for now, I can put some links later.

ariard · 2020-09-08T01:20:07Z

dlc/src/lib.rs

+const P2WPKH_WITNESS_SIZE: usize = 108;
+
+const DISABLE_LOCKTIME: u32 = 0xffffffff;
+const ENABLE_LOCKTIME: u32 = 0xfffffffe;


Maybe comment to the consensus interdependency between nSequence and nLocktime, namely setting nSequence == SEQUENCE_FINAL disable nLocktime enforcement.

Added, let me know if that looks good or not

ariard · 2020-09-08T01:21:42Z

dlc/src/lib.rs

+
+/// Represents the payouts for a unique contract outcome. Local party represents
+/// the initiator of the contract while remote party represents the party
+/// accepting the contract.


FYI we're getting right of local/remote nomenclature in RL, too confusing when you're locally building the transactions for the remote side. We picked up holder/counterparty.

In fact that's confusing because in your nomenclature local isn't the current user of this software...

I agree it's quite confusing, we used that perviously in the specs, but I don't know if we have decided on something else yet (I could see @nkohen seem not to be using it anymore in bitcoin-s implementation). Added a TODO for now and will do the renaming once something is decided.

I personally ended up using offer and accept in most places and only used local and remote when it came to the places where that made sense (only in signing pretty much)

So for example Payout would have offerPayout: u64, acceptPayout: u64 or something like that

Replaced with offer and accept

ariard · 2020-09-08T01:24:00Z

dlc/src/lib.rs

+    /// The fund transaction locking both parties collaterals
+    pub fund: Transaction,
+    /// The contract execution transactions for closing the contract on a
+    /// certain outcome


Outcome designates both event outcome and balance outcome. Maybe above Outcome -> BalanceOutcome ?

I agree the name was not great. Changed to Payout as I think it matches well but let me know what you think.

ariard · 2020-09-08T01:25:32Z

dlc/src/lib.rs

+
+/// The type of an input used for funding a DLC contract. Note that it must be
+/// a segwit input type.
+pub enum InputType {


Maybe SegwitInput and remove comment. Or mark that non-segwit are malleable and thus would break the trustless chain of pre-signed transaction

Changed to SegwitInput and removed comment.

ariard · 2020-09-08T01:52:15Z

dlc/src/lib.rs

+    assert_eq!(
+        total_collateral + local_cet_fee + remote_cet_fee,
+        fund_output_value
+    );


Good assert also local_params.input_amount + remote_params.input_amount == fund_output_value + local_change_value + remote_change_value + local_fund_fee + remote_fund_fee ?

ariard · 2020-09-08T01:56:08Z

dlc/src/lib.rs

+        previous_output: fund_outpoint,
+        witness: Vec::new(),
+        script_sig: Script::new(),
+        sequence: 0xffffffff,


If you want the nLocktime to be enforced, nSequence must be != SEQUENCE_FINAL ?

Yeah there was definitely a bug here thanks! I would like to add something in the integration tests to check this, but since I'm using the same nLockTime for fund tx and cet I'm not sure how to do it. Would it make sense to have two parameters?

I would lean towards two parameters unless it's causing other issue ?

Two parameters added (#2 (comment)) and both tested now

ariard · 2020-09-08T01:58:07Z

dlc/src/lib.rs

+        lock_time: lock_time,
+        input,
+        output,
+    };


Assert 3 outputs ? No more no less ?

As I mentioned here I'm not sure we should enforce change output for v1. But if you convince me for the specs I can easily make the change here :)

Depends. How easy it should be to break a V0.1 DLC :) ?

I still haven't been convinced that having the change output provides more than convenience, if it is a security issue I am happy to enforce it (and deal with the extra burden), but otherwise I fund it more inconvenient because:

It complicates coin selection. If it finds a UTXO that satisfies the collateral amount, I actually need to re-run with a different amount until I find something that actually have a change

It increases transaction size and so fee

Increases UTXO set of user

ariard · 2020-09-08T02:02:32Z

dlc/src/util.rs

+
+/// Convert a raw signature to DER encoded and append the sighash type, to use
+/// a signature in a signature script
+pub(crate) fn sig_to_full_sig(sig: &Signature, sig_hash_type: SigHashType) -> Vec<u8> {


finalize_sig. If we don't have this in rust-bitcoin, maybe add it ?

Renamed. I checked in rust-bitcoin but couldn't find something similar, where do you think it would be nice to have? In util/misc.rs?

There is a Signature::serialize_der() so I don't think if it's that much useful after thinking more, only if you're finalizing a big multisig ? Don't bother for now.

ariard · 2020-09-08T02:05:31Z

dlc/src/util.rs

+}
+
+/// Create a signature for a transaction input using the provided private key
+/// and place the signature and associated key on the witness stack.


nit: associated pubkey
nit: private key -> seckey/secrret key, more secp256k1 parlance

k-matsuzawa · 2020-09-08T05:18:17Z

dlc/src/lib.rs

+    fn get_size(&self) -> usize {
+        match self {
+            InputType::P2WPKH => P2WPKH_SPK_SIZE * 4 + P2WPKH_WITNESS_SIZE,
+        }
+    }


I have a question about the weight information I'm getting here.

Why is it only returning the size of TxOut::ScriptPubKey and WitnessStack?

Why is returning the size of TxOut::ScriptPubKey?

It is used as PartyParams::inputs in PartyParams::get_change_output_and_fees. However, this function does not return the size of all Inputs, so it seems that only the number of TxInputs when FUND_TX_BASE_WEIGHT is calculated is assumed.

In order to make the number of Inputs variable, it seems necessary to calculate the weight of TxInput as follows. (It is necessary to subtract the weight of TxInput from FUND_TX_BASE_WEIGHT.)

/// tx input base weight : (outpoint(36) + sequence(4) + scriptPubKeySize(1)) * 4 const TX_INPUT_BASE_WEIGHT: usize = 164; fn get_weight(&self) -> usize { match self { InputType::P2WPKH => TX_INPUT_BASE_WEIGHT + P2WPKH_WITNESS_SIZE, } }

Thanks you are right the computation was incorrect. Updated as you suggested.

Tibo-lg · 2020-09-15T03:04:17Z

Switched to using SecretKey/PublicKey of rust-secp256k1 instead of PrivateKey/PublicKey of rust-bitcoin as it seems that this is what rust-lightning is doing and otherwise is preventing me from reusing things from there for later PR.

nkohen

Generally looks good to go, mostly nits, main two structural things that might be nice to think about are having CET-related interfaces to directly require adaptor points with auxiliary functions which compute those adaptor points, and the other thing is that I find it a little strange that Payout is decoupled from Message; not really sure what your plans are on that front but at some point some data structure somewhere is going to have to correlate these I think.

nkohen · 2021-01-21T22:47:59Z

dlc/src/lib.rs

+#![deny(dead_code)]
+#![deny(unused_imports)]
+#![deny(missing_docs)]
+#![forbid(unsafe_code)]


nit: have this one twice

thanks fixed

nkohen · 2021-01-21T22:49:11Z

dlc/src/lib.rs

+
+/// The witness size of a P2WPKH input
+/// See: https://github.com/discreetlogcontracts/dlcspecs/blob/master/Transactions.md#fees
+pub const P2WPKH_WITNESS_SIZE: usize = 108;


I think this might be 107 if you are using low R signing

Good point fixed

nkohen · 2021-01-21T22:50:33Z

dlc/src/lib.rs

+
+/// Represents the payouts for a unique contract outcome. Local party represents
+/// the initiator of the contract while remote party represents the party
+/// accepting the contract.


I personally ended up using offer and accept in most places and only used local and remote when it came to the places where that made sense (only in signing pretty much)

nkohen · 2021-01-21T22:51:42Z

dlc/src/lib.rs

+
+/// Represents the payouts for a unique contract outcome. Local party represents
+/// the initiator of the contract while remote party represents the party
+/// accepting the contract.


So for example Payout would have offerPayout: u64, acceptPayout: u64 or something like that

nkohen · 2021-01-21T22:53:10Z

dlc/src/lib.rs

+}
+
+/// Contains the necessary transactions for establishing a DLC
+pub struct DlcTransactions {


might be nice to mention whether they are signed or unsigned in this struct? Or if it is both/either then mention that instead and maybe when one should expect sigs and when not

as discussed might not make sense

nkohen · 2021-01-22T06:43:31Z

dlc/src/lib.rs

+            local_change_output,
+            remote_change_output,
+        ];
+        outs.into_iter().filter(|o| o.value >= DUST_LIMIT).collect()


I believe it is still the case that to be compatible with the current spec (subject to change) all parties must have change outputs

nkohen · 2021-01-22T06:48:38Z

dlc/src/lib.rs

+/// Create an adaptor signature for the given cet
+pub fn create_cet_adaptor_sig<C: secp256k1::Signing>(
+    secp: &secp256k1::Secp256k1<C>,
+    cet: &Transaction,
+    oracle_pubkey: &SchnorrPublicKey,
+    oracle_r_value: &SchnorrPublicKey,
+    funding_sk: &SecretKey,
+    funding_script_pubkey: &Script,
+    fund_output_value: u64,
+    msg: &Message,
+) -> Result<(AdaptorSignature, AdaptorProof), Error> {
+    let adaptor_point = secp.schnorrsig_compute_sig_point(msg, oracle_r_value, &oracle_pubkey)?;
+    let sig_hash = util::get_sig_hash_msg(cet, 0, funding_script_pubkey, fund_output_value);
+
+    Ok(secp.adaptor_sign(&sig_hash, &funding_sk, &adaptor_point))
+}


I would highly recommend refactoring this to be a function which takes in an adaptor_point instead of oracle and message information, and then on top of that, if you want, a second function which computes the adaptor_point and delegates. The current way will not be so easy to transition to multi-oracle.

(Same idea goes for below in places where oracle info is used)

Good idea, refactored let me know if that fits what you had in mind.

nkohen · 2021-01-22T06:49:01Z

dlc/src/lib.rs

+    Ok(secp.adaptor_sign(&sig_hash, &funding_sk.key, &adaptor_point))
+}
+
+/// Sign the given ce using own private key, adapt the counter party signature


weirdly enough, it still shows up as ce for me?

nkohen · 2021-01-22T06:50:16Z

dlc/src/lib.rs

+    adaptor_signature: &AdaptorSignature,
+    oracle_signature: &SchnorrSignature,
+    funding_sk: &SecretKey,
+    other_pk: &PublicKey,


btw this is the context where I think local/remote terminology makes sense

kept other, feels clearer to me

nkohen · 2021-01-22T06:54:24Z

dlc/src/lib.rs

+    oracle_pubkey: &SchnorrPublicKey,
+    oracle_r_value: &SchnorrPublicKey,


ditto earlier comment here

created two functions

nkohen · 2021-01-25T19:48:17Z

dlc/src/lib.rs

+    accept_params: &PartyParams,
+    payouts: &[Payout],
+    refund_lock_time: u32,
+    fee_rate_per_kw: u64,


I think this also has to be fee_rate_per_vb

Thanks indeed!

nkohen · 2021-01-25T19:51:44Z

dlc/src/lib.rs

+    let sig_hash = util::get_sig_hash_msg(cet, 0, funding_script_pubkey, fund_output_value);
+
+    Ok(secp.adaptor_sign(&sig_hash, &funding_sk, &adaptor_point))


nit: might be nicer to have this function call the other one after computing the adaptor point, this way there is less code duplication making changes in the future easier

Yeah definitely that was a mistake tbh thanks!

nkohen

ACK

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch 8 times, most recently from 2b0dcc7 to 8fda81b Compare August 28, 2020 01:44

nkohen reviewed Aug 31, 2020

View reviewed changes

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch 4 times, most recently from 4a7ba22 to bd6b6c1 Compare September 1, 2020 07:36

ariard reviewed Sep 8, 2020

View reviewed changes

k-matsuzawa reviewed Sep 8, 2020

View reviewed changes

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch 4 times, most recently from 6e42fe5 to 9e28dd1 Compare September 15, 2020 03:02

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch from 9e28dd1 to e364797 Compare September 15, 2020 06:50

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch from e364797 to aeba084 Compare September 25, 2020 04:01

Tibo-lg mentioned this pull request Oct 1, 2020

Tuesday October 6 2020 Meeting discreetlogcontracts/dlcspecs#101

Merged

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch from aeba084 to ac6e99c Compare December 10, 2020 05:49

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch 6 times, most recently from 8ba94d8 to 93778a8 Compare January 12, 2021 01:11

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch 7 times, most recently from 3e5374a to d78e4b3 Compare January 13, 2021 07:00

nkohen reviewed Jan 22, 2021

View reviewed changes

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch 3 times, most recently from 642203d to 7daa0c6 Compare January 25, 2021 01:26

nkohen reviewed Jan 25, 2021

View reviewed changes

Tibo-lg added 2 commits January 26, 2021 16:03

Implement ecdsa adaptor dlc functions

f479736

Add integration tests

44dc955

Tibo-lg force-pushed the ecdsa-adaptor-dlc branch from 7daa0c6 to 44dc955 Compare January 26, 2021 07:04

nkohen approved these changes Jan 27, 2021

View reviewed changes

Tibo-lg merged commit 78aecae into master Jan 27, 2021

Tibo-lg deleted the ecdsa-adaptor-dlc branch October 5, 2021 12:37

		bitcoin = {version="0.23.0-adaptor.0", git = "https://github.com/tibo-lg/rust-bitcoin", branch = "ecdsa-adaptor", package="bitcoin"}
		secp256k1 = {version="0.17.3-adaptor.0", git = "https://github.com/tibo-lg/rust-secp256k1", branch = "ecdsa-adaptor", package="secp256k1"}

		const P2WPKH_INPUT_SIZE: usize = 31;
		const P2WSH_INPUT_SIZE: usize = 43;


		generate_blocks(1);

		assert!(local_rpc.send_raw_transaction(&dlc_txs.cets[0]).is_ok());

		oracle_pubkey: &SchnorrPublicKey,
		oracle_r_value: &SchnorrPublicKey,

		let sig_hash = util::get_sig_hash_msg(cet, 0, funding_script_pubkey, fund_output_value);

		Ok(secp.adaptor_sign(&sig_hash, &funding_sk, &adaptor_point))

Ecdsa adaptor dlc #2

Ecdsa adaptor dlc #2

Conversation

Tibo-lg commented Aug 27, 2020

nkohen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibo-lg Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibo-lg commented Sep 15, 2020

nkohen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibo-lg Sep 8, 2020 •

edited

Loading