Similar to #15596.

@MarcoFalke is this what you mean in #15702?

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• rpc: replace raw pointers with shared_ptrs #18592 (rpc: replace raw pointers with shared_ptrs by brakmic)
• doc: Update docs for getbalance (default minconf should be 0) #18502 (doc: Update docs for getbalance (default minconf should be 0) by uzyn)
• refactor: consolidate sendmany and sendtoaddress code  #18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This should throw an exception, not simply be ignored...

@luke-jr I'm doing the same as #15596. Why throw an error?

Because if I specify minconf=10, I expect the value with received transactions less than 10 blocks deep to be excluded. Returning a wrong answer is not a reasonable API change.

#15596 should probably be throwing an error too.

@luke-jr how about renaming to min_conf_theirs=10? That wouldn't cause confusion and allows to take into account change (or send to self) outputs with 1 confirmation.

@luke-jr I'm doing the same as #15596. Why throw an error?

#15596 has always ignored the minconf parameter, getbalance did not. So I think an error is fine here.

This should throw an exception, not simply be ignored...

Yes, error is definitely better. Programs ignoring arguments can be incredibly frustrating.

Then when should it throw an error? minconf > 0?

minconf != 0

Now raising error.

This is ready?

Is this still needed now that #15596 is merged?

Also wallet label?

Ping @meshcollider.

With the many minconf PRs and arguments we've had, I think we should at least briefly discuss this in the meeting to make sure people that haven't looked here are ok with the change

Conditional NACK until this has good release notes.

I don't like this change, but without a better alternative I think it might be an improvement over the status quo. The release notes here are bad though. If you're going to remove a feature ("tell me my balance ignoring incoming payments with less than min_conf confirmations") that once worked well enough and was more recently broken you should say why you are removing it and what possible alternatives are. I can try to write up some better release notes for this change today, and if anybody else has more information about past or present problems with this feature, it would be useful to post here. (Last discussions I remember having about this were about theoretical problems in potential future implementations of this feature.)

until this has good release notes.

Agree, these are poor, don't explain the motivation.

I don't like this change, but without a better alternative I think it might be an improvement over the status quo.

I was under the impression there was no intention in fixing the feature. If that's not the case then this should be closed.

Last discussions I remember having about this were about theoretical problems in potential future implementations of this feature

Yes, there's #14602 and also related discussion in #15596.

🐙 This pull request conflicts with the target branch and needs rebase.

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Marked "up for grabs"