[WIP] HTLC implementation in the wallet #7601

ebfull commented at 8:54 PM on February 25, 2016: none

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 commented at 9:11 PM on February 25, 2016: contributor

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

jonasschnelli added the label Wallet on Feb 25, 2016

sipa commented at 9:16 PM on February 25, 2016: member

@jonasschnelli All P2SH is already standard.

jonasschnelli commented at 9:19 PM on February 25, 2016: contributor

@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 commented at 9:24 PM on February 25, 2016: member

@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 commented at 10:37 PM on February 25, 2016: contributor

Concept ACK.

dcousens commented at 2:23 AM on February 26, 2016: contributor

concept ACK

petertodd commented at 3:45 AM on February 26, 2016: contributor

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

Concept ACK

petertodd commented at 3:50 AM on February 26, 2016: contributor

@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 commented at 6:10 AM on February 26, 2016: contributor

Concept ACK

paveljanik commented at 7:19 AM on February 26, 2016: contributor

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 commented at 7:21 AM on February 26, 2016: contributor

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

jonasschnelli commented at 7:30 AM on February 26, 2016: contributor

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 commented at 1:20 PM on February 26, 2016: member

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 commented at 1:53 PM on February 26, 2016: member

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 commented at 2:03 PM on February 26, 2016: none

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 commented at 9:54 PM on February 26, 2016: contributor

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

jtimon commented at 11:18 PM on February 26, 2016: contributor

Wow, concept ACK.

sipa commented at 5:47 AM on March 5, 2016: member

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

in src/rpc/rawtransaction.cpp:None in 9136c81d60 outdated

 838 | @@ -839,3 +839,4 @@ UniValue sendrawtransaction(const UniValue& params, bool fHelp)
 839 |  
 840 |      return hashTx.GetHex();
 841 |  }
 842 | +

jtimon commented at 2:47 PM on March 6, 2016:

This new line?

gmaxwell commented at 4:24 PM on March 30, 2016: contributor

@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 commented at 6:05 PM on March 30, 2016: none

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

ebfull commented at 5:35 AM on June 7, 2016: none

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

btcdrak commented at 12:01 PM on June 26, 2016: contributor

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

gmaxwell commented at 4:36 PM on July 5, 2016: contributor

CSV is active on mainnet!

ebfull force-pushed on Jul 18, 2016

ebfull commented at 1:09 AM on July 18, 2016: none

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.

