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

[WIP] HTLC implementation in the wallet #7601

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions qa/pull-tester/rpc-tests.py
Expand Up @@ -83,6 +83,7 @@
'rpcnamedargs.py',
'listsinceblock.py',
'p2p-leaktests.py',
'htlc.py'
]

ZMQ_SCRIPTS = [
Expand Down
152 changes: 152 additions & 0 deletions qa/rpc-tests/htlc.py
@@ -0,0 +1,152 @@
#!/usr/bin/env python3
# Copyright (c) 2014-2016 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.

#
# Test usage of HTLC transactions with RPC
#

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import *
from test_framework.mininode import sha256, ripemd160

class HTLCTest(BitcoinTestFramework):
BUYER = 0
SELLER = 1

def __init__(self):
super().__init__()
self.num_nodes = 2
self.setup_clean_chain = False

def setup_network(self):
self.nodes = start_nodes(self.num_nodes, self.options.tmpdir)
connect_nodes_bi(self.nodes, 0, 1)
self.is_network_split = False
self.sync_all()

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

@takahser takahser Nov 21, 2018

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

I think we can.

self.nodes[0].generate(432-height)
assert(get_bip9_status(self.nodes[0], 'csv')['status'] == 'active')
sync_blocks(self.nodes)

def run_test(self):
# Activate checksequenceverify
self.activateCSV()

# The buyer wishes to purchase the preimage of "254e38932fdb9fc27f82aac2a5cc6d789664832383e3cf3298f8c120812712db"
image = "254e38932fdb9fc27f82aac2a5cc6d789664832383e3cf3298f8c120812712db"
# The seller wishes to sell the preimage
preimage = "696c6c756d696e617469"

assert_equal(image, bytes_to_hex_str(sha256(hex_str_to_bytes(preimage))))

self.run_tests_with_preimage_and_image(preimage, image)

image = "5bb50b07a120dba7f1aae9623825071bc1fe4b40"
preimage = "ffffffffff"

assert_equal(image, bytes_to_hex_str(ripemd160(hex_str_to_bytes(preimage))))

self.run_tests_with_preimage_and_image(preimage, image)

def run_tests_with_preimage_and_image(self, preimage, image):
self.test_refund(image, 10)
self.test_refund(image, 100)

assert_equal(False, self.test_sell(image, 10))
self.nodes[self.SELLER].importpreimage(preimage)
assert(self.test_sell(image, 10))

def test_refund(self, image, num_blocks):
(seller_spending_tx, buyer_refund_tx) = self.fund_htlc(image, num_blocks)

# The buyer signs the refund transaction
buyer_sign = self.nodes[self.BUYER].signrawtransaction(buyer_refund_tx)
assert_equal(buyer_sign["complete"], True)

# The buyer should not be able to spend the funds yet
assert(self.expect_cannot_send(self.BUYER, buyer_sign["hex"]))

# After appearing in num_blocks number of blocks, the buyer can.
self.nodes[self.BUYER].generate(num_blocks-1)
sync_blocks(self.nodes)

self.nodes[self.BUYER].sendrawtransaction(buyer_sign["hex"])

self.nodes[self.BUYER].generate(1)
sync_blocks(self.nodes)

def test_sell(self, image, num_blocks):
(seller_spending_tx, buyer_refund_tx) = self.fund_htlc(image, num_blocks)

# The seller signs the spending transaction
seller_sign = self.nodes[self.SELLER].signrawtransaction(seller_spending_tx)

if seller_sign["complete"] == False:
return False

self.nodes[self.SELLER].sendrawtransaction(seller_sign["hex"])

return True

def fund_htlc(self, image, num_blocks):
buyer_addr = self.nodes[self.BUYER].getnewaddress("")
buyer_pubkey = self.nodes[self.BUYER].validateaddress(buyer_addr)["pubkey"]
seller_addr = self.nodes[self.SELLER].getnewaddress("")
seller_pubkey = self.nodes[self.SELLER].validateaddress(seller_addr)["pubkey"]

# Create the HTLC transaction
htlc = self.nodes[self.BUYER].createhtlc(seller_pubkey, buyer_pubkey, image, str(num_blocks))

# Import into wallets
self.nodes[self.BUYER].importaddress(htlc["redeemScript"], "", False, True)
self.nodes[self.SELLER].importaddress(htlc["redeemScript"], "", False, True)

# Buyer sends the funds
self.nodes[self.BUYER].sendtoaddress(htlc["address"], 10)
self.nodes[self.BUYER].generate(1)
sync_blocks(self.nodes)

funding_tx = False

for tx in self.nodes[self.SELLER].listtransactions("*", 500, 0, True):
if tx["address"] == htlc["address"]:
funding_tx = tx

assert(funding_tx != False)

seller_spending_tx = self.nodes[self.SELLER].createrawtransaction(
[{"txid": funding_tx["txid"], "vout": funding_tx["vout"]}],
{seller_addr: 9.99}
)
buyer_refund_tx = self.nodes[self.BUYER].createrawtransaction(
[{"txid": funding_tx["txid"], "vout": funding_tx["vout"], "sequence": num_blocks}],
{buyer_addr: 9.99}
)

# TODO: why isn't this already version 2? or shouldn't the signer figure
# out that it's necessary?
seller_spending_tx = "02" + seller_spending_tx[2:]
buyer_refund_tx = "02" + buyer_refund_tx[2:]

return (seller_spending_tx, buyer_refund_tx)

def expect_cannot_send(self, i, tx):
exception_triggered = False

try:
self.nodes[i].sendrawtransaction(tx)
except JSONRPCException:
exception_triggered = True

return exception_triggered

if __name__ == '__main__':
HTLCTest().main()

27 changes: 27 additions & 0 deletions src/keystore.cpp
Expand Up @@ -66,6 +66,33 @@ bool CBasicKeyStore::GetCScript(const CScriptID &hash, CScript& redeemScriptOut)
return false;
}

