Add remove_partial_sigs and try_finalize to SignOptions #621

kafaichoi · 2022-06-04T07:56:36Z

Description

This PR is to add 2 keys(try_finalize and remove_partial_sigs) in SignOptions. See this issue for detail #612

Notes to the reviewers

I found the negative naming of these 2 new keys do_not_finalize and do_not_remove_partial_sigs are a bit confusing(like most negative named paremeter/variable). Should we actually change it back to positive naming(do_finalize and do_remove_partial_sigs)?

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

src/wallet/mod.rs

notmandatory · 2022-06-05T17:45:00Z

I agree do_not makes the naming and and logic overly complicated, would be clearer as do_finalize and do_remove_partial_sigs, or how about try_finalize (since it may not be able to) and remove_partial_sigs.

danielabrozzoni

Concept ACK, the code looks good!
I have a couple of comments.

I agree that the current names are a bit confusing, I really like Steve's try_finalize/remove_partial_sigs suggestion :)

src/wallet/mod.rs

danielabrozzoni · 2022-06-07T12:32:16Z

src/wallet/signer.rs

@@ -665,8 +665,15 @@ pub struct SignOptions {
    /// Whether the signer should use the `sighash_type` set in the PSBT when signing, no matter
    /// what its value is
    ///
-    /// Defaults to `false` which will only allow signing using `SIGHASH_ALL`.
+    /// Defaults to `false` which will remove partial_sigs after input is signed


It looks like you've accidentally modified allow_all_sighashes docs...

rajarshimaitra

tACK c324eb1

Just few nits..

rajarshimaitra · 2022-06-10T14:03:05Z

src/wallet/mod.rs

+                // remove finalized input partial_sigs if remove_partial_sigs is set as true in SignOptions
+                // do not remove finalized input partial_sigs if remove_partial_sigs is set as false in SignOptions.


I feel like this comment is redundant.. It self evident from the statement below what its trying to do.

rajarshimaitra · 2022-06-10T14:09:45Z

src/wallet/signer.rs

+    /// Whether partial_sigs in input should be removed when the psbt inputs are signed in finalizing psbt.
+    ///
+    /// Defaults to `true` which will remove partial_sigs in inputs after signing.


Small doc nit. This sentences can be simplified a little.

Suggested change

/// Whether partial_sigs in input should be removed when the psbt inputs are signed in finalizing psbt.

///

/// Defaults to `true` which will remove partial_sigs in inputs after signing.

/// Whether to remove partial_sigs from psbt inputs while finalizing psbt.

///

/// Defaults to `true` which will remove partial_sigs after finalizing.

rajarshimaitra

tACK be8c9dc

All looks good to me.. Just one minor nit..

rajarshimaitra · 2022-06-11T11:09:36Z

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  - Signing Taproot PSBTs (key spend and script spend)
  - Support for `tr()` descriptors in the `descriptor!()` macro
 - Add support for Bitcoin Core 23.0 when using the `rpc` blockchain
+- Added `remove_partial_sigs` and `try_finalize` to `SignOptions`


Suggested change

- Added `remove_partial_sigs` and `try_finalize` to `SignOptions`

- Add `remove_partial_sigs` and `try_finalize` to `SignOptions`

danielabrozzoni · 2022-06-28T08:34:31Z

Can you please rebase on master, to pick up the CI changes? :)

danielabrozzoni · 2022-06-29T10:15:13Z

tACK 5eac541

rajarshimaitra

tACK 5eac541

modulo https://github.com/bitcoindevkit/bdk/pull/621/files#r895010365

kafaichoi · 2022-07-02T06:31:27Z

tACK 5eac541

modulo https://github.com/bitcoindevkit/bdk/pull/621/files#r895010365

Sorry for replying it previously. I saw some places using "added" so I didn't change it. But yea happy to change that if that's way we want :)

rajarshimaitra

Sorry for replying it previously. I saw some places using "added" so I didn't change it. But yea happy to change that if that's way we want :)

No issues, not a big deal..

ACK 6a79f39

notmandatory

utACK 6a79f39

notmandatory

ReACK e3a17f6

notmandatory · 2022-07-06T17:47:45Z

I noticed the CHANGELOG message ended up in the wrong place, I'll fix it in the release branch.

kafaichoi changed the title ~~Remove partial signature after finalizing psbt~~ Add do_not_finalize and do_not_remove_partial_sigs to SignOptions Jun 4, 2022

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch from 47642ff to 4bbb9ac Compare June 4, 2022 08:10

kafaichoi commented Jun 4, 2022