in src/script/standard.cpp:None in 94b6591673 outdated

  62 | +            const opcodetype accepted_timeout_ops[] = {OP_CHECKLOCKTIMEVERIFY, OP_CHECKSEQUENCEVERIFY};
  63 | +
  64 | +            BOOST_FOREACH(opcodetype hasher, accepted_hashers) {
  65 | +                BOOST_FOREACH(opcodetype timeout_op, accepted_timeout_ops) {
  66 | +                    mTemplates.insert(make_pair(TX_HTLC, CScript()
  67 | +                        << hasher << OP_ANYDATA << OP_EQUAL

jl2012 commented at 11:56 AM on August 23, 2016:

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 commented at 3:08 PM on November 23, 2016: contributor

Needs rebase

jtimon commented at 3:41 PM on December 27, 2016: contributor

Still needs rebase.

ebfull commented at 5:23 AM on December 28, 2016: none

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 commented at 8:59 AM on December 29, 2016: contributor

@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 commented at 8:18 AM on February 20, 2017: none

@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 commented at 11:06 PM on February 21, 2017: none

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

`AddPreimage` and `GetPreimage` keystore methods. 7a36d0a932

importpreimage wallet RPC command. 979f17cdb8

Add wallet support for recognizing and signing HTLC transactions. 74a43f9dbd

GetScriptForHTLC helper function. 483c04c548

Refactor _createmultisig_redeemScript key recognition. 37722f0b63

Add `createhtlc` RPC command. d13d5da6b8

Add RPC test for `createhtlc`. 0ba1a5008b

ebfull force-pushed on Mar 6, 2017

in src/policy/policy.cpp:50 in 74a43f9dbd outdated

  45 | @@ -46,6 +46,8 @@ bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType, const bool w
  46 |              return false;
  47 |          if (m < 1 || m > n)
  48 |              return false;
  49 | +    } else if (whichType == TX_HTLC) {
  50 | +        return false;

arielgabizon commented at 1:53 PM on March 30, 2017:

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

ABISprotocol commented at 9:58 PM on June 2, 2017:

@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: https://github.com/lightningnetwork/lnd/pull/115#issuecomment-305739226

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

in src/script/sign.cpp:114 in 74a43f9dbd outdated

 107 | @@ -108,6 +108,30 @@ static bool SignStep(const BaseSignatureCreator& creator, const CScript& scriptP
 108 |          ret.push_back(valtype()); // workaround CHECKMULTISIG bug
 109 |          return (SignN(vSolutions, creator, scriptPubKey, ret, sigversion));
 110 |  
 111 | +    case TX_HTLC:
 112 | +        {
 113 | +            std::vector<unsigned char> image(vSolutions[0]);
 114 | +            std::vector<unsigned char> preimage;

JeremyRubin commented at 3:17 PM on March 31, 2017:

can reserve preimage.

in src/script/ismine.cpp:132 in 74a43f9dbd outdated

 124 | @@ -125,6 +125,19 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
 125 |          break;
 126 |      }
 127 |  
 128 | +    case TX_HTLC:
 129 | +    {
 130 | +        // Only consider HTLC's "mine" if we own ALL the keys
 131 | +        // involved.
 132 | +        vector<valtype> keys;

JeremyRubin commented at 3:24 PM on March 31, 2017:

Please reserve keys.

in src/script/standard.cpp:68 in 74a43f9dbd outdated

  63 | +                make_pair(OP_SHA256, OP_BLOB32),
  64 | +                make_pair(OP_RIPEMD160, OP_BLOB20)
  65 | +            };
  66 | +            const opcodetype accepted_timeout_ops[] = {OP_CHECKLOCKTIMEVERIFY, OP_CHECKSEQUENCEVERIFY};
  67 | +
  68 | +            BOOST_FOREACH(auto hasher, accepted_hashers) {

JeremyRubin commented at 3:25 PM on March 31, 2017:

Please don't introduce any new BOOST_FOREACH

in src/script/script.h:182 in 74a43f9dbd outdated

 178 | @@ -179,6 +179,10 @@ enum opcodetype
 179 |  
 180 |  
 181 |      // template matching params
 182 | +    OP_BLOB20 = 0xf7,

JeremyRubin commented at 3:31 PM on March 31, 2017:

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

in src/script/standard.cpp:385 in 483c04c548 outdated

 380 | +                         uint32_t timeout,
 381 | +                         opcodetype hasher_type,
 382 | +                         opcodetype timeout_type)
 383 | +{
 384 | +    CScript script;
 385 | +

JeremyRubin commented at 4:28 PM on March 31, 2017:

Check that hasher_type and image.size() match?

in src/rpc/misc.cpp:350 in d13d5da6b8 outdated

 345 | +            "It returns a json object with the address and redeemScript.\n"
 346 | +
 347 | +            "\nArguments:\n"
 348 | +            "1. seller_key    (string, required) The public key of the possessor of the preimage.\n"
 349 | +            "2. refund_key    (string, required) The public key of the recipient of the refund.\n"
 350 | +            "3. hash          (string, required) SHA256 or RIPEMD160 hash of the preimage.\n"

JeremyRubin commented at 4:35 PM on March 31, 2017:

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

in src/keystore.h:123 in 7a36d0a932 outdated

 114 | @@ -102,6 +115,16 @@ class CBasicKeyStore : public CKeyStore
 115 |      virtual bool HaveCScript(const CScriptID &hash) const;
 116 |      virtual bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const;
 117 |  
 118 | +    bool GetPreimage(
 119 | +        const std::vector<unsigned char>& image,
 120 | +        std::vector<unsigned char>& preimage
 121 | +    ) const;
 122 | +    
 123 | +    bool AddPreimage(

JeremyRubin commented at 4:38 PM on March 31, 2017:

I think that you should persist the PreimageMap to disk.

in src/keystore.h:127 in 7a36d0a932 outdated

 122 | +    
 123 | +    bool AddPreimage(
 124 | +        const std::vector<unsigned char>& image,
 125 | +        const std::vector<unsigned char>& preimage
 126 | +    );
 127 | +

JeremyRubin commented at 4:40 PM on March 31, 2017:

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.

in src/wallet/rpcdump.cpp:651 in 979f17cdb8 outdated

 646 | +        return NullUniValue;
 647 | +
 648 | +    if (request.fHelp || request.params.size() != 1)
 649 | +        throw runtime_error(
 650 | +            "importpreimage \"data\"\n"
 651 | +            "\nImports a preimage for use in HTLC transactions. Only remains in memory.\n"

JeremyRubin commented at 4:42 PM on March 31, 2017:

If you add persistence, be sure to update here.

in src/script/ismine.cpp:133 in 74a43f9dbd outdated

 124 | @@ -125,6 +125,19 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey, bool&
 125 |          break;
 126 |      }
 127 |  
 128 | +    case TX_HTLC:
 129 | +    {
 130 | +        // Only consider HTLC's "mine" if we own ALL the keys
 131 | +        // involved.
 132 | +        vector<valtype> keys;
 133 | +        keys.push_back(vSolutions[1]);

JeremyRubin commented at 4:45 PM on March 31, 2017:

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

JeremyRubin commented at 4:51 PM on March 31, 2017: contributor

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.

ebfull commented at 11:07 PM on March 31, 2017: none

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 commented at 9:34 AM on June 2, 2017: none

@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 commented at 5:32 PM on October 24, 2017: member

This still alive?

ebfull commented at 4:32 AM on October 26, 2017: none

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

fanquake commented at 6:37 AM on October 26, 2017: member

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

fanquake closed this on Oct 26, 2017

meshcollider added the label Up for grabs on May 7, 2018

in qa/rpc-tests/htlc.py:33 in 0ba1a5008b

  28 | +
  29 | +    def activateCSV(self):
  30 | +        # activation should happen at block height 432 (3 periods)
  31 | +        min_activation_height = 432
  32 | +        height = self.nodes[0].getblockcount()
  33 | +        assert(height < 432)

takahser commented at 3:15 PM on November 21, 2018:

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

winar-jin commented at 8:24 AM on December 17, 2019:

I think we can.

MarcoFalke locked this on Dec 16, 2021

fanquake removed the label Up for grabs on Aug 4, 2022