💬 0xB10C commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1417047413)
> Is a copy the right way, or does it need to be removed from the expected tried table?

Just removing the address from the _expected_ tried table does not work. It's still in the new table. So the question would rather be: "Do we need to retry adding the address to the tried table?". I guess that's debatable. We're only running into this case very rarely when there's a collision - all other runs test that this works. This should be solved with #29007.

> Maybe add steps to reproduce the tes
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1842597688)
@sr-gi fixed the co-authorship. Thank you for patience :)

took @maflcko suggestion on deterministic tests.

💬 maflcko commented on issue "Increase fuzz coverage in the wallet":
(https://github.com/bitcoin/bitcoin/issues/27272#issuecomment-1842602935)
> The wallet has (almost) no coverage: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/index-sort-l.html. We should change that!

For reference, the current OSS-Fuzz coverage link: https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20231205/linux/src/bitcoin-core/src/wallet/report.html

💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417082950)
I wonder if this can be generalized to pass test-only options (along with a helper `HasTestOption`, similar to `IsDeprecatedRPCEnabled`?

The user-facing debug-help output of this seems the wrong place to document test-only options, and the source code seems sufficient to be self-documenting.

```suggestion
argsman.AddArg("-test=<option>", "Pass a test-only option", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
```

💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417083583)
In any case, `(default: %s)", "relay"` is wrong, no?

💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417084172)
```suggestion
const auto options = args.GetArgs("-addrtest");
```

💬 0xB10C commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1842629300)
Maybe get #29007 in first. This allows us to remove the getrawaddrman-test retry logic (possibly do it there). Makes the `getrawaddrman` test simpler.

I've addressed the minor comments here, but I think it makes sense to draft this PR for now until #29007 is in. With #29007 we can test the collision case when `addpeeraddress tried` returns `{"success": false, "error": "failed-adding-to-tried"}` as discussed in https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1417047413.

📝 0xB10C converted_to_draft a pull request: "rpc: addpeeraddress tried return error on failure"
(https://github.com/bitcoin/bitcoin/pull/28998)
When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue #28964.

This is fixed by new returning `{ "success": false, "error": "..." }`
...

💬 fanquake commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1842632814)
> Yes that is correct. Also the compiler installed is g++-11.

It would be useful to have compiler/version used/C{XX} flags/configure options (even if just mentioning it's the default) displayed next to the benchmarking information, as I assume this will always be one of the first questions asked in relation to benchmark output.

🤔 maflcko reviewed a pull request: "fuzz: wallet, add target for Spend"
(https://github.com/bitcoin/bitcoin/pull/28236#pullrequestreview-1767252938)
Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html

It may be good to clarify the new coverage a bit.

💬 maflcko commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1417093434)
Seems like this should be a separate fuzz target?

💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1417099754)
Could also move them out of the loop, like the others?

👍 maflcko approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-1767263157)
lgtm

💬 fanquake commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1842648355)
Concept ACK

💬 hebasto commented on pull request "test: Fix test by checking the actual exception instance":
(https://github.com/bitcoin/bitcoin/pull/28989#issuecomment-1842649380)
Friendly ping @Sjors :)

💬 fanquake commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1842717748)
Rebased on master/the current C++20 PR, which should improve the CI here.

@aureleoules does the SonarCloud output always run on the latest changes? I've dropped all the `inline` usage from the constexpr functions in `endian.h`, but https://corecheck.dev/bitcoin/bitcoin/pulls/28674 still shows all the output about using inline with constexpr?

💬 TheCharlatan commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1417160672)
Nit: Could this be private?

💬 TheCharlatan commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1417160346)
Calling `app.node()` here before `app.createNode` results in a crash on windows when running `bitcoin-qt.exe`. I think the most straight forward way to resolve this would be to move this block of code to after `createNode` is called.

Alternatively, to keep the current control flow, the way I read it, `make_node` does not actually manipulate the node context, besides eventually setting it in the `NodeImpl` further down the call stack. So I think the call to it could be moved further up:
```di
...

💬 0xB10C commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417165681)
You might also want to edit/update/remove(?) this comment. I think mentioning that we avoid collisions by adding known-working addresses to the deterministic addrman would be good.

💬 naumenkogs commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1417173713)
nit: is `the newest` enforced by `ForEachNode` iterating? Perhaps make this requirement more clear? Perhaps even add an assert for `m_connected` before `node_to_evict = node->GetId();`