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

Migrate transaction handling to libwally #2500

Merged
merged 30 commits into from Apr 8, 2019

Conversation

Projects
None yet
3 participants
@cdecker
Copy link
Member

commented Mar 25, 2019

This is my attempt to dogfood the libwally transaction handling in c-lightning. Ultimately it'll remove quite a bit of code that we need to maintain and allow us to get upstream support from the wally team. This is definitely not the easiest (or shortest) PR ever, but I did my best to keep it reviewable (see below).

I tried to structure the commits in such a way that they are easy to review
and could be tested at every small change in order to guarantee
correctness. This means that this PR takes the scenic route to its goal:

  • The first few commits deal with constructing both the old style transaction
    and the wally transaction in parallel. A number of shims for adding outputs
    and inputs were added.
  • Followed by a few commits adding assertions that everything works as
    expected, i.e., the two transactions were being generated identical to each
    other (cfr. bitcoint_tx_check and hash equality assertions when signing).
  • Then the binaries are moved off of using tx->input and tx->output since
    these are now managed by libwally internally.
  • Finally we rip out the tx->input and tx->output handling and everything
    should be back to normal.

Open questions

I'm not sure whether leaving the shims in is a good idea yet, but I like the
idea of having bitcoin/tx.c be the boundary between wally and the rest of
the code, so maybe this verbose code is good to stay?

Reviewing / Merging

Abandon all hope, ye who enter 馃毆

I kept most fixups separate, but in squash order, so reviewing becomes less
painful. Many fixups only concern a single file and could potentially be done
in a separate commit, but I felt that'd be over the top, so I separated most
things by binary they produce.

Once we agree that the PR is good, I'll squash the commits, to keep a minimal
set that has logical boundaries.

@cdecker cdecker added this to the 0.7.1 milestone Mar 25, 2019

@cdecker cdecker requested review from rustyrussell and niftynei Mar 25, 2019

@cdecker cdecker force-pushed the cdecker:wally branch from 74cb07c to f136604 Mar 25, 2019

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

ackbot?

@niftynei
Copy link
Collaborator

left a comment

nice, but kind of a pain to review. some of the commits could definitely use squashing.

@@ -7,21 +7,19 @@ struct htlc;

/**
* permute_inputs: permute the transaction inputs into BIP69 order.
* @inputs: usually bitcoin_tx->inputs, must be tal_arr.
* @tx: the transation whose inputs are to be sorted (inputs must be tal_arr).

This comment has been minimized.

Copy link
@niftynei

niftynei Mar 30, 2019

Collaborator

transation -> transaction


/**
* permute_outputs: permute the transaction outputs into BIP69 + cltv order.
* @outputs: usually bitcoin_tx->outputs, must be tal_arr.
* @tx: the transation whose outputs are to be sorted (outputs must be tal_arr).

This comment has been minimized.

Copy link
@niftynei

niftynei Mar 30, 2019

Collaborator

transation -> transaction

@@ -6,7 +6,13 @@
#include <ccan/str/hex/hex.h>
#include <common/utils.h>

const char extended_tx[] = "02000000000101b5bef485c41d0d1f58d1e8a561924ece5c476d86cff063ea10c8df06136eb31d00000000171600144aa38e396e1394fb45cbf83f48d1464fbc9f498fffffffff0140330f000000000017a9140580ba016669d3efaf09a0b2ec3954469ea2bf038702483045022100f2abf9e9cf238c66533af93f23937eae8ac01fb6f105a00ab71dbefb9637dc9502205c1ac745829b3f6889607961f5d817dfa0c8f52bdda12e837c4f7b162f6db8a701210204096eb817f7efb414ef4d3d8be39dd04374256d3b054a322d4a6ee22736d03b00000000";
const char extended_tx[] =

This comment has been minimized.

Copy link
@niftynei

niftynei Mar 30, 2019

Collaborator

extra space?

This comment has been minimized.

Copy link
@cdecker

cdecker Apr 3, 2019

Author Member

I don't quite follow, I just broke the line into 78 character lines, so it'd be nicer to read.

hsmd/hsmd.c Outdated
@@ -773,7 +773,8 @@ static struct io_plan *handle_sign_commitment_tx(struct io_conn *conn,
* pointer, as we don't always know it (and zero is a valid amount, so
* NULL is better to mean 'unknown' and has the nice property that
* you'll crash if you assume it's there and you're wrong. */
tx->input[0].amount = tal_dup(tx->input, struct amount_sat, &funding);
tx->input_amounts[0] = tal_dup(tx->input, struct amount_sat, &funding);
tx->input_amounts[0] = tx->input_amounts[0];

This comment has been minimized.

Copy link
@niftynei

niftynei Mar 30, 2019

Collaborator

