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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

elements: Add support of Liquid-BTC on elements #3078

Merged
merged 55 commits into from Oct 3, 2019

Conversation

@cdecker
Copy link
Member

commented Sep 20, 2019

This has been left to stew for quite some time, and I think I'm finally happy
with the way it turned out (that is to say "I'm getting tired of having to
rebase it since it touches a lot of code" 馃槈). I rebase and squashed it
from the state it was in for the announcement, and fixed a couple of more
fee-estimation bugs (we do estimate fees in a lot of different places it turns
out), and now we no longer get crashes running the test-suite (yay!).

We do however still get a couple of failures, most likely due to the test
being very bitcoin-specific (assuming bitcoin tx sizes for fees, and using
regtest addresses). I'm slowly making my way through them and parametrizing as
many as I can with sensible effort, but some of them I will just mark as
skippable since making them chainparam-dependent can be a lot of work.

The following tests still need triaging:

  • test_onchain_all_dust
  • test_opening_tiny_channel
  • test_no_fee_estimate
  • test_feerate_spam
  • test_block_backfill
  • test_invoice_routeboost

The tests were run with valgrind in the elements configuration, and no
issues were found. I won't add that configuration to Travis-CI though since we
are far better off with adding that configuration to our own CI system once we
have it running.

There is one really ugly part that I don't like about this PR and that's the
use of the is_elements global variable. Since we have started passing the
chainparams through to all things handling transactions, we might be able to
remove that in the future, but I'll do that in a separate (less
conflict-prone) PR, so please don't judge me too hard on that one, I was young
and inexperienced 馃槈

And yes, I know its a huge PR, sorry 馃檷鈥嶁檪

@cdecker cdecker added this to the 0.7.3 milestone Sep 20, 2019
@cdecker cdecker requested review from rustyrussell and niftynei Sep 20, 2019
@cdecker cdecker self-assigned this Sep 20, 2019
@cdecker cdecker force-pushed the cdecker:lightning-elements branch from 7cd9fa4 to e6a79a4 Sep 20, 2019
Copy link
Contributor

left a comment

Generally really good, obviously some fixups need squashing.

I think you should make chainparams a global in utils, as you did for plugins. This is better than is_elements as you'll get a nice crash if it isn't set. Probably do that and all the "wire into init msgs" in one commit. We can leave chainparams inside txs as a future aspiration if we ever want to dp multichain. That could easily go in as a separate PR, if we wanted.

@@ -6,6 +6,7 @@

secp256k1_context *secp256k1_ctx;
const tal_t *tmpctx;
bool is_elements = false;

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

Worse, this is not initialized in subdaemons; defaulting to false seem wrong. Classic trick here is to use a pointer, which nicely segfaults if not initialized/


/* For whatever reason the length computation gets upset if we tell it
* that we are using elements. It wants to discover it on its own, NO
* CLUES! (Ms. Doyle) */

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

Inline a reference to the Elements issue?

This comment has been minimized.

Copy link
@cdecker

cdecker Sep 25, 2019

Author Member

I filed ElementsProject/libwally-core#139 for this an fixed up the comment 馃憤

push_le32(b->hdr.target, push, &p);
push_le32(b->hdr.nonce, push, &p);
}
sha256_double(&out->shad, p, tal_bytelen(p));

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

Creating an array for this is nasty. You want:

