GetLegacyBalance returns arbitrarily different values than GetBalance, sometimes even negative balances
It is only used to throw a warning in some cases, so instead of fixing it, it should be safe to remove it (since the "balance check" is done by the wallet internally).
The first commit is a scripted-diff that does not change the objdump of bitcoind for me and seems like a nice cleanup to me.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/const CWalletTx ?\* ?pcoin = &/const CWalletTx\& wtx = /g' src/wallet/wallet.cpp
sed -i -e 's/\<pcoin->/wtx./g' src/wallet/wallet.cpp
sed -i -e 's/\<pcoin\>/\&wtx/g' src/wallet/wallet.cpp
-END VERIFY SCRIPT-
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
GetLegacyBalance is the only one that returns the correct values... The newer stuff is still broken.
A negative balance can clearly not be correct
@MarcoFalke This PR looks a bit unfinished: the calculated totalAmount is never used after this change?
Can you describe the code path that will be taken that makes the GetLegacyBalance call technically redundant?
Negative balances can indeed be correct. Receives are only counted once they reach a certain confirmation level. But sends (including ones that use unconfirmed inputs) are always counted. So if you receive 50 BTC, and send 40 BTC, until the 50 BTC confirms, you have a balance of -40 BTC.
Can you describe the code path that will be taken that makes the GetLegacyBalance call technically redundant?
It is practically redundant in that it may fail in cases where we'd later create the transaction because we have sufficient balance and it may not fail in cases where we are unable to create a transaction due to insufficient balance.
So the check is at best confusing. Definitely not enough reason to keep that code around in sendmany. Especially considering that none of the other RPCs make this check. Compare for example sendtoaddress.
The PR seems unfinished:
The minconf API argument passed by the user is being ignored after your change:
{"minconf", RPCArg::Type::NUM, /* default */ "1",
"Only use the balance confirmed at least this many times."},
Drop that argument?
Also totalAmount is calculated but not used.
TBH these kind of things should really be detected automatically using compiler warnings or a linter before hitting human review. Human review time is scarce.
Going to reopen later, maybe
@MarcoFalke What was the egg (🥚) + hash you posted initially about? :-)
Concept ACK