test: expand LimitOrphan and EraseForPeer coverage #30082

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?

        expected_num_orphans -= 2;
        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: contributor

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: contributor

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

bitcoin locked this on Aug 5, 2025