struct sha256_ctx shactx;
sha256_init(&shactx);
sha256_update(...);
sha256_le32(...)...
sha256_double_done(&shactx, &out->shad);
@@ -97,6 +97,9 @@ const u8 *bitcoin_tx_output_get_script(const tal_t *ctx,
u8 *res;
assert(outnum < tx->wtx->num_outputs);
output = &tx->wtx->outputs[outnum];
if (output->features & WALLY_TX_IS_COINBASE)
return NULL;

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

This may be invalid for elements, but it's certainly possible for bitcoin?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

You also need to sweep all the other callers of bitcoin_tx_output_get_script: there are quite a few, and they don't expect NULL.

This comment has been minimized.

Copy link
@cdecker

cdecker Sep 25, 2019

Author Member

Made it a bit more generic: if libwally hands us back a NULL script, we return a NULL script. That way we defer the decision to libwally.

This comment has been minimized.

Copy link
@cdecker

cdecker Sep 25, 2019

Author Member

I also went through all callers and added NULL-handling where not already done so.

lightningd/chaintopology.c Show resolved Hide resolved
bitcoin/tx.c Outdated Show resolved Hide resolved


def test_minconf_withdraw(node_factory, bitcoind):
"""Issue 2518: ensure that ridiculous confirmation levels don't overflow

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

How did this new test get in here?

@@ -452,6 +453,187 @@ def is_p2wpkh(output):
assert only_one(fundingtx['vin'])['txid'] == res['wallettxid']


def test_withdraw(node_factory, bitcoind, chainparams):

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

How did this new test get in here?

@@ -420,6 +424,9 @@ static const struct utxo **wallet_select(const tal_t *ctx, struct wallet *w,
/* Account for witness (1 byte count + sig + key) */
input_weight += 1 + (1 + 73 + 1 + 33);

/* Elements inputs have 6 bytes of blank proofs attached. */
input_weight += 6;

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

But you didn't make this conditional on elements?

@@ -384,7 +384,7 @@ struct bitcoin_tx *bitcoin_tx(const tal_t *ctx,
/* If we are constructing an elements transaction we need to
* explicitly add the fee as an extra output. So allocate one more
* than the outputs we need internally. */
if (is_elements)
if (chainparams->is_elements)

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 23, 2019

Contributor

This commit message is not correct any more?

This comment has been minimized.

Copy link
@cdecker

cdecker Sep 26, 2019

Author Member

Fixed up into a later cleanup that does this kind of conversion.

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2019

Still working on the remaining review comments, will rebase and squash tomorrow.

@cdecker cdecker force-pushed the cdecker:lightning-elements branch from e6a79a4 to edd457a Sep 26, 2019
@cdecker

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2019

Ok, mostly fixed up the feedback. Still working on the non-fee-paying assets being set to 0. I'll add multi-asset support for tracking (but only consider the fee-paying asset in our UTXO selection).

cdecker added 21 commits Apr 10, 2019
Signed-off-by: Christian Decker <decker.christian@gmail.com>
There were a few places we were rebuilding the config path by appending
`bitcoin.conf` to the bitcoin directory. So now we just remember it and
reference it instead.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Since we will soon be writing the `liquid-regtest` section instead of the
`regtest` section we should make that configurable.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Using a global variable is a bit lazy, but weaving the network type through
the entire stack is a daunting task. Maybe we can make that happen at a later
stage.

Most of the changes in `chainparams.c` are just formatting the
`genesis_blockhash` a bit nicer (`clang-format` to the rescue).

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Since the difference between non-elements and elements block headers is just
the middle 2 fields, I split the old parsing code so I could add the middle
part.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This was a bit of trial and error due to libwally not liking hints when it
comes to length measurements, also the parsing bumps against a masking issue
in libwally that I'd following up on their issue tracker.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
The header is not a contiguous section of memory in elements, and it is of
variable length, so the simple trick of hashing in-memory data won't work
anymore. Some of the datafields would have been wrong on big-endian machines
anyway, so this is better anyway.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Skipping coinbase transactions and ensuring that the transaction is serialized
correctly when sending it onwards.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Without this we wouldn't see the elements specific fields and functions from
lightningd.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
If we are handling an elements transaction the value is not stored in the
satoshi field, rather it is stored in the `value` field which is prefixed with
a version (0x01) and is counted in `asset` units.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Depending on the network we end up with different signature hash algorithms,
so we just collect that decision in one place.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
We use to use the non-elements ones and then patch them manually. By using the
correct ones right from the start we have less work on our side.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
cdecker added 23 commits Jun 29, 2019
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Since elements addresses look quite different from the bitcoin mainnet
addresses I just added a sample to the chainparams fixture. In addition I
extracted some of the fixed strings to reference chainparams instead.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
In elements we add an explicit fee output, if we don't consider it when
selecting coins, we end up underpaying the fees.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is required since liquid-regtest and liquidv1 have different asset tags
for L-BTC which is the fee-paying asset.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
It'll be creating quite a few transactions and we will have to know which
params to use.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is the main reason we started weaving the chainparams everywhere: being
able to compare the asset type with the fee paying asset tag, thus determining
the value of the asset correctly (we still treat any non-fee-paying assets as
having value 0).

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Turns out that if we have the init message contain both the chainparams as
well as a transaction that needs to be parsed we need to set the parser to
elements mode before we reach the transaction...
We may be able to re-enable them later, but this is just painful right now.
The fundchannel plugin needs to know how to build a transaction, so we need to
tell it which chainparams to use. Also adds `chainparams` as a global, since
that seems to be the way to do things in plugins.
This was still using the non-elements estimation only and did not consider the
elements overhead.
Without this we'd be attaching valgrind to all elements-cli call, resulting in
a huge overhead and timeouts.
We used to match specifically on `is_elements && coinbase`, but we can just
hand off responsibility to libwally and then make sure we handle it correctly.
We now have a pointer to chainparams, that fails valgrind if we do anything
chain-specific before setting it.

Suggested-by: Rusty Russell <@rustyrussell>
Currently the only source for amount_asset is the value getter on a tx output,
and we don't hand it too far around (mainly ignoring it if it isn't the
chain's main currency). Eventually we could bubble them up to the wallet, use
them to select outputs or actually support assets in the channels.

Since we don't hand them around too widely I thought it was ok for them to be
pass-by-value rather than having to allocate them and pass them around by
reference. They're just 41 bytes currently so the overhead should be ok.

Signed-off-by: Christian Decker <@cdecker>
@cdecker cdecker force-pushed the cdecker:lightning-elements branch from edd457a to 1b60aae Sep 30, 2019
@cdecker cdecker marked this pull request as ready for review Sep 30, 2019
@cdecker cdecker requested a review from rustyrussell Sep 30, 2019
Copy link
Contributor

left a comment

Ack 1b60aae

@rustyrussell rustyrussell merged commit 7283efa into ElementsProject:master Oct 3, 2019
3 checks passed
3 checks passed
bitcoin-bot/acks Acks by rustyrussell
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kilrau kilrau referenced this pull request Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.