Transactions: don't expose mutable access to inner PSBTs

danielabrozzoni commented 3 years ago

Co-Authored-by: Sebastian Geisler sebastian@blockstream.io Co-Authored-by: Daniela Brozzoni daniela@revault.dev

darosior commented 3 years ago

Looks really good. Thanks Daniela and @sgeisler :)

Did you do some testing on downstream projects?

darosior commented 3 years ago

Also, please remove Signed-off-by from the commit. It's really your work and Sebastian's, not mine!

danielabrozzoni commented 3 years ago

Did you do some testing on downstream projects?

Nope, will do

darosior commented 3 years ago

reACK fe1e31f0427dab17741c6637c2b89de6aad51702

danielabrozzoni commented 3 years ago

After a bit of testing:

cosignerd https://github.com/revault/cosignerd/blob/ed0c9a5402aa0c9d20130de4584f7838539a8832/src/processing.rs#L95-L101 this seems easy to fix with add_signature https://github.com/revault/cosignerd/blob/ed0c9a5402aa0c9d20130de4584f7838539a8832/src/processing.rs#L111-L135 I'm not sure about this one... it seems done that way to add signatures only at the end, when all the validation is done, but I'm not sure why we should do so.
revault_net: seems ok
coordinatord: seems ok
revaultd: L O L https://github.com/revault/revaultd/blob/master/src/daemon/control.rs#L791-L793 maybe we need a "clear_sigs" method or something like that? A lot of psbt_mut used in the tests to add/remove inputs... It should be easy fixable

TL;DR: all good, maybe we need a "clear_signatures" method

darosior commented 3 years ago

Thanks for investigating. I'll take care of that

darosior commented 3 years ago

ACK fe1e31f0427dab17741c6637c2b89de6aad51702

revault / revault_tx