The high-level description of addrman has outdated information with respect to the eviction behavior, both for the New and Tried tables (at least since #5941) - this has confused me in the past.
This PR corrects this and also adds basic info about the bucket size and position.
114 | * * One single address can occur in up to 8 different buckets to increase selection chances for addresses that 115 | * are seen frequently. The chance for increasing this multiplicity decreases exponentially. 116 | - * * When adding a new address to a full bucket, a randomly chosen entry (with a bias favoring less recently seen 117 | - * ones) is removed from it first. 118 | + * * When adding a new address to an occupied position of a bucket, it will not replace the existing entry 119 | + * unless that address is also stored in another bucket or it doesn't meet one of several quality criteria.
Perhaps explicitly reference IsTerrible here:
* unless that address is also stored in another bucket or it doesn't meet one of several quality criteria (see IsTerrible for exact criteria).
Thanks - done!
ACK. The current documentation is incorrect, and these new comments are clear. Thanks for documenting this!
103 | @@ -104,19 +104,22 @@ class CAddrInfo : public CAddress 104 | * * Make sure no (localized) attacker can fill the entire table with his nodes/addresses. 105 | * 106 | * To that end: 107 | - * * Addresses are organized into buckets. 108 | - * * Addresses that have not yet been tried go into 1024 "new" buckets. 109 | + * * Addresses are organized into buckets that can each store up to 64 entries. 110 | + * * Addresses to which our node has not successfully connected go into 1024 "new" buckets. 111 | * * Based on the address range (/16 for IPv4) of the source of information, 64 buckets are selected at random.
Perhaps mention AS bucketing.
* * Based on the address range (/16 for IPv4) of the source of information, or if an asmap is provided, the AS it belongs to (for IPv4/IPv6), 64 buckets are selected at random.
done as suggested, thanks!
ACK; good idea, thanks for updating the documentation.
121 | * * Addresses of nodes that are known to be accessible go into 256 "tried" buckets. 122 | * * Each address range selects at random 8 of these buckets. 123 | * * The actual bucket is chosen from one of these, based on the full address. 124 | - * * When adding a new good address to a full bucket, a randomly chosen entry (with a bias favoring less recently 125 | - * tried ones) is evicted from it, back to the "new" buckets. 126 | + * * When adding a new good address to an occupied position of a bucket, a short-lived feeler connection to the
* * When adding a new good address to an occupied position of a bucket, a feeler connection to the
short-lived and feeler feels kinda redundant? one possibility would be to say FEELER or ConnectionType::Feeler so it points to the lengthy docs in net.h, but eh doesn't matter too much regardless. you've fixed the content.
changed to FEELER
ACK 004324c649 thanks for correcting this!
so, funnily enough, I recently also noticed some incorrect addrman documentation- namely the description for Create(). If you're interested, feel free to cherry-pick that / any of the other addrman doc improvements that I have over here- https://github.com/amitiuttarwar/bitcoin/commit/3b507036f55817d46e25b3b8aa348b756f407f47
ACK 004324c649deb4020f26b1949075409a71975da6
(happy to reACK if you take @jonatack or @amitiuttarwar suggestions)
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Thanks for the reviews! I took all the suggestions on the high-level description.
@amitiuttarwar Good find with Create! For the scope of this PR, I took the short version of your correction, keeping the existing terminology ("entry") to stay in line with the rest of the documentation. I think a more general doc overhaul of the various other functions in addrman would better be a separate PR.
looks good, thanks for fixing these :)
reACK 036d7eadf5dd0e06e0734a5d78dbe28f4bfaa07f
695 | @@ -692,8 +696,7 @@ class CAddrMan 696 | //! Find an entry. 697 | CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); 698 | 699 | - //! find an entry, creating it if necessary. 700 | - //! nTime and nServices of the found node are updated, if necessary. 701 | + //! Create a new entry and add it to the internal data structures mapInfo, mapAddr and vRandom.
Nit: naming the internal data structures (mapInfo, mapAddr and vRandom) is probably too much implementation-specific detail for the header comments:
//! Create a new entry and add it to the internal data structures.
I had the same thought but then changed my mind it in order to prevent the misunderstanding that all relevant internal structures (notably vvNew) are populated. This is something the caller has to do.
ACK 036d7eadf5dd0e06e0734a5d78dbe28f4bfaa07f
One minor comment inline. Feel free to ignore.
