test: avoid non-determinism in asmap-addrman test#23084
Conversation
|
It seems the probability of collision when adding a second tried address with a different /16 may be 1/2^16 = 1/65536, which may be an acceptable level of failure in the absence of a deterministic addrman. The original versions of #22831 with a |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
The easiest way to prevent intermittent fails might be to only add 1 address to tried and then new (total 2), I don't think there need to be more addresses present for the regression test to work. |
|
Makes sense. IIRC the test didn't fail with only one tried entry, but I will retest that. Failing which, can remove the added assert here. A possible alternative or supplementary test would be a tiny hardcoded 2+2 peers.dat directly in the test file. |
jonatack
changed the title
|
@mzumsande you are correct, thank you for making me re-verify this. Updated this patch. It is now the same approach as the addpeeraddress test in test/functional/rpc_net.py in commit 869f136. |
jonatack
force-pushed
the
asmap-addrman-test-assert-on-result-instead-of-log
branch
from
94eaec1 to
78d27d1
Compare
|
Code Review ACK 78d27d12039aa204ed40009e36e62cb945568795 Just for the sake of a minimal regression test, we wouldn't even need the second address in new, but I think it's better to include it. |
|
Oh, just noticed I forgot to update the comment above the change. Done. |
This is the same approach as for the addpeeraddress test in `test/functional/rpc_net.py` in commit 869f136. The probability of collision when adding an addrman entry is expected to be 1/2^16 = 1/65536 for an address from a different /16. This change hopes to avoid these collisions by adding 1 tried entry before adding 1 new table one, instead of 2 tried entries followed by 2 new entries, which appears to have caused a collision in the CI. To verify the regression test stills fails when expected: - git checkout 181a120 && git cherry-pick ef242f5 - recompile bitcoind - git checkout this branch and run test/functional/feature_asmap.py. Expected output: ``` AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. != ``` Co-authored-by: Martin Zumsande <[email protected]>
jonatack
force-pushed
the
asmap-addrman-test-assert-on-result-instead-of-log
branch
from
78d27d1 to
5825b34
Compare
|
Does this PR fix the Line 96 in 16ccb3a |
|
Re-ACK 5825b34 |
jonatack
deleted the
asmap-addrman-test-assert-on-result-instead-of-log
branch
5825b34 test: avoid non-determinism in asmap-addrman test (Jon Atack) Pull request description: This is the same approach as for the addpeeraddress test in `test/functional/rpc_net.py` in commit 869f136. The probability of collision when adding an addrman entry is expected to be 1/2^16 = 1/65536 for an address from a different /16. This change hopes to avoid these collisions by adding 1 tried entry before adding 1 new table one, instead of 2 tried entries followed by 2 new entries, which appears to have caused a collision in the CI. To verify the regression test still fails when expected: - `git checkout 181a120 && git cherry-pick ef242f5` - recompile bitcoind - git checkout this branch and run `test/functional/feature_asmap.py`. Expected output: ``` AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. != ``` Closes bitcoin#23078. Co-authored-by: Martin Zumsande <[email protected]> ACKs for top commit: mzumsande: Re-ACK 5825b34 Tree-SHA512: eb6624a196fa5617c84aad860c8f91e8a8f60fc9fe2d7168facc298ee38db5e93b7e988e11c2ac1b399dc2c6b8fad360fb10bdbf10e598a1878300388475a200
…ve Windows 4befc8f ci: Enable feature_asmap.py test on native Windows (Hennadii Stepanov) Pull request description: This PR is a [follow up](bitcoin/bitcoin#23084 (comment)) of #23084. It enables the `feature_asmap.py` functional test which is fixed in #23084. ACKs for top commit: MarcoFalke: cr ACK 4befc8f Tree-SHA512: bdd813f0a360b7acc4be764bb402de91be519282c107ad1952f5dccd1171e39cca0e4ce11b6257de1135ddde8deb8465d1dccf29450a1cd16f6894ed86c67cb8
Summary: ``` The probability of collision when adding an addrman entry is expected to be 1/2^16 = 1/65536 for an address from a different /16. This change hopes to avoid these collisions by adding 1 tried entry before adding 1 new table one, instead of 2 tried entries followed by 2 new entries, which appears to have caused a collision in the CI. ``` Backport of [[bitcoin/bitcoin#23084 | core#23084]]. Test Plan: In a loop: ./test/functional/test_runner.py feature_asmap Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12363
6 participants
This is the same approach as for the addpeeraddress test in
test/functional/rpc_net.pyin commit 869f136.The probability of collision when adding an addrman entry is expected to be 1/2^16 = 1/65536 for an address from a different /16. This change hopes to avoid these collisions by adding 1 tried entry before adding 1 new table one, instead of 2 tried entries followed by 2 new entries, which appears to have caused a collision in the CI.
To verify the regression test still fails when expected:
git checkout 181a120 && git cherry-pick ef242f5test/functional/feature_asmap.py. Expected output:Closes #23078.
Co-authored-by: Martin Zumsande [email protected]