HomePhabricator

addrman: Fix AddrMan::Add() return semantics and logging

Description

addrman: Fix AddrMan::Add() return semantics and logging

Summary:
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.

Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.

[addrman] Add doxygen comment to AddrMan::Add()

Does not document the return value since we're going to fix the
semantics in a future commit.

https://github.com/bitcoin/bitcoin/pull/23380/commits/e58598e833d5737900fe3c4369e26f2a08166892

[addrman] Rename Add_() to AddSingle()

https://github.com/bitcoin/bitcoin/pull/23380/commits/2658eb6d68460272deefb3fcc653b03f6ec6e7cf

[addrman] Add Add_() inner function, fix Add() return semantics

Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.

Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.

p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).

https://github.com/bitcoin/bitcoin/pull/23380/commits/2095df7b7bfcb9ab0c5710a93112f7f341e753c9

[MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp

Keep the internal {Function}_() functions grouped together.

Review with git diff --color-moved=dimmed-zebra

https://github.com/bitcoin/bitcoin/pull/23380/commits/61ec0539b26a902a41a2602187a71f9dba3c6935

This is a backport of core#23380

Depends on D15086

Test Plan: ninja all check-all

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D15088

Details

Provenance
John Newbery <john@johnnewbery.com>Authored on Oct 28 2021, 11:46
PiRKCommitted on Jan 5 2024, 14:40
PiRKPushed on Jan 5 2024, 14:40
Reviewer
Restricted Project
Differential Revision
D15088: addrman: Fix AddrMan::Add() return semantics and logging
Parents
rABC1c535ea0faf7: wallet: Replace confusing getAdjustedTime() with GetTime()
Branches
Unknown
Tags
Unknown