test: expand LimitOrphan and EraseForPeer coverage

instagibbs commented at 3:28 pm on May 10, 2024: member

Inspired by refactorings in #30000 as the coverage appeared a bit sparse.

Added some minimal border value testing, timeouts, and tightened existing assertions.

DrahtBot commented at 3:28 pm on May 10, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, glozow, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28031 (Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling by glozow)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on May 10, 2024

in src/test/orphanage_tests.cpp:190 in bb2389a18e outdated

175     {
176-        size_t sizeBefore = orphanage.CountOrphans();
177         orphanage.EraseForPeer(i);
178-        BOOST_CHECK(orphanage.CountOrphans() < sizeBefore);
179+        BOOST_CHECK(orphanage.CountOrphans() == expected_num_orphans - 2);
180+        expected_num_orphans -= 2;

glozow commented at 12:39 pm on May 13, 2024:

nit: just update the value beforehand?

0        expected_num_orphans -= 2;
1        BOOST_CHECK(orphanage.CountOrphans() == expected_num_orphans);

instagibbs commented at 4:50 pm on May 13, 2024:

done

in src/test/orphanage_tests.cpp:201 in bb2389a18e outdated

203+    tx.vin[0].prevout.n = 0;
204+    tx.vin[0].prevout.hash = Txid::FromUint256(InsecureRand256());
205+    tx.vin[0].scriptSig << OP_1;
206+    tx.vout.resize(1);
207+    tx.vout[0].nValue = 1*CENT;
208+    tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey()));

glozow commented at 2:38 pm on May 13, 2024:

Is there a reason to do this manually instead of using MakeTransactionSpending()?

instagibbs commented at 4:50 pm on May 13, 2024:

nope, changed

in src/test/orphanage_tests.cpp:224 in bb2389a18e outdated

213+
214+    // Jump ORPHAN_TX_EXPIRE_TIME minutes, orphan should be timed out on limiting
215+    SetMockTime(GetTime() + 20 * 60);
216+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 1);
217+    orphanage.LimitOrphans(1, rng);
218+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 0);

glozow commented at 2:39 pm on May 13, 2024:

Maybe out of scope but I wonder if it would be best to add a current_time parameter to the TxOrphanage methods to make it easier to test and fuzz the expiration (also should have a test for the sorting for GetChildrenFromSamePeer results)

instagibbs commented at 4:51 pm on May 13, 2024:

probably a good idea, though you might have better opinions on orphanage interface. I tightened the time testing a bit more by setting mocktime at beginning, then checking boundary conditions via subsequent setting of the time.

instagibbs force-pushed on May 13, 2024

in src/test/orphanage_tests.cpp:208 in dd7bf60713 outdated

210+    // One second shy of expiration
211+    SetMockTime(now - 1 + 20 * 60);
212+    orphanage.LimitOrphans(1, rng);
213+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 1);
214+
215+    // Jump ORPHAN_TX_EXPIRE_TIME minutes, orphan should be timed out on limiting

glozow commented at 4:29 pm on May 15, 2024:

Note you’ve only jumped 1 second here

instagibbs commented at 2:03 pm on June 7, 2024:

done

in src/test/orphanage_tests.cpp:204 in dd7bf60713 outdated

206+    orphanage.AddTx(timeout_tx, 0);
207+    orphanage.LimitOrphans(1, rng);
208+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 1);
209+
210+    // One second shy of expiration
211+    SetMockTime(now - 1 + 20 * 60);

glozow commented at 4:31 pm on May 15, 2024:

Would be nice if the constants were moved to txorphanage.h so we don’t need the magic number here. Could be useful in other tests as well.

instagibbs commented at 2:03 pm on June 7, 2024:

done in first commit now

glozow commented at 9:11 am on May 17, 2024: member

lgtm

instagibbs force-pushed on Jun 7, 2024

refactor: move orphanage constants to header file 28dbe218fe

instagibbs force-pushed on Jun 7, 2024

instagibbs commented at 2:12 pm on June 7, 2024: member

rebased due to silent merge conflict, with cleanups suggested by @glozow :+1:

in src/test/orphanage_tests.cpp:185 in 6831f3972e outdated

181+
182+    // Non-existent peer; nothing should be deleted
183+    orphanage.EraseForPeer(/*peer=*/-1);
184+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), expected_num_orphans);
185+
186+    // Test EraseOrphansFor: Each of first three peers announced

glozow commented at 2:52 pm on June 7, 2024:

EraseOrphansFor doesn’t exist

instagibbs commented at 3:06 pm on June 7, 2024:

:sweat: fixed

