[WIP] HTLC implementation in the wallet #7601

ebfull · 2016-02-25T20:54:26Z

This adds support for timelocked contracts which can be used to atomically
swap hash preimages over the blockchain. It is very similar to the script
used by the lightning network.

Written in collaboration with Pieter Wuille.

jonasschnelli · 2016-02-25T21:11:23Z

Interesting!
Haven't looked at it in detail, but would this affect "policy" by adding the TX_HTLC as to the standard templates?

sipa · 2016-02-25T21:16:48Z

@jonasschnelli All P2SH is already standard.

jonasschnelli · 2016-02-25T21:19:40Z

@sipa: Ah. Right. Then – I guess – the standard.cpp -> Solver() changes are only because we want to detect these types of P2SH scripts in the wallet.

sipa · 2016-02-25T21:24:13Z

@jonasschnelli To be clear: this indeed does make the HTLC type transaction standard in non-P2SH setting; it's however still smaller and cheaper than the largest raw multisig that is allowed. Perhaps that indeed shouldn't happen.

gmaxwell · 2016-02-25T22:37:57Z

Concept ACK.

dcousens · 2016-02-26T02:23:46Z

concept ACK

petertodd · 2016-02-26T03:45:39Z

Interestingly, this is useful for Paypub, as well as ZKCP, among other things.

Concept ACK

petertodd · 2016-02-26T03:50:41Z

@sipa For Paypub, having the payments be visible in the blockchain for the receiver is extremely useful; w/o the ability to do so in scriptPubKey you'd likely implement it with an extra OP_RETURN output (maybe until we have a better Bitmessage to use instead). Not necessarily an argument either for or against doing so, but worth noting.

btcdrak · 2016-02-26T06:10:36Z

Concept ACK

paveljanik · 2016-02-26T07:19:28Z

Some tests fail with:

Running testscript wallet.py ...
Initializing test directory /tmp/testr_T8PJ
start_node: bitcoind started, calling bitcoin-cli -rpcwait getblockcount
start_node: calling bitcoin-cli -rpcwait getblockcount returned
JSONRPC error: preimage must be hexadecimal string (not '')

I can't reproduce locally 8)

paveljanik · 2016-02-26T07:21:43Z

Concept ACK
Needs some tests. Needs BIP for new OP_ANYDATA.

jonasschnelli · 2016-02-26T07:30:58Z

Concept ACK.

1.) Not sure if this requires a BIP, I guess we don't need a BIP for every contract template and I can't find the other template matching params (like OP_SMALLINTEGER) in the BIPs.

I also concept ACK the IsStandard rule for HTLC to allow blockchain-visible HTLC, though, this might require a BIP.

sipa · 2016-02-26T13:20:55Z

OP_ANYDATA is just local wallet logic, it uses a range of script opcodes that are disabled in execution, and is only used inside the matching templates (I think it's a bad idea to mix them in the same CScript type...), so I disagree with needing a BIP for that. IsStandard changes so far have also been done without a BIP, but maybe we can postpone making raw HTLC standard for now. Certainly agree we need tests.

luke-jr · 2016-02-26T13:53:21Z

I agree IsStandard changes do not need a BIP, but it seems any transaction type supported by the reference implementation necessarily involves coordination with other software/people, and therefore should have a BIP written describing how it works.

Is there a benefit to bare HTLC over P2SH (or SegWit) HTLC? If not, it seems pointless to expand the policy to allow for it. Note that many users do not enable bare multisig (and it should probably be disabled by default).

ebfull · 2016-02-26T14:03:22Z

I don't think initially we should make this standard outside P2SH (yet), so that's definitely a change that should be made to this PR.

This work so far allows you to "import" a preimage into CWallet (so that you can redeem the atomic swap branch of the script) but it does not persist this to disk, and I'm curious if others think it should. (I'm skeptical of making bitcoind store secrets, especially if for some use cases they are ephemeral.)

gmaxwell · 2016-02-26T21:54:53Z

As an example of what this was written to enable: https://bitcoincore.org/en/2016/02/26/zero-knowledge-contingent-payments-announcement/

jtimon · 2016-02-26T23:18:32Z

Wow, concept ACK.

sipa · 2016-03-05T05:47:45Z

We should probably also wait until CSV is in, and add support for it in the constructed scripts here.

