Skip to content

feat(selector): allow multiple change sources with ChangeDescriptor#18

Merged
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
nymius:feat/add-ChangeDescriptor-enum
Oct 6, 2025
Merged

feat(selector): allow multiple change sources with ChangeDescriptor#18
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
nymius:feat/add-ChangeDescriptor-enum

Conversation

@nymius
Copy link
Contributor

@nymius nymius commented Aug 8, 2025

Description

Previously, the change_descriptor field only accepted a definite descriptor to derive the change output script pubkey. This worked for most cases but failed for scenarios - such as silent payments - where a definite descriptor is not yet available.

To address this, introduce the ChangeDescriptor enum, which supports:

  • The standard case of providing a definite descriptor.
  • An alternative where the script pubkey and its maximum satisfaction weight can be specified directly.

This makes change output generation flexible enough to handle both standard and exceptional workflows.

Fixes #17

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@nymius nymius self-assigned this Aug 8, 2025
@nymius nymius requested a review from ValuedMammal September 10, 2025 15:58
@nymius nymius force-pushed the feat/add-ChangeDescriptor-enum branch 2 times, most recently from c589508 to bcb018f Compare September 11, 2025 12:49
Previously, the `change_descriptor` field only accepted a definite
descriptor to derive the change output script pubkey. This worked for
most cases but failed for scenarios - such as silent payments - where a
definite descriptor is not available.

Now `change_descriptor` is `change_script` and can take a ScriptSource,
which can be a descriptor or the script itself.

Also, `change_weights` field is added as a field of SelectorParams, to
externalize the calculus of the satisfaction weight from the API.

This allowed the remove of the `to_cs_change_weigths` method.

`From<(ScriptSource, Amount)>` for Output is added for conveniency.
@nymius nymius force-pushed the feat/add-ChangeDescriptor-enum branch from bcb018f to aadeca5 Compare September 27, 2025 09:11
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK aadeca5

@ValuedMammal ValuedMammal merged commit 97847c5 into bitcoindevkit:master Oct 6, 2025
3 checks passed
@nymius nymius deleted the feat/add-ChangeDescriptor-enum branch January 30, 2026 13:35
@evanlinjin
Copy link
Member

ApproachNACK

There is a problem with the current approach: SelectorParams has two independent fields that can both specify the change output's satisfaction weight, and they can disagree.

  • change_script: ScriptSource — when this is the Descriptor variant, the satisfaction weight is derivable from the descriptor via max_weight_to_satisfy.
  • change_weight: DrainWeights — this also encodes the satisfaction weight (the spend cost of the change output).

The caller is responsible for keeping these in sync, but nothing enforces that. If change_script is a Descriptor and the caller provides a DrainWeights with a different satisfaction weight, we silently use the wrong value.

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.

Replace change_descriptor: DefiniteDescriptor field in SelectorParams by change_spk: ScriptBuf

3 participants