--- Log opened Mon Mar 14 00:00:26 2022
02:55 < darosior> sanket1729: i don't have an opinion on the API, but if it can help here is how we use it: https://github.com/revault/revault_tx/blob/c7e3e29ed37c06196ea241015cfe92c28d1957bf/src/transactions/mod.rs#L332-L368.
04:29 < andytoshi> jeremyrubin: what Psbt are we returning here? are you proposing that finalize() clone the whole psbt before modifying it?
04:29 < andytoshi> that's very expensive, we should do it inline so the user has some choice
04:31 < sanket1729> Let's do both then. finalize_mut() and finalize()
04:32 < andytoshi> sgtm
05:02 -!- afilini [~afiliniaf@2001:bc8:1828:245::2] has joined ##miniscript
05:02 < afilini> Maybe he is suggesting to take ownership of the psbt and then return it?
05:36 -!- enick_544 [~afilini-m@2001:bc8:1828:245::2] has quit [Ping timeout: 240 seconds]
05:36 -!- afilini [~afiliniaf@2001:bc8:1828:245::2] has quit [Ping timeout: 252 seconds]
05:37 -!- enick_544 [~afilini-m@2001:bc8:1828:245::2] has joined ##miniscript
05:37 -!- afilini [~afiliniaf@2001:bc8:1828:245::2] has joined ##miniscript
05:46 < sanket1729> afilini: I think that would still require cloning the initial state of psbt to error the unmodified form in the error case. 
05:48 < afilini> Ah yes, if you wanna return it unchanged you need to clone
05:48 < sanket1729> I think we can leave the API iteration after some feedback in Rc or next major release. We have all the functionality that we need with `finalize` and `finalize_mut`. 
05:49 < afilini> I thought the idea was to make it more obvious that the psbt will be changed
05:49 < afilini> Sometimes completely, sometimes partially
05:49 < afilini> Yeah I'm personally ok with both options, I was just trying to interpret his idea
06:06 -!- shesek [~shesek@user/shesek] has joined ##miniscript
06:19 < jeremyrubin> The error would be partially modified 
06:19 < jeremyrubin> You'd take ownership of it and return it 
06:20 < jeremyrubin> If you want it unmodified after, it'd be up to caller to clone beforehand 
06:21 < jeremyrubin> I we just need to be sure that if a given input is going to fail to finalize that we've not yet modified it's data unfixable 
06:22 < jeremyrubin> afilini I think gets it 
06:25 < jeremyrubin> darosior's approach makes sense to, ensure you aren't going to corrupt an input before modifying it saving you the clone. 
06:28 < jeremyrubin> It's also possible to codify that you must have a check  done before you finalize by making is_finalizable grant an object capability that wraps the psbt. E.g. Result<Finalizable(PSBT),  (Error, Psbt)>
06:29 < jeremyrubin> And Finalizable is not constructable outside the library, and is the only way to call finalize. 
06:30 < jeremyrubin> What's cool is that using that sort of API we can actually construct the final witnesses in the Finalizable and not repeat thos allocs nor modify the psbt. But that might be a future work thing 
06:34 -!- shesek [~shesek@user/shesek] has quit [Remote host closed the connection]
06:35 -!- shesek [~shesek@user/shesek] has joined ##miniscript
06:41 -!- shesek [~shesek@user/shesek] has quit [Remote host closed the connection]
07:53 < sanket1729> andytoshi: can you also look at 306 and 308?
08:01 < andytoshi> sanket1729: sure
08:06 < sanket1729> I am also not sure about _mut naming in 305
08:07 < sanket1729> I think I understand jeremyrubin's suggestion. Probably better than current 305. I also found a bug in 305 that needs updating, and while I am at it will also update APIs to consume self and return Result(Psbt, (Psbt, Error)) 
08:20 < andytoshi> cool
08:20 < andytoshi> thank goodness for the integration ixes
08:20 < andytoshi> tests*
08:20 < andytoshi> no way i would've seen that bug
08:25 < sanket1729> I have created a draft PR for the tests that I will undraft after we merge 305 and 308
08:27 < andytoshi> dope
08:53 < sanket1729> andytoshi: Final three PRs: 308(multi_a interpreter)/310(bug fix) and then finally integration tests(309). 
09:01 < andytoshi> dope, will get through them today
10:17 < jeremyrubin> sanket1729: i think the implementation looks fine, docs a little off. 
10:17 < sanket1729> I changed it multiple times, forgot to update the docs.
10:19 < jeremyrubin> nw
10:20 < jeremyrubin> i think the API makes some sense, the only sort of issue that i see is that we clobber the input in finalizer::finalize, so when we return Err((Psbt, Error)) we no longer have the info to try e.g. adding data and re-finalizing (or even really seeing exactly what went wrong)
10:21 < jeremyrubin> i think it's OK because now for users it is more clear that they should keep a clone when using this API
10:22 < jeremyrubin> the only other option would be, before we clobber the input in finalize_input, if we first swap the input for a blank one, then clobber it.
10:23 < jeremyrubin> maybe we'd want     interpreter_inp_check(psbt, secp, index, utxos)?; to take in an input ref instead of / with index
10:24 < jeremyrubin> and then it'd be possible to just try a finalized input, if it fails, throw the finalized input into an error Error::InvalidFinalizedInput{finalized_from:psbts.input[index].clone(), finalized: failing_input}
10:25 < jeremyrubin> but if it succeeds, then clobber it and swap the psbts.input[index] = suceeding_input
10:25 < jeremyrubin> not sure if that makes sense, but then the error case of finalize would not have dropped all the info out of the failed to finalize psbt
10:26 < jeremyrubin> ahhh
10:27 < jeremyrubin> maybe even simpler would be to have the finalize call just clone the input each time before it tries to finalize and "restore it" if it fails? not great for performance, but better than a whole psbt clone
10:28 < jeremyrubin> or lastly, have the finalize "reset everything" part happen as a separate function, then nothing gets clobbered
10:30 < sanket1729> If the input is mutated, it is always mutated successfully, or it stays unmutated. So, all the desired behaviors can be achieved by looping psbt inputs and calling `finalize_inp`
10:30 < jeremyrubin> that seems not quite right?
10:31 < jeremyrubin> https://github.com/rust-bitcoin/rust-miniscript/blob/6cd1fb66241b08439d8ad2d1335828007519c204/src/psbt/finalizer.rs#L433
10:31 < sanket1729> Yes. If that happens, it is an implementation bug!
10:32 < sanket1729> I can re-arrange the code so that mutation only happens later to make it more clear
10:33 < jeremyrubin> ahh
10:33 < jeremyrubin> wasnt clear to me that was the case
10:34 < jeremyrubin> +1 point for adding "implementation error" error variant so we know if it's a caller issue or a bug
10:35 < jeremyrubin> sanket1729: one example where this is maybe not true is in theory if a signer adds invalid signatures
10:35 < jeremyrubin> a better finalizer might check these things, but interpreter can detect (in theory) bad sigs
10:40 < sanket1729> Yes, true. Good point about signers adding bad signatures. The current interpreter will detect invalid signatures for inputs that it can finalize. It is not only an implementation error than. 
10:40 < sanket1729> Anycase, I can re-arrange the code so that check happens before mutation
11:04 < jeremyrubin> sanket1729 awesome
18:41 -!- warren [~warren@fedora/wombat/warren] has quit [Quit: QUIT]
18:41 -!- warren_ [~warren@fedora/wombat/warren] has joined ##miniscript
--- Log closed Tue Mar 15 00:00:26 2022