jtimon · 2016-03-06T14:47:42Z

src/rpc/rawtransaction.cpp

This new line?

gmaxwell · 2016-03-30T16:24:36Z

@sipa CSV is almost in, but I don't think we should be deploying enabled generation for them until the softfork is good and settled. Maybe behind a hidden option or testnet only?

ebfull · 2016-03-30T18:05:55Z

Additionally, supporting RIPEMD160 for ZKCPs is a must. I will try to find some time to write an arithmetic circuit for it.

ebfull · 2016-06-07T05:35:35Z

After segwit lands (?) I will try to rebase and clean this PR up.

btcdrak · 2016-06-26T12:01:50Z

@ebfull segwit has been merged to master and is also activated on testnet.

gmaxwell · 2016-07-05T16:36:18Z

CSV is active on mainnet!

ebfull · 2016-07-18T01:09:49Z

Rebased this on to master. Now supports RIPEMD160. Switched to CSV (block denominated mode). Added some RPC tests.

It can be trivially extended to use CLTV in the RPC, I just need to come up with a solid UX for it. The same goes for "relative" time strings like "10h" or "5d." Also, still need to bring over the "preimage" display that showed in listtransactions.

I'll try to work alongside #7534 for the remaining work, including submitting a BIP.

jl2012 · 2016-08-23T11:56:43Z

src/script/standard.cpp

