Skip to content
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

wallet2: fix double counting outs if more than one pubkey #3985

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

moneromooo-monero
Copy link
Collaborator

No description provided.

@moneromooo-monero
Copy link
Collaborator Author

Wait. That's wrong, it just works by accudent on that test case. Will fix better.

@moneromooo-monero moneromooo-monero changed the title wallet2: fix double counting subaddress outs if more than one pubkey wallet2: fix double counting outs if more than one pubkey Jun 11, 2018
@@ -1147,13 +1155,16 @@ void wallet2::process_new_transaction(const crypto::hash &txid, const cryptonote
// additional tx pubkeys and derivations for multi-destination transfers involving one or more subaddresses
std::vector<crypto::public_key> additional_tx_pub_keys = get_additional_tx_pub_keys_from_extra(tx);
std::vector<crypto::key_derivation> additional_derivations;
for (size_t i = 0; i < additional_tx_pub_keys.size(); ++i)
if (pk_index == 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now how that strange amount was caused (i.e. the tx had two identical tx pubkeys which falsely doubled the received amount), but I wonder if this part is necessary/relevant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those keys only need to be generated/checked once, same idea, though these don't actually cause a problem technically so its mostly a speedup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but I doubt there would be any real speedup, because the subaddress PR was merged much later (October 2017) than when the cold signing bug was found and fixed (December 2016); it wouldn't have been possible for anyone to create a tx which mistakenly has two tx public keys due to the cold signing bug and then to simultaneously attach additional tx public keys which are specific to the subaddresses scheme.

Not that I'm strongly against it, just feels a little strange to me.

Copy link
Contributor

@fluffypony fluffypony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

Copy link
Contributor

@fluffypony fluffypony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

@fluffypony fluffypony merged commit 58cceaa into monero-project:master Jul 3, 2018
fluffypony added a commit that referenced this pull request Jul 3, 2018
58cceaa wallet2: fix double counting outs if the tx pubkey is duplicated (moneromooo-monero)
thaerkh added a commit to masari-project/masari that referenced this pull request Jul 5, 2018
d3987ef8 pushed a commit to oxen-io/oxen-core that referenced this pull request Jul 6, 2018
Transactions from the web wallet could have a duplicated public key,
which caused the Monero wallet to double-count ins and outs.

Fixed by applying upstream patch (Monero PR monero-project#3985).
GoPrivatePay pushed a commit to GoPrivatePay/PrivatePay that referenced this pull request Jul 6, 2018
ZuccBucc pushed a commit to zuccbucc-project/zuccbucc that referenced this pull request Jul 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants