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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eagerly finalize inputs #171

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

LLFourn
Copy link
Contributor

@LLFourn LLFourn commented Nov 17, 2020

If we know the final witness/scriptsig for an input we should add it right away to the PSBT. Before, if we couldn't finalize any of them we finalized none of them. Also we don't try and re-finalize inputs that have already been finalized.

This is needed for foreign UTXOs since you don't have the descriptor for the other guy's input^.

Ready for concept acks but I'll write a test for it before merge.

^It would be nice if we could attach descriptors to PSBTs. I am not sure what the approach should be for validating finalized witness/scriptsig.

@afilini
Copy link
Member

afilini commented Nov 17, 2020

Definitely concept ACK, it seems a much smarter approach compared to what we were doing before!

@afilini
Copy link
Member

afilini commented Nov 20, 2020

Now that I think about it, wouldn't it make more sense to also return another boolean (or maybe, for clarity, a struct with named fields) to tell the user whether we've actually signed something? Because currently we only have is_finalized, but that's not really useful if you have inputs from multiple parties since generally you can't finalize the tx entirely, but maybe you can sign a few inputs.

And fix the fallout.
@LLFourn LLFourn force-pushed the wallet/eagerly_finalize_inputs branch from 9aaf1bb to 3ac9a5a Compare November 23, 2020 04:27
If we know the final witness/scriptsig for an input we should add it
right away to the PSBT. Before, if we couldn't finalize any of them we
finalized none of them.
@LLFourn LLFourn force-pushed the wallet/eagerly_finalize_inputs branch from 3ac9a5a to acc0ae1 Compare November 23, 2020 05:08
@LLFourn
Copy link
Contributor Author

LLFourn commented Nov 23, 2020

Test added. It's based on this PR so it can compile: #187. Ready for final review once that's sorted.

@afilini I see your point. It would be nice to know if progress was made. For my use the "is_finalized" is enough to get what I need done. I don't have a strong vision for that yet though so maybe another PR unless your confident in a particular design?

@afilini
Copy link
Member

afilini commented Nov 23, 2020

I don't have a strong vision for that yet though so maybe another PR unless your confident in a particular design?

Yeah that's good for me, I'm still not sure if it's better to just say "i've signed something" or maybe something more detailed like a list of which keys signed, etc. So for now it can stay like that and we can have a future upgrade to add those stuff, maybe as part of a broader refactoring of the whole policy module which really needs some work.

@afilini afilini merged commit acc0ae1 into bitcoindevkit:master Nov 23, 2020
nickfarrow pushed a commit to nickfarrow/bdk that referenced this pull request Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants