Only allow up to 1024 dropped transactions in the transaction queue by halfalicious · Pull Request #5687 · ethereum/aleth

halfalicious · 2019-07-22T03:29:54Z

halfalicious · 2019-07-22T04:26:47Z

clang6 failures aren't due to my changes (fairly certain I've seen these failures before), will file a new issue:

Test Case "saveNodes": 
/home/builder/project/test/unittests/libp2p/peer.cpp(188): error: in "libp2p/p2p/saveNodes": check host.peerCount() == c_peers has failed [0 != 5]
/home/builder/project/test/unittests/libp2p/peer.cpp(189): error: in "libp2p/p2p/saveNodes": check host2.peerCount() == c_peers has failed [4 != 5]
/home/builder/project/test/unittests/libp2p/peer.cpp(215): fatal error: in "libp2p/p2p/saveNodes": critical check r[2].itemCount() >= c_nodes has failed

*** 3 failures are detected (5 failures are expected) in the test module "Master Test Suite"

[Edit] Filed #5689

codecov-io · 2019-07-22T04:32:49Z

Codecov Report

Merging #5687 into master will increase coverage by 0.13%.
The diff coverage is 92.22%.

@@            Coverage Diff             @@
##           master    #5687      +/-   ##
==========================================
+ Coverage   62.98%   63.11%   +0.13%     
==========================================
  Files         351      353       +2     
  Lines       30026    30111      +85     
  Branches     3370     3378       +8     
==========================================
+ Hits        18912    19005      +93     
+ Misses       9893     9877      -16     
- Partials     1221     1229       +8

gumb0 · 2019-07-22T15:30:18Z

libethereum/TransactionQueue.cpp

+                            << c_maxDroppedTransactionCount
+                            << "), removing dropped transaction from list (txhash: "
+                            << *m_dropped.begin() << ")";
+        m_dropped.erase(m_dropped.begin());


This way it removes random transaction. As @chfast pointed out in the issue, it would be nicer to have some kind of LRU cache here, what do you think?

This way it removes random transaction. As @chfast pointed out in the issue, it would be nicer to have some kind of LRU cache here, what do you think?

@gumb0 Agreed, didn't go with that in the first place since it increases the storage requirements and wasn't sure it was necessary but I'll go ahead and add that in.

I don't care so much any more. I think it is ok to merge it as it is and fill additional issue.

I'm fine if we merge this as is, too

halfalicious · 2019-07-30T05:10:05Z

cc @chfast / @gumb0 : Ready for review

gumb0 · 2019-07-30T14:09:59Z

libethereum/TransactionQueue.cpp

        return ImportResult::AlreadyKnown;

-    if (m_dropped.count(_h) && _ik == IfDropped::Ignore)
+    if (m_dropped.contains(_h) && _ik == IfDropped::Ignore)


I think tx should also be touched here, if it's found.

gumb0 · 2019-07-30T14:16:32Z

libdevcore/LruCache.h

+template <class Key, class Value>
+class LruCache
+{
+    typedef Key key_type;


Better to use using key_type = Key; instead of typedef

Agreed, much more intuitive / readable

gumb0 · 2019-07-30T17:07:14Z

libdevcore/LruCache.h

+public:
+    explicit LruCache(size_t _capacity) : m_capacity(_capacity) {}
+
+    LruCache(LruCache<key_type, value_type> const& _l)


I think default-generated copy and move constructors and assignment operators should be enough.

halfalicious · 2019-07-31T03:41:07Z

Rebased to address CHANGELOG merge conflict

Boost::compute LRU cache was used as inspiration

Use LruCache to manage dropped transactions

Use using instead of typedef for LruCache custom types Touch dropped transaction hash in LRU cache when checking for dropped transaction during transaction queue import. Remove copy/move ctors/assignment operators from LruCache since the default ones provide the same functionality.

halfalicious · 2019-07-31T03:43:36Z

Squashed some commits

gumb0

Looks good to me, one minor thing: you could make empty(), size(), capacity(), clear() and iterator getters noexcept.

gumb0 · 2019-07-31T14:39:53Z

test/unittests/libdevcore/LruCache.cpp

+    auto iter = _lruCache.begin();
+    while (iter != _lruCache.cend() && i < _data.size())
+    {
+        EXPECT_EQ(iter->first, _data[i].first);


Probably EXPECT_EQ(*iter, _data[i]) should work

halfalicious · 2019-08-02T02:56:33Z

Looks good to me, one minor thing: you could make empty(), size(), capacity(), clear() and iterator getters noexcept.

@gumb0: Thanks for calling this out

halfalicious added the in progress label Jul 22, 2019

halfalicious force-pushed the tq-maxdropped branch from 0df25d7 to 4612963 Compare July 22, 2019 04:19

halfalicious removed the in progress label Jul 22, 2019

halfalicious requested review from chfast and gumb0 July 22, 2019 04:29

gumb0 reviewed Jul 22, 2019

View reviewed changes

halfalicious added the in progress label Jul 22, 2019

halfalicious changed the title ~~Only allow up to 1024 dropped transactions in the transaction queue~~ [WIP] Only allow up to 1024 dropped transactions in the transaction queue Jul 22, 2019

halfalicious force-pushed the tq-maxdropped branch 4 times, most recently from 2813173 to c7e5f2f Compare July 30, 2019 04:29

halfalicious removed the in progress label Jul 30, 2019

halfalicious changed the title ~~[WIP] Only allow up to 1024 dropped transactions in the transaction queue~~ Only allow up to 1024 dropped transactions in the transaction queue Jul 30, 2019

gumb0 reviewed Jul 30, 2019

View reviewed changes

halfalicious added 3 commits July 30, 2019 20:40

Enforce max size for dropped transaction list

3f62786

Update license message

9813492

Update changelog

8eea628

halfalicious force-pushed the tq-maxdropped branch from 03fe17e to b4885f0 Compare July 31, 2019 03:40

halfalicious added 3 commits July 30, 2019 20:43

LRU cache implementation and tests

d2a2b9a

Boost::compute LRU cache was used as inspiration

Integrate LruCache into transaction queue

c0c02f1

Use LruCache to manage dropped transactions

halfalicious force-pushed the tq-maxdropped branch from b4885f0 to 1fdb02a Compare July 31, 2019 03:43

gumb0 approved these changes Jul 31, 2019

View reviewed changes

Make LruCache methods noexcept and simplify test

2542256

halfalicious merged commit d57e0d2 into master Aug 2, 2019

gumb0 deleted the tq-maxdropped branch August 7, 2019 16:38

Conversation

halfalicious commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halfalicious commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious commented Jul 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious commented Jul 31, 2019

Uh oh!

halfalicious commented Jul 31, 2019

Uh oh!

gumb0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halfalicious commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

halfalicious commented Jul 22, 2019 •

edited

Loading

halfalicious commented Jul 22, 2019 •

edited

Loading

codecov-io commented Jul 22, 2019 •

edited

Loading

halfalicious Jul 22, 2019 •

edited

Loading