What is the purpose of CAddrInfo::nRefCount ?

[kesehap299]

According to the field comment, CAddrInfo::nRefCount represents "reference count in new sets (memory only)".
But since the commit e6b343d880 [1], each address deterministically hashes to a single fixed location in the "new" and "tried" tables.
So the "new" table will always have only one entry with the same address.

Before this commit, the same address could be inserted in multiple entries (at least, 8 entries as defined in ADDRMAN_NEW_BUCKETS_PER_ADDRESS), so it made sense to keep track how many reference to same addresses were in the "new" table.

If the purpose is to know if the address is in "new" or "tried" table, wouldn't a boolean field do the trick ?

[1] https://github.com/bitcoin/bitcoin/commit/e6b343d880f50d52390c5af8623afa15fcbc65a2

[1714681380]

1714681380

[1714681380]

1714681380

[1714681380]

1714681380

[NotATether]

In the old implementation, a boolean wouldn't be possible anyway because of multiple occurrences of the same address, and in the new implementation, it wouldn't really matter much as the nRefCount int is only going to be 0 or 1 which are bool values.

[kesehap299]

So, if this field is only going to be 0 or 1 in the current implementation, can the test below be removed from the code ?

ADDRMAN_NEW_BUCKETS_PER_ADDRESS will never be reached.
And the stochastic test makes it harder for the same address to fill multiple entries, something that is impossible today (since the entry is deterministic for each address).

Code:

        
bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
{
         ....
        // do not update if the max reference count is reached
        if (pinfo->nRefCount == ADDRMAN_NEW_BUCKETS_PER_ADDRESS)
            return false;

        // stochastic test: previous nRefCount == N: 2^N times harder to increase it
        int nFactor = 1;
        for (int n = 0; n < pinfo->nRefCount; n++)
            nFactor *= 2;
        if (nFactor > 1 && (insecure_rand.randrange(nFactor) != 0))
            return false;
        ....
}

[kesehap299]

Can infoExisting.nRefCount > 1 && pinfo->nRefCount == 0 (from code snippet below) also be removed ?

Since infoExisting.nRefCount > 1 will never be true, only infoExisting.IsTerrible() is really being evaluated in the code.

Code:

bool CAddrMan::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
{
         ....
        if (!fInsert) {
            CAddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]];
            if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) {
                // Overwrite the existing new table entry.
                fInsert = true;
            }
        }
        ....
}

[NotATether]

So, if this field is only going to be 0 or 1 in the current implementation, can the test below be removed from the code ?

ADDRMAN_NEW_BUCKETS_PER_ADDRESS will never be reached.
And the stochastic test makes it harder for the same address to fill multiple entries, something that is impossible today (since the entry is deterministic for each address).

Yes, since the loop will effectively be capped at n=0 so the stochastic test will never be executed.

As for the second post's snippet I haven't analyzed it yet.