False positive collisions could occur because of GetBucketPosition #22554

<!-- This issue tracker is only for technical issues related to Bitcoin Core. General bitcoin questions and/or support requests are best directed to the Bitcoin StackExchange at https://bitcoin.stackexchange.com. For reporting security issues, please read instructions at https://bitcoincore.org/en/contact/. If the node is "stuck" during sync or giving "block checksum mismatch" errors, please ensure your hardware is stable by running memtest and observe CPU temperature with a load-test tool such as linpack before creating an issue! -->

<!-- Describe the issue -->

In CAddrMan::Add_ method if pinfo->GetNewBucket(nKey, source, m_asmap) returns same value(within an instance), pinfo->GetBucketPosition(nKey, true, nUBucket) would also return same consecutive values. This could lead to collision when it is not real one.

Expected behavior

<!--- What behavior did you expect? -->

Check should be made to confirm if pinfo is empty while checking fInsert.

Actual behavior

<!--- What was the actual behavior (provide screenshots if the issue is GUI-related)? -->

Some addrman_new_collisions tests failed To reproduce

<!--- How reliably can you reproduce the issue, what are the steps to do so? -->

System information Ubuntu 20

<!-- What version of Bitcoin Core are you using, where did you get it (website, self-compiled, etc)? -->

Master

<!-- What type of machine are you observing the error on (OS/CPU and disk type)? -->

500GB, Linux

<!-- GUI-related issue? What is your operating system and its version? If Linux, what is your desktop environment and graphical shell? -->

<!-- Any extra information that might be useful in the debugging process. -->

<!--- This is normally the contents of a `debug.log` or `config.log` file. Raw text or a link to a pastebin type site are preferred. -->

Some addrman_new_collisions tests failed

Do you have steps to reproduce?

If the same new bucket/position is selected, then we don't try to insert the entry again:

https://github.com/bitcoin/bitcoin/blob/1488f55fa57a1400a57be837b574183f019c7855/src/addrman.cpp#L365-L367

(if (vvNew[nUBucket][nUBucketPos] != nId) ensures that the entry isn't already in that position in the bucket)

Some addrman_new_collisions tests failed

Do you have steps to reproduce?

I trying to see if I can achieve that. Which function is toString for uint256?

@janus are you able to reproduce this with the current version of the software? If not perhaps this can be closed.

Reading the codeblock as it stands today, it still appears that the entry will only be added if it does not already exist there:

https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/addrman.cpp#L605-L627

Check should be made to confirm if pinfo is empty while checking fInsert.

We will always have pinfo at the check, as it's either found and returned by Find(), or else is created with Create(), so I don't think I understand what this check would do.

https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/addrman.cpp#L563

https://github.com/bitcoin/bitcoin/blob/f7bdcfc83f5753349018be3b5a663c8923d1a5eb/src/addrman.cpp#L600-L603

Closing for now, but happy to reopen if there is more info provided.