Concept ACK

Fee: 0 is perhaps confusing?

SelectCoinsMinConf behavior change sounds great.

@AdamISZ Joinmarket results in a lot of transactions like this, thoughts?

@gmaxwell I think that was just a mistake in my description, it looks like it doesn't report anything for the fee unless it has the correct fee. If it can't calculate the fee it doesn't subtract anything when reporting the amount.

rebased after #8580

I expected this to leave out the fee field on coinjoins in listtransactions and gettransaction but find it isn't:

$ ./bitcoin-cli gettransaction 14947302eab0608fb2650a05f13f6f30b27a0a314c41250000f77ed904475dbb | grep fee
"fee": 50000.31337000,

I replaced the first commit with a different formulation by @sdaftuar that I like better. The functionality is the same and all the other commits are unchanged.

@gmaxwell from IRC in regards to listtransactions and gettransaction:

14:01:01 < morcos> if you ran that same command on the old code your grep would find fee twice
14:01:26 < morcos> it prints an overall fee for the transaction and it prints a fee for each accounting entry
14:02:15 < morcos> it was already the case that if it didn't think the tx was from you (meaning none of it was from you) it would leave off the overall fee for the tx
14:02:42 < gmaxwell> ah, I see. what the heck is the fee for the accounting entries for?
14:02:43 < morcos> i changed that logic to be whether all of it was from you
14:02:52 < morcos> don't get me started on that
14:03:23 < morcos> getbalance("*") uses those random incorrect negative fees to offset other errors in tracking balances
14:03:29 < morcos> so it was not possible to fix those
14:03:49 < morcos> i suppose we could not print them, but that seems like an api change

ACK.

• Commits 1, 2, 3, 5, and 7 look good and do not appear to change behavior at all assuming we don't call IsTrusted on a transaction with 0 inputs like you mentioned. But maybe it would be good to assert that IsTrusted isn't called on a transaction with no inputs? If not, I would think if IsTrusted were called on a coinbase transaction or something with no inputs, the old behavior of returning false would be better than new behavior of potentially returning true.
• I don't have enough context to understand commit 4 yet.
• Commit 6 seems like an improvement, avoiding returning meaningless negative fees, though it would seem more direct to avoid the IsMine() code entirely here and just check whether input transactions exist in the wallet (maybe with a IsAllInWallet function instead of IsAllFromMe).

Commit messages could be a little better, and could mention whether the changes affect behavior or not.

Rebased with more verbose commit messages.

@ryanofsky's question about the gettransaction commit led me to notice there was a bug in that commit, where we were outputting the fee based on whether transaction was IsAllFromMe(ISMINE_ALL) but calculating the debit used for the fee based on the passed in filter. This has been changed so the fee is now only output if all inputs meet the required filter. I believe this is a strict improvement from current behavior. The suggestion to try to output fees based on whether or not its possible to calculate the correct fee seems a bit of a half measure, and that a future PR could go all the way to saving require stated to always know the fee (as has been previously discussed).

All valid transactions have vin.size() > 0, so I'm not worried about the IsAllFromMe defaulting to true. Certainly any transaction with vin.size() == 0 couldn't be in the mempool and therefore couldn't pass IsTrusted anyway.

utACK 1bab7890fea0a4c13ad0632e95f304403213ee3e

I believe part of this was merged with bumpfee. Not sure it needs rebase, but a rebase would be nice to see what else is left?

rebased to accommodate first commit already being merged. no changes.

utACK acbbc05 (rechecked and confirmed changes identical to 1bab7890fea0a4c13ad0632e95f304403213ee3e)

I would have used an enum with value (DIRTY, YES, NO) and stored an array of it in WalletTx indexed by ISMINE_* value, instead of having to declare two boolean by ISMINE_* version we want to support later.

Needs rebase.

Still needs rebase.

If you intend to no longer maintain this, please close it.

This has some bug fixes so it should probably still happen, but I'll need to spend a bit of time to make sure the rebase still makes sense since there have been a bunch of changes. I'll bring it to a resolution shortly.

Needs rebase. (Also marked "Up for grabs", just in case...)

Closing in favor of #12508
Thanks @kallewoof