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

💬 Sjors commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842759347)
Just to set expectations, I might also NACK such a generic arbitrary filtering system, given the can of worms it would open, but I'd have to think about that more.

🤔 furszy reviewed a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009#pullrequestreview-1767421817)
Concept ACK

💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1842790794)
Fixed up capnp as well. Discussed this a bit with @theuni yesterday, and it seems to most straightforward to just keep using `/lib` (also discussed above) given thats how depends was built to work, and it's not clear why changing things to exist in various (depending on the system and it's configuration) GNU dirs is any better (for now). If we actually switched all packages in depends to using CMake, maybe we could revisit this (also assuming they all themselves use GNUInstallDirs).