coin-grinder selection algorithm does not take cost_of_change as a parameter.
CoinGrinder doesn’t use coin_params.m_cost_of_change and the comparison of selection results from CoinGrinder, SRD, and Knapsack does not need it either as only the weight of the selections is relevant. So, line 96 in that fuzz test is both incorrect and dead code. It should be removed.
cite: stackexchange
verified coin_grinder fuzz target still works after removal
coin-grinder selection algorithm does not take cost_of_change as a
parameter.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | brunoerg, frankomosh |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK 5ec6446a1b83e673a51d9e86fa4c0c0e9930fccc
m_cost_of_change isn't used for anything of coin-grinder, it's a no-op in the target and it's fine to remove it.
Isn't this sort of a duplicate and should be removed also?
My proposed change is for coin-grinder which does not use a cost_of_change parameter. This parameter here is in the bnb fuzz_target, so I would not believe it to be here in error.
Sorry, my question wasn’t clear. Yes, the bnb target needs m_cost_of_change. I only meant this one line: its value is replaced on L233 before anything reads it. Was flagged a while back if I am not wrong (https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201745084).
However its not blocking, and I guess its probably fine to leave as is, here.
Oh, I see what you mean now. Yeah, that looks like another oversight but unrelated to this PR. Hmm, it would be pretty easy to write a tool or use a tool to find re-assignments like this. Anyway, thanks for the ACK on this PR, its probably best to leave it as is since its so difficult to get some ACKs around here!
Concept ACK
@frankomosh still seeking ACK
ACK 5ec6446a1b83e673a51d9e86fa4c0c0e9930fccc