PSBT, continued - #3740
Conversation
d10286d to
dc1a942
Compare
|
Valgrind errors are caused by a bug I found in libwally-core earlier this week; there's a patch to fix libwally here ElementsProject/libwally-core#200 |
| u32 outnum, u32 sequence, const u8 *scriptSig, | ||
| struct amount_sat amount, const u8 *scriptPubkey, | ||
| const u8 *input_wscript); |
There was a problem hiding this comment.
I'm sure I will get those 3 u8[] params wrong at some point xD
There was a problem hiding this comment.
Hm yeah a type system might be nice here ;)
There was a problem hiding this comment.
I think that would be a great cleanup, but since there is likely a lot of wiring involved we should move that to a cleanup PR I guess.
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
Looking very good, just some minor clarifications. I really hope the checks when loading from the DB never fail, since that'd be an indication of DB corruption anyway and we couldn't continue/close channels with those issues anyway. Can we turn them into assert()s to have a stronger indication of something being very wrong?
| if (!db_column_signature(stmt, 5, &last_sig.s)) | ||
| abort(); |
There was a problem hiding this comment.
Could this column be null? If not why would this ever fail? Being unable to load a raw signature from the DB sounds like a DB corruption.
There was a problem hiding this comment.
I'm not sure what you're asking here. It should definitely not ever be NULL -- we don't open a channel until we've saved the last_sig for the original commit tx.
There was a problem hiding this comment.
Excellent, that answers the question. I think the abort() call is warranted here, since if we hit it something deeper is broken. The reason I was asking is because of the continue in my previous comment, where I think we'd also need to abort().
|
Seems your patch made it into libwally, should we pull in the new version now or do you prefer bundling the changes and pull them in all at once? |
was running into buffer overrun errors? something about the iteration method was broken
We need this so we can access it when populating bitcoin inputs in the next commit
Instead we will stash them into the PSBT as a utxo/witness record (which includes the amount)
|
I've got a patch to update libwally that'll add to this PR. |
Update the `bitcoin_tx_add_input` interface to accept a witness script and or scriptPubkey. We save the amount + witness script + witness program (if known) to the PSBT object for a transaction when creating an input.
Make comment a bit more descriptive / usefu
when re-populating a channel's data from the database, since we don't store the psbt data (with input scripts + amounts), we need to re-populate it. the right solution is to patch the psbt into the database; for now we 'monkey-patch' it in.
We'll need these for the database methods we're going to add shortly
We need to update the psbt's global transaction simultaneously, so we wrap access to the locktime in a method which will handle both
When libwally exposes it, we can use theirs. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We'll need this for settng the tx correctly, for reasons
we'll use these for the commitment txs!
calling `wally_psbt_finalize` doesn't return a status indicator; instead you must call `psbt_is_finalized` to check that it's eligible for 'extraction' -- extraction will fail if the psbt is not in a finalized state.
note: missing migration at the moment lol
We're going to use the hsm for a migration, so we need to set up the HSM before we get to the wallet migration code. All that this requires is removing the places in HSM init that we touch the database struct -- easy enough to accomplish by passing the required field back out from init, and then associating it onto the wallet after it's been initialized.
We update the `last_tx` in `channels` to be psbt format, instead of a linearized transaction. We need the amount of the input populated, which we have since this is the 'funding' amount. Ideally we'd also populate the funding scriptPubkey, but to do that we'd need to access the HSM module to fetch our local funding pubkey, which isn't initialized at the time that the database migrations are run. Since the only field the HSM uses currently when signing these is the amount field, it's ok to just leave it out. needs a test!
We use a database snapshot with 3 channels -- two of which have HTLCs dangling and one is an initial open channel tx in the 'old' tx hex format in last_tx and confirm that they are successfully updated to PSBT format on start.
needs to be update elsewhere too!
For any transaction that's got 'finalized' signature data for an input, we should add this information to the psbt also
Prior to this commit, passing a NULL stack to `bitcoin_tx_input_set_witness` unsets the witness stack on the bitcoin_tx's wally_tx but leaves the final witness on the PSBT unchanged. at the moment, libwally's `wally_psbt_input_set_final_witness` will blow up if you attempt to set a NULL witness -- instead we manually remove it if the passed in stack is NULL. previously we would leave the PSBT's witness unchanged.
|
ACK 2f22bb7 |
3 participants
More internal updates. Here we pull out the input_amount structure and instead always populate the utxo 'parent' of an input with the amount and relevant script pubkey.
This means that we need to start serializing the
last_txas a psbt so that we can preserve input amount data for onchaind's signatures, which we do here.Changelog-None