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

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.

@mzumsande - I think this fixes the logging bug that you encountered.

Concept ACK I encountered this in the form of the RPC addpeeraddress returning true although nothing was added - which is fixed by this PR as well.

ACK 61ec0539b26a902a41a2602187a71f9dba3c6935

This PR primarily deals with the bug, where Addrman::Add() returns true, even when the AddrInfo object has not been successfully added to the table. This PR also focuses on other minor changes.

(A short description of this PR, in chronological order of the commits. This might help fellow reviewers to understand this PR better.)

Description:

• Expanded the documentation for the Addrman::Add() function making it more clear.
• In AddrManImpl class Add_() has been renamed to AddSingle() and a new definition of Add_() is introduced which can tackle addition of multiple instances AddressInfo object.
• AddSingle() function has been appropriately modified to return bool 0 if the AdrrInfo object has not been successfully entered into the table.
• AddrmanImpl::Add() has been appropriately modified to inculcate the changes. One doubt though: was there any specific reason for using auto ret = instead of bool ret = ?
• Finally, reordering of AddrManImpl::Good_() has been done to group internal functions together.

This PR is able to correct the described bug successfully, and I agree with the changes made in this PR.

A Sidenote:

(Note: This is not specifically part of this PR. And can be taken as a brainstorming activity)

In the AddrmanImpl class, what do you think about making the internal {Function}_() functions protected or private, as a non-class member need not call this function directly, and they should call their respective {Function}()?

<p>

Thank you for the very thorough review @shaavan!

(Note: This is not specifically part of this PR. And can be taken as a brainstorming activity)

In the AddrmanImpl class, what do you think about making the internal {Function}_() functions protected or private, as a non-class member need not call this function directly, and they should call their respective {Function}()?

You're exactly correct that they don't need to be accessed outside the class. In fact, they already are private. There's a private specifier a few lines above:

https://github.com/bitcoin/bitcoin/blob/ab25ef8c7f767258d5fe44f53b35ad8bd51ed5cd/src/addrman_impl.h#L144

There's a private specifier a few lines above:

Oops, I missed that! 😅. Thanks for informing me!

AddrmanImpl::Add() has been appropriately modified to inculcate the changes. One doubt though: was there any specific reason for using auto ret = instead of bool ret = ? @shaavan - The idea was that all of the public functions could have a standard structure of:

ReturnType AddrManImpl::Function(args...)
{
    LOCK(cs);
    Check();
    const auto ret = Function_(args...);
    Check();
    return ret;
}

which could then be replaced by a variadic template to reduce the boilerplate/repetation. I guess that would still work if const auto ret = Function_(args...); was replaced by const ReturnType ret = Function_(args...); though.

In any case, I don't think it matters too much either way.

In any case, I don't think it matters too much either way.

Makes sense. Both auto and RetType would have the same functionality. But the auto keyword allows maintaining a standard structure, so it makes sense to use auto here.

ACK 61ec0539b26a902a41a2602187a71f9dba3c6935

The bug is fixed and the refactoring makes sense.

ACK 61ec0539b26a902a41a2602187a71f9dba3c6935

Reviewed code and verified that addpeeraddress now actually returns whether the address was successfully added.

Concept ACK. Good idea. I'm seeing addrman unit test failures that appear to begin with 2095df7b7bfcb; maybe there is an interaction with other changes on master that wasn't apparent in this branch, but I am only beginning to catch up with the changes of the past few weeks. Edit: it looks like the errors had a different cause.