+            BOOST_FOREACH(opcodetype hasher, accepted_hashers) {
+                BOOST_FOREACH(opcodetype timeout_op, accepted_timeout_ops) {
+                    mTemplates.insert(make_pair(TX_HTLC, CScript()
+                        << hasher << OP_ANYDATA << OP_EQUAL


In the "ELSE" case any relay node may replace the top stack with anything without invalidating the tx. See related discussion at https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-August/013036.html

afk11 · 2016-11-23T15:08:53Z

Needs rebase

jtimon · 2016-12-27T15:41:37Z

Still needs rebase.

ebfull · 2016-12-28T05:23:20Z

Before I rebase, I need to understand the consensus regarding preventing malleability of these scripts.

https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-August/013036.html

Multiple suggestions were made and I'm not personally qualified to pick from them.

jl2012 · 2016-12-29T08:59:44Z

@ebfull: I'd suggest this one. But you may want to discuss it on mailing list to make sure it is compatible with other implementations (if any)

OP_IF [HASHOP] <digest> OP_EQUALVERIFY <seller pubkey> OP_ELSE <num> [TIMEOUTOP] OP_DROP <buyer pubkey> OP_ENDIF OP_CHECKSIG

da2ce7 · 2017-02-20T08:18:14Z

@ebfull, this looks very interesting to me and many others. Would be wonderful for you can find the time to have a second look at this pull request since v0.14 has been forked off.

ebfull · 2017-02-21T23:06:02Z

I will be rebasing using @jl2012's script as the foundation, and update my BIP draft.

arielgabizon · 2017-03-30T13:53:22Z

src/policy/policy.cpp

        if (m < 1 || m > n)
            return false;
+    } else if (whichType == TX_HTLC) {
+        return false;


It looks like you're assuming all TX_HTLC transactions are DOS attacks?
Why reject them all?

@arielgabizon Would this actually reject all TX_HTLC items? I had the impression or understand that there was a design such that these would actually be completed in most instances. In lnd, a description of this was given as follows: "If payment/htlc amount is too small, than such output is called dust. If for some reason channel have been force closed during payment, and dust htlc/payment haven't been settled (both sides haven't agreed to remove the htlc and change the balances accordingly), th(e)n we (do) not include it in commitment transaction (thereby giv(ing it) as a fee to miners) in order to make it valid." For reference to this comment (from a contributor in lnd) see: lightningnetwork/lnd#115 (comment)

With that said, I'm not sure of how the differences will play out between how bitcoin core is planning on handling it vs. how it is handled presently in the lnd development process.

cc @ebfull

JeremyRubin

Concept Ack.

I did some code review, overall looks very clean and concise.

I was slightly concerned with the use of new template parameters, but I think that the proposed new ones are reasonable & general purpose.

I think that the preimage map should persist entries; but perhaps that can be done as a later PR. I would also suggest adding a Lock/Unlock feature in RPC, because I think that that would ensure safer use of entries stored in the preimage map for users.

JeremyRubin · 2017-03-31T15:17:47Z

src/script/sign.cpp

+    case TX_HTLC:
+        {
+            std::vector<unsigned char> image(vSolutions[0]);
+            std::vector<unsigned char> preimage;


can reserve preimage.

JeremyRubin · 2017-03-31T15:24:13Z

src/script/ismine.cpp

+    {
+        // Only consider HTLC's "mine" if we own ALL the keys
+        // involved.
+        vector<valtype> keys;


Please reserve keys.

JeremyRubin · 2017-03-31T15:25:17Z

src/script/standard.cpp

+            };
+            const opcodetype accepted_timeout_ops[] = {OP_CHECKLOCKTIMEVERIFY, OP_CHECKSEQUENCEVERIFY};
+
+            BOOST_FOREACH(auto hasher, accepted_hashers) {


Please don't introduce any new BOOST_FOREACH

JeremyRubin · 2017-03-31T15:31:28Z

src/script/script.h



    // template matching params
+    OP_BLOB20 = 0xf7,


Prefer to not make any new template matching parameters, but I see how it would be inconvenient to otherwise.

JeremyRubin · 2017-03-31T16:28:06Z

src/script/standard.cpp

+                         opcodetype timeout_type)
+{
+    CScript script;
+


Check that hasher_type and image.size() match?

JeremyRubin · 2017-03-31T16:35:10Z

src/rpc/misc.cpp

+            "\nArguments:\n"
+            "1. seller_key    (string, required) The public key of the possessor of the preimage.\n"
+            "2. refund_key    (string, required) The public key of the recipient of the refund.\n"
+            "3. hash          (string, required) SHA256 or RIPEMD160 hash of the preimage.\n"


Maybe hash should be optional, and if not supplied a GetRandHash preimage should be selected and returned to the caller.

JeremyRubin · 2017-03-31T16:38:24Z

src/keystore.h

+        std::vector<unsigned char>& preimage
+    ) const;
+
+    bool AddPreimage(


I think that you should persist the PreimageMap to disk.

JeremyRubin · 2017-03-31T16:40:56Z

src/keystore.h

+        const std::vector<unsigned char>& image,
+        const std::vector<unsigned char>& preimage
+    );
+


Maybe add a RemovePreimage, LockPreimage, and UnlockPreimage call, which could be used to ensure that a node no longer has the ability to solve a particular HTLC or does not until explicitly allowed. These should also go to disk.

JeremyRubin · 2017-03-31T16:42:00Z

src/wallet/rpcdump.cpp

+    if (request.fHelp || request.params.size() != 1)
+        throw runtime_error(
+            "importpreimage \"data\"\n"
+            "\nImports a preimage for use in HTLC transactions. Only remains in memory.\n"


If you add persistence, be sure to update here.

JeremyRubin · 2017-03-31T16:45:28Z

src/script/ismine.cpp

+        // Only consider HTLC's "mine" if we own ALL the keys
+        // involved.
+        vector<valtype> keys;
+        keys.push_back(vSolutions[1]);


Maybe just for safety, assert(vSolutions.size() > 3)

ebfull · 2017-03-31T23:07:37Z

Thanks for the reviews @arielg and @JeremyRubin. I intend to rebase this again and clean it up.

Also, BIP199 has been submitted to start standardizing what is in this PR.

ABISprotocol · 2017-06-02T09:34:40Z

@ebfull Thanks for mentioning... that was my question, regarding BIP 199, when reading this thread.

A few questions actually -

Does BIP 199 need to be at final before this (HTLC) can be implemented in Core wallet? Probably dumb process question.
Bitcoin wiki on Lightning network states in part, regarding "key features," that "payments can be routed across more than one blockchain (including altcoins and sidechains) as long as all the chains support the same hash function to use for the hash lock, as well as the ability the ability to create time locks." Although I see that this is not actually part of the proposed functionality here by way of the commits, might it ultimately allow the payment routing across more than one blockchain with further development?
TX_HTLC transactions - What is the way in which they are accepted and what would be a sample condition in which one would obviously be rejected?

jb55 · 2017-10-24T17:32:57Z

This still alive?

ebfull · 2017-10-26T04:32:26Z

Unfortunately I'm too busy to carry the torch on this PR. :(

fanquake · 2017-10-26T06:37:03Z

I'll close this then. If someone wants to restart/pickup this work they can cherry pick/reference and open a new PR.

takahser · 2018-11-21T15:15:11Z

qa/rpc-tests/htlc.py

+        # activation should happen at block height 432 (3 periods)
+        min_activation_height = 432
+        height = self.nodes[0].getblockcount()
+        assert(height < 432)


can we use assert(height < min_activation_height) here instead?

I think we can.

jonasschnelli added the Wallet label Feb 25, 2016

jtimon reviewed Mar 6, 2016
View reviewed changes

src/rpc/rawtransaction.cpp Outdated

Copy link

Contributor

jtimon Mar 6, 2016

Choose a reason for hiding this comment

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

This new line?

sipa mentioned this pull request May 17, 2016

Minimal RPC & wallet support for CLTV-enabled multisig addresses #7534

Closed

ebfull force-pushed the zkcp branch from 9136c81 to 94b6591 Compare July 18, 2016 00:17

jl2012 reviewed Aug 23, 2016
View reviewed changes

zookozcash mentioned this pull request Feb 20, 2017

HTLC for Bitcoin zcash/zcash#2116

Open

ebfull added 7 commits February 28, 2017 11:57

AddPreimage and GetPreimage keystore methods.

7a36d0a

importpreimage wallet RPC command.

979f17c

Add wallet support for recognizing and signing HTLC transactions.

74a43f9

GetScriptForHTLC helper function.

483c04c

Refactor _createmultisig_redeemScript key recognition.

37722f0

Add createhtlc RPC command.

d13d5da

Add RPC test for createhtlc.

0ba1a50

ebfull force-pushed the zkcp branch from 94b6591 to 0ba1a50 Compare March 6, 2017 15:00

arielgabizon reviewed Mar 30, 2017

View reviewed changes

JeremyRubin reviewed Mar 31, 2017

View reviewed changes

ABISprotocol mentioned this pull request Jun 2, 2017

Avoid dust outputs in commitment transactions lightningnetwork/lnd#115

Merged

fanquake closed this Oct 26, 2017

meshcollider added the Up for grabs label May 7, 2018

takahser reviewed Nov 21, 2018

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

fanquake removed the Up for grabs label Aug 4, 2022

[WIP] HTLC implementation in the wallet #7601

[WIP] HTLC implementation in the wallet #7601

Uh oh!

Conversation

ebfull commented Feb 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Feb 25, 2016

Uh oh!

sipa commented Feb 25, 2016 via email

Uh oh!

jonasschnelli commented Feb 25, 2016

Uh oh!

sipa commented Feb 25, 2016

Uh oh!

gmaxwell commented Feb 25, 2016

Uh oh!

dcousens commented Feb 26, 2016

Uh oh!

petertodd commented Feb 26, 2016

Uh oh!

petertodd commented Feb 26, 2016

Uh oh!

btcdrak commented Feb 26, 2016

Uh oh!

paveljanik commented Feb 26, 2016

Uh oh!

paveljanik commented Feb 26, 2016

Uh oh!

jonasschnelli commented Feb 26, 2016

Uh oh!

sipa commented Feb 26, 2016 via email

Uh oh!

luke-jr commented Feb 26, 2016

Uh oh!

ebfull commented Feb 26, 2016

Uh oh!

gmaxwell commented Feb 26, 2016

Uh oh!

jtimon commented Feb 26, 2016

Uh oh!

sipa commented Mar 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Mar 30, 2016

Uh oh!

ebfull commented Mar 30, 2016

Uh oh!

ebfull commented Jun 7, 2016

Uh oh!

btcdrak commented Jun 26, 2016

Uh oh!

gmaxwell commented Jul 5, 2016

Uh oh!

ebfull commented Jul 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afk11 commented Nov 23, 2016

Uh oh!

jtimon commented Dec 27, 2016

Uh oh!

ebfull commented Dec 28, 2016

Uh oh!

jl2012 commented Dec 29, 2016

Uh oh!

da2ce7 commented Feb 20, 2017

Uh oh!

ebfull commented Feb 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyRubin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebfull commented Feb 25, 2016 •

edited

Loading

takahser Nov 21, 2018 •

edited

Loading