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

jnewbery · 2021-10-28T12:03:35Z

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.

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

jnewbery · 2021-10-28T12:04:00Z

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

mzumsande · 2021-10-28T12:38:01Z

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.

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).

Keep the internal {Function}_() functions grouped together. Review with `git diff --color-moved=dimmed-zebra`

shaavan

ACK 61ec053

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}()?

jnewbery · 2021-10-28T13:46:20Z

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:

bitcoin/src/addrman_impl.h

Line 144 in ab25ef8

private:

shaavan · 2021-10-28T13:56:49Z

There's a private specifier a few lines above:

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

jnewbery · 2021-10-28T14:07:17Z

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.

shaavan · 2021-10-28T14:11:49Z

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.

naumenkogs · 2021-10-29T07:47:57Z

ACK 61ec053

The bug is fixed and the refactoring makes sense.

mzumsande · 2021-10-30T20:21:29Z

ACK 61ec053

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

… logging 61ec053 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery) 2095df7 [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery) 2658eb6 [addrman] Rename Add_() to AddSingle() (John Newbery) e58598e [addrman] Add doxygen comment to AddrMan::Add() (John Newbery) Pull request description: 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. ACKs for top commit: naumenkogs: ACK 61ec053 mzumsande: ACK 61ec053 shaavan: ACK 61ec053 Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb

Hopefully fixes new addrman unit test failures that appear to have been introduced in commit 2095df7 "Add Add_() inner function, fix Add() return semantics" of PR bitcoin#23380, which was merged in 994aaaa. That commit removed the bool fNew logic from AddSingle() and replaced it with returning fInsert. This patch brings back the fNew logic and combines it with fInsert, so AddSingle() returns whether or not fNew and fInsert are both true.

jonatack · 2021-11-03T19:56:18Z

Concept ACK. Good idea. I'm seeing addrman unit test failures that appear to begin with 2095df7; 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.

jnewbery added 2 commits October 28, 2021 12:51

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

e58598e

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

[addrman] Rename Add_() to AddSingle()

2658eb6

jnewbery force-pushed the 2021-10-addrman-add-logging branch from 8853611 to 7f06f53 Compare October 28, 2021 12:52

jnewbery added 2 commits October 28, 2021 14:00

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

61ec053

Keep the internal {Function}_() functions grouped together. Review with `git diff --color-moved=dimmed-zebra`

jnewbery force-pushed the 2021-10-addrman-add-logging branch from 7f06f53 to 61ec053 Compare October 28, 2021 13:01

DrahtBot added the P2P label Oct 28, 2021

shaavan approved these changes Oct 28, 2021

View reviewed changes

fanquake merged commit 994aaaa into bitcoin:master Nov 1, 2021

jnewbery deleted the 2021-10-addrman-add-logging branch November 1, 2021 07:08

This was referenced Nov 3, 2021

Addrman unit test failures #23433

Closed

p2p: fix regression in AddrManImpl::AddSingle() #23434

Closed

bitcoin locked and limited conversation to collaborators Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

jnewbery commented Oct 28, 2021

Uh oh!

jnewbery commented Oct 28, 2021

Uh oh!

mzumsande commented Oct 28, 2021

Uh oh!

shaavan left a comment •

edited

Loading

Uh oh!

jnewbery commented Oct 28, 2021

Uh oh!

shaavan commented Oct 28, 2021

Uh oh!

jnewbery commented Oct 28, 2021

Uh oh!

shaavan commented Oct 28, 2021

Uh oh!

naumenkogs commented Oct 29, 2021

Uh oh!

mzumsande commented Oct 30, 2021 •

edited

Loading

Uh oh!

jonatack commented Nov 3, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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

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

Uh oh!

Conversation

jnewbery commented Oct 28, 2021

Uh oh!

jnewbery commented Oct 28, 2021

Uh oh!

mzumsande commented Oct 28, 2021

Uh oh!

shaavan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 28, 2021

Uh oh!

shaavan commented Oct 28, 2021

Uh oh!

jnewbery commented Oct 28, 2021

Uh oh!

shaavan commented Oct 28, 2021

Uh oh!

naumenkogs commented Oct 29, 2021

Uh oh!

mzumsande commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shaavan left a comment •

edited

Loading

mzumsande commented Oct 30, 2021 •

edited

Loading

jonatack commented Nov 3, 2021 •

edited

Loading