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

[WIP] HTLC implementation in the wallet #7601

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
@ebfull

ebfull commented Feb 25, 2016

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

This comment has been minimized.

Member

jonasschnelli commented Feb 25, 2016

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

@sipa

This comment has been minimized.

Member

sipa commented Feb 25, 2016

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Feb 25, 2016

@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

This comment has been minimized.

Member

sipa commented Feb 25, 2016

@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

This comment has been minimized.

Member

gmaxwell commented Feb 25, 2016

Concept ACK.

@dcousens

This comment has been minimized.

Contributor

dcousens commented Feb 26, 2016

concept ACK

@petertodd

This comment has been minimized.

Contributor

petertodd commented Feb 26, 2016

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

Concept ACK

@petertodd

This comment has been minimized.

Contributor

petertodd commented Feb 26, 2016

@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

This comment has been minimized.

Member

btcdrak commented Feb 26, 2016

Concept ACK

@paveljanik

This comment has been minimized.

Contributor

paveljanik commented Feb 26, 2016

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

This comment has been minimized.

Contributor

paveljanik commented Feb 26, 2016

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

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Feb 26, 2016

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

This comment has been minimized.

Member

sipa commented Feb 26, 2016

@luke-jr

This comment has been minimized.

Member

luke-jr commented Feb 26, 2016

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

This comment has been minimized.

ebfull commented Feb 26, 2016

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

This comment has been minimized.

Member

gmaxwell commented Feb 26, 2016

@jtimon

This comment has been minimized.

Member

jtimon commented Feb 26, 2016

Wow, concept ACK.

@sipa

This comment has been minimized.

Member

sipa commented Mar 5, 2016

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

@jtimon

View changes

src/rpc/rawtransaction.cpp Outdated
@@ -839,3 +839,4 @@ UniValue sendrawtransaction(const UniValue& params, bool fHelp)
return hashTx.GetHex();
}

This comment has been minimized.

@jtimon

jtimon Mar 6, 2016

Member

This new line?

@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Mar 30, 2016

@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

This comment has been minimized.

ebfull commented Mar 30, 2016

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

@ebfull

This comment has been minimized.

ebfull commented Jun 7, 2016

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

@btcdrak

This comment has been minimized.

Member

btcdrak commented Jun 26, 2016

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

@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Jul 5, 2016

CSV is active on mainnet!

@ebfull

This comment has been minimized.

ebfull commented Jul 18, 2016

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.

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

This comment has been minimized.

@jl2012

jl2012 Aug 23, 2016

Member

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

This comment has been minimized.

Contributor

afk11 commented Nov 23, 2016

Needs rebase

@jtimon

This comment has been minimized.

Member

jtimon commented Dec 27, 2016

Still needs rebase.

@ebfull

This comment has been minimized.

ebfull commented Dec 28, 2016

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

This comment has been minimized.

Member

jl2012 commented Dec 29, 2016

@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

This comment has been minimized.

da2ce7 commented Feb 20, 2017

@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.

@zookozcash zookozcash referenced this pull request Feb 20, 2017

Open

HTLC for Bitcoin #2116

@ebfull

This comment has been minimized.

ebfull commented Feb 21, 2017

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

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

This comment has been minimized.

@arielgabizon

arielgabizon Mar 30, 2017

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

This comment has been minimized.

@ABISprotocol

ABISprotocol Jun 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: 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.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

can reserve preimage.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

Please reserve keys.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

Please don't introduce any new BOOST_FOREACH

@@ -179,6 +179,10 @@ enum opcodetype
// template matching params
OP_BLOB20 = 0xf7,

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

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

opcodetype timeout_type)
{
CScript script;

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

Check that hasher_type and image.size() match?

"\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"

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

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

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

I think that you should persist the PreimageMap to disk.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

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.

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"

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

If you add persistence, be sure to update here.

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

This comment has been minimized.

@JeremyRubin

JeremyRubin Mar 31, 2017

Contributor

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

@ebfull

This comment has been minimized.

ebfull commented Mar 31, 2017

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

This comment has been minimized.

ABISprotocol commented Jun 2, 2017

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

A few questions actually -

  1. Does BIP 199 need to be at final before this (HTLC) can be implemented in Core wallet? Probably dumb process question.
  2. 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?
  3. 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

This comment has been minimized.

Contributor

jb55 commented Oct 24, 2017

This still alive?

@ebfull

This comment has been minimized.

ebfull commented Oct 26, 2017

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

@fanquake

This comment has been minimized.

Member

fanquake commented Oct 26, 2017

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

@fanquake fanquake closed this Oct 26, 2017

@jb55

This comment has been minimized.

Contributor

jb55 commented Oct 26, 2017

fwiw I've rebased this at https://github.com/jb55/bitcoin/commits/htlc, I'll try to revive it. Should be a fun learning exercise... Will open a new PR if I succeed.

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

This comment has been minimized.

@takahser

takahser Nov 21, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment