What is the purpose of CAddrInfo::nRefCount ?

NotATether

legendary

Activity: 1568

Merit: 6660

bitcoincleanup.com / bitmixlist.org

Quote from: kesehap299 on June 01, 2021, 11:12:21 AM

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.

kesehap299

newbie

Activity: 4

Merit: 3

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;
            }
        }
        ....
}

kesehap299

newbie

Activity: 4

Merit: 3

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;
        ....
}

NotATether

legendary

Activity: 1568

Merit: 6660

bitcoincleanup.com / bitmixlist.org

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

newbie

Activity: 4

Merit: 3

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

Topic: What is the purpose of CAddrInfo::nRefCount ? (Read 80 times)