Skip to content

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Dec 14, 2016

Prior to this commit, the fFromMe member was written to and serialized but not
actually used in any meaningful way.

The one place where the fFromMe was used in a nontrivial way was
CWallet::AddToWallet, but the only actual effect it had there was to trigger
additional CWalletDB::WriteTx calls that would change serialized fFromMe fields
in previously existing database rows from false to true (without changing other
fields).

@jonasschnelli
Copy link
Contributor

I agree that this is dead code and should probably be removed.
The only reason I could think of why to keep this is to have a flag to later determine which transactions in the wallet where created by the own wallet over CreateTransaction().
Although not sure how useful this would be.

@laanwj
Copy link
Member

laanwj commented Dec 15, 2016

Yes, is there any value in keeping/generating this information? I guess we do a more thorough job by just looking at the inputs/outputs of a transaction. Thisl also catches transactions generated and broadcasted through the raw transactions API.

Prior to this commit, the fFromMe member was written to and serialized but not
actually used in any meaningful way.

The one place where the fFromMe was used in a nontrivial way was
CWallet::AddToWallet, but the only actual effect it had there was to trigger
additional CWalletDB::WriteTx calls that would change serialized fFromMe fields
in previously existing database rows from false to true (without changing other
fields).
@ryanofsky ryanofsky changed the title Remove CWalletTx::fFromMe member. Add documentation for CWalletTx::fFromMe member. Dec 15, 2016
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Dec 15, 2016

@jonasschnelli's comment helped me understand the purpose of the fFromMe field, and now I kind of think it's worth keeping, especially since getting rid of it doesn't simplify things much.

I changed my commit for now to just document the fFromMe field instead of getting rid of it.

@maflcko maflcko added the Docs label Dec 15, 2016
@pstratem
Copy link
Contributor

@ryanofsky This was made dead by changes to the trickle logic

originally the use was for this one line

                       if (wtx.fFromMe)
                           fTrickleWait = true;

I dont see any reason not to remove it.

@ryanofsky ryanofsky changed the title Add documentation for CWalletTx::fFromMe member. Remove CWalletTx::fFromMe member. Dec 16, 2016
@ryanofsky
Copy link
Contributor Author

Ok @pstratem, removed fFromMe again. I don't have any opinion on whether it should be kept or removed. I just think it should be documented as in commit 39c77b0 if it is not removed.

Reasons I could imagine why someone might not want to remove it is that it provides a nice little clue about the origin of a transaction that could be useful for debugging, or to show as information in the UI. Also, removing the variable doesn't simplify the code very much. Another reason to keep it might be that having it could make it easier to remove the fTimeReceivedIsTxTime variable which I think is actually a stranger thing to serialize. It has similar meaning to fFromMe, but unlike fFromMe is not merged properly in AddToWallet.

@laanwj
Copy link
Member

laanwj commented Dec 19, 2016

I tend to agree there's no reason to make a quick decision on this one. No need to remove this for 0.14.
Adding the brainstorming label.

@ryanofsky
Copy link
Contributor Author

I think I will close this PR, but feel free to reopen if it you want to keep it around. I created a separate PR #9378 with just the documentation commit.

@ryanofsky ryanofsky closed this Dec 19, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants