-
Notifications
You must be signed in to change notification settings - Fork 0
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
src: fix #9335, prevent duplicate txis in fluff queue. #20
base: master
Are you sure you want to change the base?
Conversation
b2333dd
to
a899386
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
This would fix the issue but it includes another tx parse when adding txs to the fluff queue, which isn't ideal IMO.
Another way, because the txs are already sorted before being sent, is to just check sequential txs are not equal, if they are only add one of them to the message.
This does mean we still get duplicates in the queue but I don't think this isn't too big of a problem.
{ | ||
cryptonote::transaction transaction; | ||
if(cryptonote::parse_and_validate_tx_from_blob(tx, transaction)) | ||
{ | ||
if(fluff_txs_hash.find(transaction.hash) != fluff_txs_hash.end()) | ||
{ | ||
break; | ||
} | ||
fluff_txs_hash.emplace(transaction.hash); | ||
} | ||
fluff_txs.push_back(std::move(tx)); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed, right?
We already check for duplicates in the whole message at the top of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha. Saw the code here: https://github.com/0xFFFC0000/monero/blob/a899386881a79fecae1fbaf0fc463f33367599c6/src/cryptonote_protocol/cryptonote_protocol_handler.inl#L997
We don’t need the code here in cryptonote_protocol_handler.inl
.
Thanks for mentioning it.
There is a minor issue. I was thinking about doing it without parse tx. But there was a malleability bug with varints where you could encode the value 0 as "\00", "\8000", "\808000", etc. So there may be two different blobs which have the same TXID. |
efb82f0
to
79b7a45
Compare
Fix duplicate transaction monero-project#9335
79b7a45
to
86264f3
Compare
Please retry analysis of this Pull-Request directly on SonarCloud |
Seed nodes for new stressnet
No description provided.