IsAllFromMe

kallewoof commented at 5:25 am on February 22, 2018: member

Rebases #9167 by @morcos on master.

Why do this: this is mostly a refactor which fixes some bugs, e.g. fee in gettransaction for mixed owner inputs.

kallewoof force-pushed on Feb 22, 2018

ken2812221 commented at 11:15 am on February 22, 2018: contributor

What’s the purpose of changing coding style?

kallewoof commented at 12:51 pm on February 22, 2018: member

@ken2812221 To abide by the coding style conventions (see contributing on main page).

laanwj added the label Wallet on Feb 22, 2018

jnewbery commented at 8:44 pm on April 4, 2018: member

Needs rebase

kallewoof force-pushed on Apr 6, 2018

kallewoof commented at 0:41 am on April 9, 2018: member

So, the new IsAllFromMe code does not check if GetDebit(ISMINE_ALL) > 0, which IsMine does. The result is that the existing code will say ’not eligible’ and the new code will say ’eligible’, for many of the coin selector test cases. I think adding a check for GetDebit to IsAllFromMe is the right approach, but I believe @morcos removed this part for a reason in #9167.

Ping @morcos. Still investigating the issue, so this may become irrelevant.

Edit: It seems this:

0CTransaction(hash=777c42d727, ver=2, vin.size=0, vout.size=1, nLockTime=0)
1    CTxOut(nValue=0.01000000, scriptPubKey=)

is considered all mine. Which technically speaking is correct, but CWalletTx::GetDebit explicitly returns 0 (i.e. not mine) for vin.empty(), so I might adapt that logic in IsAllFromMe as well.

kallewoof commented at 2:40 am on April 9, 2018: member

I believe fdb1d59 (count 0-vin transactions as not mine in IsAllFromMe) and 5073476 (consider coinbase transaction as all mine in CWallet) address the problems, but I’m not sure the latter is entirely correct in all cases. If a random person checks a coinbase transaction, I think it will claim it is theirs now. I think adding a ‘can spend’ check should suffice here.

Edit: CWallet::IsAllFromMe is only used in the feebumper, and since coinbase transactions can’t be fee-bumped anyway, 5073476 should be fine. To clarify: the IsFromMe code already behaves this way (i.e. consider coinbase transactions as from me, if in miner wallet); it may be worth it to fall back to IsFromMe style check of GetDebit(..) > 0 though.

kallewoof force-pushed on May 10, 2018

Empact commented at 5:21 pm on May 17, 2018: member

utACK ee25bc5

sipa commented at 6:37 pm on May 17, 2018: member

Can you add a PR description? It’s unclear what this is trying to solve without reading the code (the same is true for the original PR, which lists the changes but not the goal).

kallewoof commented at 5:05 am on May 18, 2018: member

@sipa From my understanding (and I could be wrong), it’s mostly a refactor which fixes some bugs, e.g. fee in gettransaction for mixed owner inputs. Am I missing something, @morcos? If not, I’ll update the PR description.

Empact commented at 8:01 am on May 18, 2018: member

@kallewoof I would draw from the commit messages, it’s fairly clear if you interpret them in aggregate.

kallewoof commented at 8:10 am on May 18, 2018: member

@Empact That was what I intended with the above description. Did I miss anything?

Empact commented at 6:52 am on May 21, 2018: member

Here’s my interpretation of the commits:

Change OutputEligibleForSpending to only consider outputs trusted / mine if all their inputs are mine, rather than just one of them.
Change gettransaction rpc to only include fee information if all inputs pass the relevant filter, rather than just one.
Change CWallet::IsAllFromMeto consider coinbase transactions mine.

kallewoof commented at 11:18 pm on May 21, 2018: member

@Empact That’s a description of what is done, not what the purpose is, which is kinda what the original PR did. I think @sipa wants a description of why not what.

DrahtBot closed this on Jul 22, 2018

DrahtBot commented at 11:50 pm on July 22, 2018: member

DrahtBot reopened this on Jul 22, 2018

DrahtBot added the label Needs rebase on Jul 24, 2018

kallewoof force-pushed on Jul 25, 2018

DrahtBot removed the label Needs rebase on Jul 25, 2018

DrahtBot added the label Needs rebase on Aug 25, 2018

[wallet] Add wtx caching of IsAllFromMe 04af007615

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

There is no change in behavior.

8075c471f4

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

b5ff60264d

[wallet] Remove redundant check for IsFromMe from GetAddressBalances.

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

1942d15426

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

5b503f1200

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

No change in behavior.

3dfa33c5ed

wallet: Consider coinbase transactions as 'all mine' in CWallet b50439bfaa

kallewoof force-pushed on Sep 10, 2018

DrahtBot removed the label Needs rebase on Sep 10, 2018

DrahtBot commented at 1:24 pm on September 21, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14437 (Refactor: Start to separate wallet from node by ryanofsky)
#13756 (wallet: “avoid_reuse” wallet flag for improved privacy by kallewoof)
#12297 (Improve CWallet::IsAllFromMe for false results by promag)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

DrahtBot commented at 3:16 pm on November 9, 2018: member

DrahtBot added the label Needs rebase on Nov 9, 2018

kallewoof commented at 4:56 am on November 12, 2018: member

Apologies for picking this up and then dropping the ball, but I don’t feel comfortable in my understanding of this code enough to keep rebasing this beyond this point. I suggest reopening the original PR #9167 (marked up for grabs) and ignoring my work on top of it.

kallewoof closed this on Nov 12, 2018

kallewoof deleted the branch on Nov 12, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

IsAllFromMe #12508

Conflicts