test: avoid non-determinism in asmap-addrman test

jonatack commented at 10:43 am on September 24, 2021: member

This is the same approach as for the addpeeraddress test in test/functional/rpc_net.py in commit 869f1368.

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:

0AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. !=

Closes #23078.

Co-authored-by: Martin Zumsande mzumsande@gmail.com

fanquake added the label Tests on Sep 24, 2021

jonatack commented at 11:14 am on September 24, 2021: member

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 peers.dat file are also free of non-determinism.

DrahtBot commented at 11:52 am on September 24, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22872 (log: improve checkaddrman logging with duration in milliseconds by jonatack)

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.

mzumsande commented at 12:24 pm on September 24, 2021: member

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. assert_equal(len(addrs), 4) could also fail, I think in an extremely unlikely edge case there may even only be 2 addrs present (A1 -> new, A1 new -> tried, A2 ->new , A2 -> tried collision, so it stays in new, A3 ->new collision with A2, A4 ->new collision with A2)

jonatack commented at 1:18 pm on September 24, 2021: member

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 renamed this:
~~test: assert on result instead of log in asmap-addrman test~~
test: avoid non-determinism in asmap-addrman test
on Sep 24, 2021

jonatack commented at 4:53 pm on September 24, 2021: member

@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 on Sep 24, 2021

mzumsande commented at 7:38 pm on September 24, 2021: member

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.

jonatack commented at 7:57 pm on September 24, 2021: member

Oh, just noticed I forgot to update the comment above the change. Done.

test: avoid non-determinism in asmap-addrman test

This is the same approach as for the addpeeraddress test in
`test/functional/rpc_net.py` in commit 869f1368.

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 <mzumsande@gmail.com>

5825b34783

jonatack force-pushed on Sep 24, 2021

hebasto commented at 8:03 am on September 25, 2021: member

CI error fixed in #23089.

Does this PR fix the feature_asmap.py functional test on Windows? See: https://github.com/bitcoin/bitcoin/blob/16ccb3a1cd9125eb24a5b45a98099ff98660767a/.cirrus.yml#L96

mzumsande commented at 10:18 pm on September 25, 2021: member

Re-ACK 5825b34783545f9470d5ab94b87c918980715675

MarcoFalke merged this on Sep 27, 2021

MarcoFalke closed this on Sep 27, 2021

jonatack deleted the branch on Sep 27, 2021

sidhujag referenced this in commit 70c030a13a on Sep 27, 2021

MarcoFalke referenced this in commit f036c35e51 on Sep 27, 2021

jonatack commented at 10:55 am on September 28, 2021: member

CI error fixed in #23089.

Does this PR fix the feature_asmap.py functional test on Windows? See:

https://github.com/bitcoin/bitcoin/blob/16ccb3a1cd9125eb24a5b45a98099ff98660767a/.cirrus.yml#L96

Thanks @hebasto. I didn’t realize they were disabled on Windows.

Fabcien referenced this in commit 181d570e4a on Oct 24, 2022

DrahtBot locked this on Oct 30, 2022

test: avoid non-determinism in asmap-addrman test #23084

Conflicts