IsAllFromMe

morcos commented at 8:30 pm on November 15, 2016: member

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 https://github.com/bitcoin/bitcoin/pull/8456/commits/766e8a40b478353a89622f42809ddb11e695a0c9 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”.

fanquake added the label Wallet on Nov 16, 2016

jonasschnelli commented at 9:47 am on November 16, 2016: contributor

Concept ACK

gmaxwell commented at 6:20 am on November 22, 2016: contributor

Fee: 0 is perhaps confusing?

SelectCoinsMinConf behavior change sounds great. @AdamISZ Joinmarket results in a lot of transactions like this, thoughts?

morcos commented at 9:16 pm on November 29, 2016: member

@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 force-pushed on Dec 5, 2016

morcos commented at 8:08 pm on December 5, 2016: member

rebased after #8580

gmaxwell commented at 6:20 pm on December 9, 2016: contributor

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 force-pushed on Dec 13, 2016

morcos commented at 6:02 pm on December 13, 2016: member

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 commented at 4:01 pm on January 4, 2017: member

@gmaxwell from IRC in regards to listtransactions and gettransaction:

014:01:01 < morcos> if you ran that same command on the old code your grep would find fee twice
114:01:26 < morcos> it prints an overall fee for the transaction and it prints a fee for each accounting entry
214: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
314:02:42 < gmaxwell> ah, I see. what the heck is the fee for the accounting entries for?
414:02:43 < morcos> i changed that logic to be whether all of it was from you
514:02:52 < morcos> don't get me started on that
614:03:23 < morcos> getbalance("*") uses those random incorrect negative fees to offset other errors in tracking balances
714:03:29 < morcos> so it was not possible to fix those
814:03:49 < morcos> i suppose we could not print them, but that seems like an api change

gmaxwell commented at 4:07 pm on January 4, 2017: contributor

ACK.

ryanofsky commented at 11:49 pm on January 18, 2017: member

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 force-pushed on Jan 19, 2017

morcos commented at 2:58 pm on January 19, 2017: member

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 commented at 4:16 pm on January 19, 2017: member

utACK 1bab7890fea0a4c13ad0632e95f304403213ee3e

TheBlueMatt commented at 3:59 pm on February 7, 2017: member

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?

[wallet] Add wtx caching of IsAllFromMe 025faaeea0

[wallet] Replace calculation in IsTrusted to use new IsAllFromMe.

There is no change in behavior.

10832d4f97

[wallet] Only consider coins as mine for spending if all inputs are mine

Change coin selection in SelectCoinsMinConf to only consider coins mine for spending at the fewer required confirmations if all inputs being spent are mine instead of requiring at least one.

de772f7b5a

[wallet] Remove redundant check for IsFromMe from GetAddressBalances.

No change in behavior. IsTrusted was already a stricter check.

473da63292

[rpc] Only calculate fee in gettransaction if we are sure we can.

Fee is now only displayed if all inputs meet the required filter, so if displayed the fee will always be correct.  Unusual fees may still be displayed in details.

faa1ec5f5a

[refactor] Inline last case of wtx.IsFromMe and remove function

No change in behavior.

acbbc052ce

morcos force-pushed on Feb 14, 2017

morcos commented at 6:12 pm on February 14, 2017: member

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

ryanofsky commented at 11:43 pm on February 27, 2017: member

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

NicolasDorier commented at 6:20 am on March 8, 2017: contributor

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 commented at 8:10 pm on August 15, 2017: contributor

Needs rebase.

laanwj commented at 1:04 pm on October 4, 2017: member

Still needs rebase.

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

morcos commented at 2:41 pm on November 9, 2017: member

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.

MarcoFalke added the label Up for grabs on Jan 30, 2018

MarcoFalke commented at 12:52 pm on January 30, 2018: member

Needs rebase. (Also marked “Up for grabs”, just in case…)

morcos commented at 2:50 pm on February 22, 2018: member

Closing in favor of #12508 Thanks @kallewoof

morcos closed this on Feb 22, 2018

DrahtBot locked this on Sep 8, 2021

IsAllFromMe #9167