glozow commented at 3:03 pm on June 7, 2024: member

almost ack 6831f3972ee5d660ce0bff9bb573745338417544, thanks for taking suggestions

instagibbs force-pushed on Jun 7, 2024

glozow commented at 8:46 am on June 10, 2024: member

ACK eaf4de028de8fa13227b6089785889f1c6c15b4d

in src/test/orphanage_tests.cpp:186 in eaf4de028d outdated

182+    // Non-existent peer; nothing should be deleted
183+    orphanage.EraseForPeer(/*peer=*/-1);
184+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), expected_num_orphans);
185+
186+    // Each of first three peers announced
187+    // two transactions each.

rkrux commented at 11:02 am on June 17, 2024:

Took me a while to understand why only the first 3 peers announced 2 txs each, but it’s infact the first 10 peers as per the above 3 loops that announced 2 txs each - the third tx for each of them being ignored because of its large size.

instagibbs commented at 1:57 pm on June 17, 2024:

I’m switching the word to “stored” to be clearer

in src/test/orphanage_tests.cpp:203 in eaf4de028d outdated

201+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), expected_num_orphans);
202+    orphanage.LimitOrphans(/*max_orphans=*/expected_num_orphans - 1, rng);
203+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), expected_num_orphans - 1);
204     orphanage.LimitOrphans(40, rng);
205-    BOOST_CHECK(orphanage.CountOrphans() <= 40);
206+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 40);

rkrux commented at 11:06 am on June 17, 2024:

Nit: Tying it to expected_num_orphans like expected_num_orphans -3 in the LimitOrphans call would make it more explicit (but not necessarily in the BOOST_CHECK), otherwise it made me wonder whether the current count is more than 40 or not.

instagibbs commented at 1:57 pm on June 17, 2024:

good point, I added an assert to make it clear its larger, since I think that’s the assertion we really want

in src/test/orphanage_tests.cpp:209 in eaf4de028d outdated

209+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 10);
210     orphanage.LimitOrphans(0, rng);
211-    BOOST_CHECK(orphanage.CountOrphans() == 0);
212+    BOOST_CHECK_EQUAL(orphanage.CountOrphans(), 0);
213+
214+    // Add one more orphan, check timeout logic

rkrux commented at 12:01 pm on June 17, 2024:

Can you please share how exactly is this being done check timeout logic here?

I see from the AddTx code that if the tx size is more than the standard weight, then it will not be added in the orphanage. Is the timeout logic here that the size of this tx is less than the standard weight and therefore it’s inserted?

glozow commented at 12:11 pm on June 17, 2024:

orphans are expired in the first part of LimitOrphans

rkrux commented at 1:31 pm on June 17, 2024:

Oh timeout as in expiration of the orphans. I assumed it referred to some kind of timeout of the insertion operation. Makes sense to me now, thanks @glozow.

instagibbs commented at 1:57 pm on June 17, 2024:

considering this resolved

in src/txorphanage.h:21 in eaf4de028d outdated

13@@ -14,6 +14,11 @@
14 #include <map>
15 #include <set>
16 
17+/** Expiration time for orphan transactions */
18+static constexpr auto ORPHAN_TX_EXPIRE_TIME{20min};
19+/** Minimum time between orphan transactions expire time checks */
20+static constexpr auto ORPHAN_TX_EXPIRE_INTERVAL{5min};
21+

rkrux commented at 12:02 pm on June 17, 2024:

+1 on moving these to the header file!

rkrux approved

rkrux commented at 12:03 pm on June 17, 2024: none

tACK eaf4de0

Make successful, so are all the unit tests and functional test. Asked couple questions for my understanding.

test: expand LimitOrphan and EraseForPeer coverage 172c1ad026

instagibbs force-pushed on Jun 17, 2024

rkrux approved

rkrux commented at 6:26 pm on June 17, 2024: none

reACK 172c1ad

Thanks for very quickly addressing my comments @instagibbs, appreciate it!

DrahtBot requested review from glozow on Jun 17, 2024

glozow commented at 4:36 pm on July 18, 2024: member

reACK 172c1ad026cc38c6f52679e74c14579ecc77c48e

instagibbs commented at 6:05 pm on July 25, 2024: member

@achow101 RFM?

achow101 commented at 9:19 pm on August 5, 2024: member

ACK 172c1ad026cc38c6f52679e74c14579ecc77c48e

achow101 merged this on Aug 5, 2024

achow101 closed this on Aug 5, 2024

test: expand LimitOrphan and EraseForPeer coverage #30082

Code Coverage

Reviews

Conflicts