-
Notifications
You must be signed in to change notification settings - Fork 37.8k
Remove CWalletTx::fFromMe member. #9351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9b723ae
to
6b9a284
Compare
I agree that this is dead code and should probably be removed. |
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).
6b9a284
to
39c77b0
Compare
@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 |
@ryanofsky This was made dead by changes to the trickle logic originally the use was for this one line
I dont see any reason not to remove it. |
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 |
I tend to agree there's no reason to make a quick decision on this one. No need to remove this for 0.14. |
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. |
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).