Skip to content

Feat/coin selection strategies - #151

Merged
cygnet3 merged 8 commits into
masterfrom
feat/coin_selection_strategies
Oct 15, 2026
Merged

Feat/coin selection strategies#151
cygnet3 merged 8 commits into
masterfrom
feat/coin_selection_strategies

Conversation

@Sosthene00

@Sosthene00 Sosthene00 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch from e3ca307 to f77c5cc Compare August 12, 2026 10:34
yancyribbens
yancyribbens reviewed Aug 12, 2026
View reviewed changes
Comment thread spdk-wallet/src/client/coin_select.rs Resolved
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch 4 times, most recently from 5417acb to cf0c6e7 Compare August 26, 2026 11:37
@Sosthene00
Sosthene00 marked this pull request as ready for review August 26, 2026 11:37
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch 3 times, most recently from 3d2e376 to 5dcb753 Compare September 8, 2026 08:40
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch 4 times, most recently from 8238cc4 to 3165e06 Compare September 17, 2026 20:45
@Sosthene00

Sosthene00 commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Significant changes for this new version, trying to make it simpler :

  • removed all code related to mandatory inputs
  • removed all code related to multiple change outputs
  • Drain is not a strategy anymore, it wasn't making much sense
  • InputSelection doesn't expose all fields as pub, and has getters instead
  • Better algorithm for strategies, we only create a Greedy proposal if the others failed.
  • BNB_MAX_ROUNDS can be passed as an argument to relevant methods (default value stays as before though)
  • Shorter commits history

@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch 3 times, most recently from e03df6f to 1efb1c5 Compare September 22, 2026 14:57
sdmg15
sdmg15 reviewed Sep 22, 2026
View reviewed changes

@sdmg15 sdmg15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tAck 1efb1c5#r4074784031.
Left few nits.
A question: Can we have a more straight function for users that just let them create the transaction but with defaults?

Comment thread spdk-wallet/src/client/coin_select.rs Resolved
Comment thread spdk-wallet/src/client/spend.rs Resolved
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch from 1efb1c5 to 8235f27 Compare September 23, 2026 11:55
@Sosthene00

Sosthene00 commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

tAck 1efb1c5#r4074784031. Left few nits. A question: Can we have a more straight function for users that just let them create the transaction but with defaults?

I think we can, but since a lot things will immediately change with introduction of psbt right after this PR I'd rather wait for things to settle before thinking of shortcuts and quality of life functions

@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch 2 times, most recently from 20958c6 to 4007508 Compare September 26, 2026 15:40
@cygnet3
cygnet3 force-pushed the feat/coin_selection_strategies branch from 4007508 to 33dcefe Compare September 28, 2026 12:17
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch 2 times, most recently from 72e4dca to 38dd6ab Compare September 30, 2026 12:01
@still-gleam still-gleam mentioned this pull request Sep 30, 2026
Closed
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch 2 times, most recently from ff02d6e to 143bbaa Compare October 1, 2026 17:34
@cygnet3
cygnet3 force-pushed the feat/coin_selection_strategies branch from 143bbaa to d7ab277 Compare October 3, 2026 12:42
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch from d7ab277 to c34639f Compare October 3, 2026 13:30
@cygnet3
cygnet3 force-pushed the feat/coin_selection_strategies branch 6 times, most recently from 44cf957 to 037d5dd Compare October 4, 2026 21:35
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch from 037d5dd to aec4b3e Compare October 7, 2026 13:06
@Sosthene00

Sosthene00 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

As discussed on another channel I rolled back the change on Strategy and InputSelection and kept the available_utxos as HashMap. I think it makes sense like that and I will work to update Dana accordingly

Sosthene00 and others added 5 commits October 9, 2026 11:16
@Sosthene00
feat: add coin_select module
e992bc2 
Introduce Changeless, LowestFee, and FeeRateCap BnB strategies with a
Greedy fallback, plus a separate DrainSelection for spend-all.
@Sosthene00
refactor: split coin selection from tx construction
dc9bf67 
Propose selections independently of building the unsigned transaction,
and keep coin_select internals out of the crate's public API.
@Sosthene00 @cursoragent
fix: size OP_RETURN data against the push limit
85fe109 
DATA_CARRIER_SIZE truncated weight estimates and rejected spends above
205 bytes. Drop it so selection and construction share the PushBytes bound.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cygnet3 @Sosthene00
refactor: parse Data in RecipientAddress as PushBytesBuf
0d7c0cb
@cygnet3 @Sosthene00
refactor: rename some functions to make their purpose clearer
783d82b 
rename check_recipient_networks -> validate_recipient_networks

rename prevouts -> validate_and_convert_to_txout

rename candiate_from_txout -> validate_and_convert_to_candidate
@Sosthene00
Sosthene00 force-pushed the feat/coin_selection_strategies branch from aec4b3e to dafc57c Compare October 9, 2026 09:16
cygnet3
cygnet3 reviewed Oct 14, 2026
View reviewed changes
Comment thread spdk-wallet/src/client/structs.rs
/// Wallet change amount (zero for drain / changeless selections).
pub change: Amount,
/// Index into `recipients` of the change output, if any.
pub change_index: Option<usize>,

@cygnet3 cygnet3 Oct 14, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These new fields have some data duplication. Instead of having a change field, can't we read the change amount using the change_index and the recipients fields? Also the fee can be read by subtracting the output_sum from the input_sum. Is there a reason for having these as separate fields?

@cygnet3 cygnet3 Oct 15, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'll make a separate PR for that to keep this PR untouched

@cygnet3
cygnet3 force-pushed the feat/coin_selection_strategies branch from dafc57c to 678d2de Compare October 15, 2026 08:51
@cygnet3
refactor: provide available_utxos as a hashmap
1624522 
Provide the available utxos as a hashmap rather than an array slice of
tuples. This has 2 advantages:

- We don't need to iterate over the entire list when looking for a txout
associated with a selected outpoint, we can just use '.get'
- It automatically guarantees that any outpoint is unique, since we
can't have duplicate keys
@cygnet3
refactor: inline validate_and_convert_to_candidate
b78f050 
This function is relatively straightforward and is only used in 1 place,
so inline it instead.
@cygnet3
lint: allow implicit_hasher
3f2fa1c 
Unless there is an interest to allow for custom hashers, just ignore
this pedantic error.
@cygnet3
cygnet3 force-pushed the feat/coin_selection_strategies branch from 678d2de to 3f2fa1c Compare October 15, 2026 08:53
@cygnet3
cygnet3 merged commit dc6c474 into master Oct 15, 2026
2 checks passed
@cygnet3
cygnet3 deleted the feat/coin_selection_strategies branch October 15, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cygnet3 cygnet3 cygnet3 left review comments
+3 more reviewers
@yancyribbens yancyribbens yancyribbens left review comments
@sdmg15 sdmg15 sdmg15 left review comments
@evanlinjin evanlinjin evanlinjin left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Sosthene00 @yancyribbens @sdmg15 @evanlinjin @cygnet3