IsAllFromMe

[kallewoof]

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.

[ken2812221]

What's the purpose of changing coding style?

[kallewoof]

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

[jnewbery]

Needs rebase

[kallewoof]

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:

CTransaction(hash=777c42d727, ver=2, vin.size=0, vout.size=1, nLockTime=0)
    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]

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.

[Empact]

utACK ee25bc5

[sipa]

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]

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

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

[kallewoof]

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

[Empact]

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]

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

The last travis run for this pull request was 73 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

[DrahtBot]

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]

Needs rebase

[kallewoof]

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.