tests: Improve p2p tx propagation functional test #9762
Conversation
tests/functional_tests/p2p.py
Outdated
| for daemon in [daemon2, daemon3]: | ||
| # Due to Dandelion++, the network propagates transactions with a | ||
| # random delay, so poll for the transaction with a timeout | ||
| timeout = 16 |
There was a problem hiding this comment.
@vtnerd Do you know the expected maximum propagation delay in this scenario, based on the D++ paper? That value would act as an approximate lower bound for the timeout, which could then be further padded to reflect physical realities of data transmission and processing.
There was a problem hiding this comment.
The delay should usually trigger the inbound fluff delay where it's using a poisson distribution, simply mimicking what Bitcoin was doing for the same situation. 95% of the values are in the 3-7.3 second range. A delay of 16 is basically nearly impossibl, so this should be acceptable.
As an explanation of why not exponential here - one of the nodes will have no outbound peers. I don't recall the paper stating how to handle the situation, so I decided to make it immediately fluff, as it would be an edge case. A fluff does randomized poisson delays, similar to what Bitcoin was doing at the time. I don't recall the d++ specifying how to handle fluff precisely either, maybe I need to revisit that paper.
There was a problem hiding this comment.
There was a problem hiding this comment.
Unless I've done the math horribly wrong, it looks like 13s should yield a failure probability of roughly 1 in a billion (I decided a million was too low); I've updated the test timeout accordingly.
|
I guess you were convinced the failure was primarily the sleep timeout? That was my assessment, as it seemed like an obvious issue. |
iamamyth
force-pushed
the
tests-p2p-tx-propagation
branch
from
eb9cb97 to
6d147d1
Compare
|
I think the low, fixed timeout generates quite a few false positive failures, consistent with the observed behavior of this test in CI. If any actual transaction propagation errors exist, I would expect they do not owe to recent connection management commits. I just modified the test to better differentiate propagation to 0-2 daemons, which might make it a bit more useful. |
iamamyth
force-pushed
the
tests-p2p-tx-propagation
branch
2 times, most recently
from
216523d to
f918d48
Compare
|
Test failure is a known-flaky unit test ( |
|
@vtnerd I made one minor change to show how many daemons see the transactions, but I think this will clear up the p2p test issue. |
|
An example failure in a recent CI build (without this change): https://github.com/monero-project/monero/actions/runs/13163403906/job/36738631593?pr=9771. Every failure I've seen on CI (and I've seen quite a few, at this point) is the same behavior, it's not the RPC refusing the connection, or generating a garbage response, it's simply the transaction not appearing in the pool by design; the failures are successes and the test has a wrong methodology. |
I'm not against this patch but what I don't understand is why this test only recently started to fail randomly, if it's just timeout probability. To me it seems there's more to it. |
I don't know when the failures started (CI has so many broken tests, even now, that I imagine many people just ignore it). However, as stated, due to the test's structural flaw, if, in fact, it used to pass and now fails, that may reflect an improvement in Dandelion++ or networking code, because it should have had a moderate probability of failure in the past. Of course, it could just as easily look better now because of some new bug producing interference; a proper investigation of that possibility would require establishing a timeline to narrow the search. None of the aforementioned possibilities alter the correctness of this patch, which will make the test actually useful in the future, rather than noisy, broken, and unable to surface relevant contextual information, such as how many daemons received the transaction. |
Reduce the likelihood of false positive failures in the p2p transaction propagation functional test by waiting up to a maximum timeout for a transaction to propagate, rather than using a fixed timeout, to reflect the random delay of Dandelion++ transaction propagation. This strategy also speeds test execution in cases where propagation occurs faster than the previously expected fixed delay.
iamamyth
force-pushed
the
tests-p2p-tx-propagation
branch
from
f918d48 to
950ddbf
Compare
|
@iamamyth please also open against |
Reduce the likelihood of false positive failures in the p2p transaction propagation functional test by waiting up to a maximum timeout for a transaction to propagate, rather than using a fixed timeout, to reflect the random delay of Dandelion++ transaction propagation. This strategy also speeds test execution in cases where propagation occurs faster than the previously expected fixed delay.