bool CBasicKeyStore::GetPreimage(
const std::vector<unsigned char>& image,
std::vector<unsigned char>& preimage
) const
{
LOCK(cs_KeyStore);

PreimageMap::const_iterator it = mapPreimages.find(image);
if (it != mapPreimages.end()) {
preimage = it->second;

return true;
}
return false;
}

bool CBasicKeyStore::AddPreimage(
const std::vector<unsigned char>& image,
const std::vector<unsigned char>& preimage
)
{
LOCK(cs_KeyStore);

mapPreimages[image] = preimage;
return true;
}

static bool ExtractPubKey(const CScript &dest, CPubKey& pubKeyOut)
{
//TODO: Use Solver to extract this?
Expand Down
23 changes: 23 additions & 0 deletions src/keystore.h
Expand Up @@ -39,6 +39,17 @@ class CKeyStore
virtual bool HaveCScript(const CScriptID &hash) const =0;
virtual bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const =0;

//! Support for HTLC preimages
virtual bool GetPreimage(
const std::vector<unsigned char>& image,
std::vector<unsigned char>& preimage
) const =0;

virtual bool AddPreimage(
const std::vector<unsigned char>& image,
const std::vector<unsigned char>& preimage
) =0;

//! Support for Watch-only addresses
virtual bool AddWatchOnly(const CScript &dest) =0;
virtual bool RemoveWatchOnly(const CScript &dest) =0;
Expand All @@ -50,6 +61,7 @@ typedef std::map<CKeyID, CKey> KeyMap;
typedef std::map<CKeyID, CPubKey> WatchKeyMap;
typedef std::map<CScriptID, CScript > ScriptMap;
typedef std::set<CScript> WatchOnlySet;
typedef std::map<std::vector<unsigned char>, std::vector<unsigned char>> PreimageMap;

/** Basic key store, that keeps keys in an address->secret map */
class CBasicKeyStore : public CKeyStore
Expand All @@ -59,6 +71,7 @@ class CBasicKeyStore : public CKeyStore
WatchKeyMap mapWatchKeys;
ScriptMap mapScripts;
WatchOnlySet setWatchOnly;
PreimageMap mapPreimages;

public:
bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey);
Expand Down Expand Up @@ -102,6 +115,16 @@ class CBasicKeyStore : public CKeyStore
virtual bool HaveCScript(const CScriptID &hash) const;
virtual bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const;

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

bool AddPreimage(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you should persist the PreimageMap to disk.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

virtual bool AddWatchOnly(const CScript &dest);
virtual bool RemoveWatchOnly(const CScript &dest);
virtual bool HaveWatchOnly(const CScript &dest) const;
Expand Down
2 changes: 2 additions & 0 deletions src/policy/policy.cpp
Expand Up @@ -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;

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@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

} else if (whichType == TX_NULL_DATA &&
(!fAcceptDatacarrier || scriptPubKey.size() > nMaxDatacarrierBytes))
return false;
Expand Down