IsAllFromMe

[morcos]

Created a new wallet and walletTx function IsAllFromMe which correctly computes whether all the inputs to a transaction match the requested IsMine filter.

Original first commit 766e8a4 already merged.

• Commits 1 and 6 add the new function and remove the old walletTx IsFromMe.
• Commit 2 update IsTrusted to use the new function. No change in behavior as long as vin.size() > 0. Open question as to whether this should be changed to use ISMINE_ALL?
• Commit 4 remove code that was already redundant because of call to IsTrusted
• Commit 3 make a small change to SelectCoinsMinConf to only consider new outputs spendable quickly if all the inputs were ours, not just at least one.
• Commit 5 changes the output in gettransaction for mixed debit transactions where some inputs were ours and some weren't. It'll report the correct fee if possible, otherwise 0. Note that the fee reported in the details and any other function which depends on ListTransactions is not changed as getbalance("*") depends on having incorrect negative fees calculated on mixed debit transactions in order to track the right balances.

Note that there are other places in the code such as AddToWalletIfInvolvingMe which ideally would be updated to distinguish between 0 satoshi prevouts that are "MINE" and prevouts that aren't "MINE".

[jonasschnelli]

Concept ACK

[gmaxwell]

Fee: 0 is perhaps confusing?

SelectCoinsMinConf behavior change sounds great.

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

[morcos]

@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.

[morcos]

rebased after #8580

[gmaxwell]

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,

[morcos]

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.

[morcos]

@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

[gmaxwell]

ACK.

[ryanofsky]

• 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.

[morcos]

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.

[ryanofsky]

utACK 1bab789

[TheBlueMatt]

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?

[morcos]

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

[ryanofsky]

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

[NicolasDorier]

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.

[jonasschnelli]

Needs rebase.

[laanwj]

Still needs rebase.

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

[morcos]

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.