???

This comment has been minimized.

Copy link
@cdecker

cdecker Mar 31, 2019

Author Member

Squashing will resolve this :-)

push_varint(0, push, pushp);
continue;
}
elements = tal_count(tx->input[i].witness);

elements = witness->num_items;

This comment has been minimized.

Copy link
@niftynei

niftynei Mar 30, 2019

Collaborator

not really a big deal but you could get rid of elements and just use the witness->num_items in line below

@@ -121,9 +113,6 @@ void bitcoin_tx_input_set_witness(struct bitcoin_tx *tx, int innum,
size_t stack_size = tal_count(witness);

/* Free any lingering witness */

This comment has been minimized.

Copy link
@niftynei

niftynei Mar 30, 2019

Collaborator

this comment seems out of place now

@niftynei

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2019

i know you're planning to squash/redo how these commits appear but

ACK f136604

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

i know you're planning to squash/redo how these commits appear but

Yeah sorry about the unsquashed commits, I was hoping it'd make things easier to review, but it meant leaving some intermediate fixes dangling. All commits with fixup! as a prefix of the commit message will be squashed into the corresponding parent commit 馃槈

@rustyrussell
Copy link
Contributor

left a comment

Great work! Some minor fixups as suggested by @niftynei and of course much squashing and rebasing...

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Awesome, I'll fix and squash ASAP :-)

Thanks for the review ^^

@cdecker cdecker force-pushed the cdecker:wally branch from f136604 to f72173d Apr 3, 2019

cdecker added some commits Mar 1, 2019

wally: Add wally_tx in bitcoin_tx
Allows us to slowly migrate individual parts.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate version and locktime to libwally tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Add shims to generate both transaction versions in parallel
We are slowly migrating towards a wally-transactions only world, but to make
this reviewable we start building both old and new style transactions in
parallel. In a second pass we'll then start removing the old ones and use
libwally only.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate the funding transaction to use the shims
Signed-off-by: Christian Decker <decker.christian@gmail.com>
tx: Change permute_{inputs,outputs} to sort both old and new txs
This is required in order for both old and new transactions to be identical.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Add a consistency check for old and new style txs
During the migration to `libwally` we want to make absolutely sure that both
transactions are generated identical, and can eventually be switched over.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate the funding transaction
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate the withdraw transaction
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate initial_commit_tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate commit_tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Build wally transactions in parallel with the old ones
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate close_tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate htlc_tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Add setters for output amounts, input witnesses and scripts
These are used when grinding the feerate and signing. These are just simple
facades that keep both wally and old style transactions in sync.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Use output and input setters when signing / grinding feerate
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Use input and output setters in onchaind and htlc_tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate run-tx-encode to directly access the wally_tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Switch signatures over to using the wally_tx hash
First step towards decomissioning the handrolled bitcoin_tx operations.

Signed-off-by: Christian Decker <decker.christian@gmail.com>

cdecker added some commits Mar 22, 2019

wally: Move input amounts into a separate array
The `wally_tx_input`s do not keep track of their input value, which means we
need to track them ourselves if we try to sign these transactions at a later
point in time.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Use libwally only to serialize transactions
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Remove unused sha256_tx_for_sig function
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate permute_tx to be wally_tx first
This is the second to last time I'm touching this file, just need to remove
the `tx->input` and `tx->output` swapping once they are removed.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Add accessor methods for script and amount
These are handled internally in the `wally_tx` and do not conform to our usual
tallocated strings that can by inspected using `tal_bytelen`, and we don't
really want to litter our code with whitelisting comments for the
`amount_sat.satoshis` access, so these just do read-only on the fly conversions.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate onchaind to use the wally tx and the helpers
Weaning `onchaind` off its use of the internal bitcoin_tx input and output
fields, since we're going to remove them soon, I promise.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate hsmd to use the wally tx
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate main daemon to use wally transactions
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Migrate channeld over to use libwally
Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Remove tx->input and tx->output, wally all the way!
This is what all of this has been working towards: ripping out the handwoven
transaction handling. By removing the custom parsing we can finally switch
over to using `wally_tx` as sole representation of transactions in
memory. The commit is a bit larger but it's mostly removing setters and old
references to the input and output fields.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wally: Post-migration cleanups
Signed-off-by: Christian Decker <decker.christian@gmail.com>

@cdecker cdecker force-pushed the cdecker:wally branch from f72173d to 74562ca Apr 3, 2019

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Rebased and squashed. Ready for the next review barrage @rustyrussell and @niftynei :-)

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Ack 74562ca

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Let's not spend too much time on (re-re-re-)review. Applied.

@rustyrussell rustyrussell merged commit 21a0dad into ElementsProject:master Apr 8, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.