💬 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();`

💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1842735340)
> Can we get rid of linker warnings:

I assume this is a combination of `-mmacosx-version-min`, `-Wl,-platform_version` and maybe cctools. Note that the warnings have also already been appearing in our security checks/tests, for some time. This should dissapear when we switch to lld, so it might not be worth making more changes now.

💬 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_r1417175382)
Assume `fDisconnect` could be useful here too. Just in case `ForEachNode` gets updated.

💬 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_r1417178196)
Not sure why anyone would do `maxconnections=1` in practice, but then if their firewall allows to find only one blocksonly node (and they are not in blocksonly), the behavior might be confusing: they will be frequently disconencted from this peer.

Perhaps, this could be logged if maxconnections=1

📝 maflcko opened a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009)
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.

Fix this by detecting them in the fuzz target.

Can be tested by introducing a bug such as:

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::ato
...

💬 Sjors commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842754101)
NACK. There's no universal way to filter all present and future datacarrying styles. This invites an endless cat and mouse game inside very critical code paths.

It seems better if you just keep this patch for Knots, or alternatively, come up with a generic design to filter transactions. E.g. by loading rules from a file in some template language.