Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Stop tracking sent transactions once they've been imported into the blockchain #5706

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

halfalicious
Copy link
Contributor

Fix #5686

Remove transactions from EthereumCapability's list of sent transactions (which it uses to
prevent sending the same transaction to peers more than once) once
they've made it into the blockchain.
@halfalicious halfalicious requested review from gumb0 and chfast and removed request for gumb0 July 30, 2019 04:23
@halfalicious halfalicious changed the title [WIP] Stop tracking sent transactions once they've been imported into the blockchain Stop tracking sent transactions once they've been imported into the blockchain Jul 30, 2019
@halfalicious halfalicious requested a review from gumb0 July 30, 2019 04:23
@codecov-io
Copy link

Codecov Report

Merging #5706 into master will increase coverage by 0.01%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5706      +/-   ##
==========================================
+ Coverage   62.98%   62.99%   +0.01%     
==========================================
  Files         351      350       -1     
  Lines       30014    30001      -13     
  Branches     3367     3363       -4     
==========================================
- Hits        18903    18900       -3     
+ Misses       9890     9882       -8     
+ Partials     1221     1219       -2

Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Looks fine, just two minor suggestions

}
auto h = m_host.lock();
if (h && goodTransactions.size())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (h && goodTransactions.size())
if (h && !goodTransactions.empty())

Copy link
Member

Choose a reason for hiding this comment

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

I would move the check for empty vector inside removeSentTransactions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, that way we get the benefit of the check for all callers.

Move empty check into EthereumCapability::removeSentTransactions (so
that it's performed regardless of the caller) and use empty() to check
for empty condition rather than !size().
@halfalicious halfalicious merged commit e993828 into master Jul 30, 2019
@halfalicious halfalicious deleted the update-sent-transactions branch July 31, 2019 03:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aleth never clears "sent transactions" list
3 participants