-
Notifications
You must be signed in to change notification settings - Fork 990
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
fix: wallet coin selection respects max_block_weight #2546
fix: wallet coin selection respects max_block_weight #2546
Conversation
Deprecate "soft" max_outputs limit and introduce "hard" max_outputs limit based on max_block_weight.
Looks like this introduces some bona fide test compilation failures. |
@ignopeverell my fault, fixed! |
@@ -253,7 +262,7 @@ where | |||
amount, | |||
current_height, | |||
minimum_confirmations, | |||
max_outputs, | |||
calculate_max_inputs_in_block(change_outputs), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if this is a misunderstanding, I'm not too familiar with this code. But shouldn't you calculate the max number of inputs for change_outputs + 1
to account for the receiver's output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, but first, we need to understand do we need a change. First, we calculate assuming that there will be no change (this gives us also the lowest fee). We check do we have fee+amount
on our balance or not and only then we figure out do we need a change output or we can transfer without it.
If we need one - we set:
let num_outputs = change_outputs + 1; |
and then select coins using
num_outputs
grin/wallet/src/libwallet/internal/selection.rs
Lines 322 to 330 in 6a6a423
coins = select_coins( | |
wallet, | |
amount_with_fee, | |
current_height, | |
minimum_confirmations, | |
calculate_max_inputs_in_block(num_outputs), | |
selection_strategy_is_use_all, | |
parent_key_id, | |
) |
At least that's how I get it, probably @yeastplume can verify does this make sense or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct, that's what happens.
LGTM, but merge conflict introduced, sorry. I think I'm okay with removing the concept of a manually specifiable |
Deprecate "soft" max_outputs limit and introduce "hard" max_outputs
limit based on max_block_weight. Should fix #2510