View reviewed changes

src/wallet/mod.rs Outdated Show resolved Hide resolved

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch from 4bbb9ac to e272a14 Compare June 5, 2022 02:43

danielabrozzoni reviewed Jun 7, 2022

View reviewed changes

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch from e272a14 to 09d4bc6 Compare June 9, 2022 06:42

kafaichoi changed the title ~~Add do_not_finalize and do_not_remove_partial_sigs to SignOptions~~ Add remove_partial_sigs and try_finalize to SignOptions Jun 9, 2022

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch 4 times, most recently from c018a7a to c324eb1 Compare June 9, 2022 07:50

rajarshimaitra reviewed Jun 10, 2022

View reviewed changes

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch 3 times, most recently from 726a14d to be8c9dc Compare June 11, 2022 08:48

rajarshimaitra approved these changes Jun 11, 2022

View reviewed changes

notmandatory assigned kafaichoi Jun 21, 2022

notmandatory mentioned this pull request Jun 21, 2022

W25 BDK Library Team Call bitcoindevkit/.github#9

Closed

14 tasks

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch 2 times, most recently from 46a2f4e to 5eac541 Compare June 29, 2022 05:39

rajarshimaitra suggested changes Jul 1, 2022

View reviewed changes

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch from 5eac541 to 6a79f39 Compare July 2, 2022 06:30

rajarshimaitra approved these changes Jul 3, 2022

View reviewed changes

notmandatory added this to the Release 0.20.0 Feature Freeze milestone Jul 4, 2022

notmandatory approved these changes Jul 4, 2022

View reviewed changes

notmandatory mentioned this pull request Jul 5, 2022

W27 BDK Library Team Call bitcoindevkit/.github#12

Closed

24 tasks

kafaichoi added 2 commits July 6, 2022 17:10

add remove_partial_sigs to SignOptions

c2e4ba8

add try_finalize to SignOptions

e3a17f6

kafaichoi force-pushed the remove-partial-signature-after-finalizing-psbt branch from 6a79f39 to e3a17f6 Compare July 6, 2022 10:13

notmandatory approved these changes Jul 6, 2022

View reviewed changes

notmandatory merged commit dd51380 into bitcoindevkit:master Jul 6, 2022

This was referenced Jul 6, 2022

Remove partial signatures after finalizing the psbt #612

Closed

Broadcast should tell user to finalize PSBTs bitcoindevkit/bdk-cli#97

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remove_partial_sigs and try_finalize to SignOptions #621

Add remove_partial_sigs and try_finalize to SignOptions #621

kafaichoi commented Jun 4, 2022 •

edited

Loading

notmandatory commented Jun 5, 2022

danielabrozzoni left a comment

danielabrozzoni Jun 7, 2022

kafaichoi Jun 9, 2022

rajarshimaitra left a comment

rajarshimaitra Jun 10, 2022

rajarshimaitra Jun 10, 2022

rajarshimaitra left a comment

rajarshimaitra Jun 11, 2022

danielabrozzoni commented Jun 28, 2022

danielabrozzoni commented Jun 29, 2022

rajarshimaitra left a comment

kafaichoi commented Jul 2, 2022

rajarshimaitra left a comment •

edited

Loading

notmandatory left a comment

notmandatory left a comment

notmandatory commented Jul 6, 2022

		// remove finalized input partial_sigs if remove_partial_sigs is set as true in SignOptions
		// do not remove finalized input partial_sigs if remove_partial_sigs is set as false in SignOptions.

	- Added `remove_partial_sigs` and `try_finalize` to `SignOptions`
	- Add `remove_partial_sigs` and `try_finalize` to `SignOptions`

Add remove_partial_sigs and try_finalize to SignOptions #621

Add remove_partial_sigs and try_finalize to SignOptions #621

Conversation

kafaichoi commented Jun 4, 2022 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

notmandatory commented Jun 5, 2022

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni Jun 7, 2022

Choose a reason for hiding this comment

kafaichoi Jun 9, 2022

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra Jun 10, 2022

Choose a reason for hiding this comment

rajarshimaitra Jun 10, 2022

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra Jun 11, 2022

Choose a reason for hiding this comment

danielabrozzoni commented Jun 28, 2022

danielabrozzoni commented Jun 29, 2022

rajarshimaitra left a comment

Choose a reason for hiding this comment

kafaichoi commented Jul 2, 2022

rajarshimaitra left a comment • edited Loading

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

notmandatory commented Jul 6, 2022

kafaichoi commented Jun 4, 2022 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading