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

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1842795172)
Concept ACK

💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1417239636)
> Is this workaround still needed with https://github.com/bitcoin/bitcoin/pull/28856? That PR seems to drop the android workaround, so maybe switching both of these libraries to cmake would just fix the problem automatically.

Yes, the switch to CMake in #28856 didn't fix this issue.

💬 naumenkogs commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1417250008)
ea3138b7b189158db80bfd1f9d9dd17e7ef52f3f

might as well jump `pindex` forward here, so that the second condition is satisfied?

💬 aureleoules commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1842819914)
> Maybe something was being cached, as this seems to have updated now.

Yes it may take some time for the sonarcloud worker to finish, as well as sonarcloud to refresh the results.
I'll improve the UX for that 😅

💬 naumenkogs commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#issuecomment-1842833890)
The code looks alright, but i'd still rather have [this](https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1557211930) addressed first. It looks like even #28170 doesn't directly handle this, but rather prepare the codebase for another PR handling it?

💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1842866482)
I would NACK that as you are creating infrastructure for things like OFAC censorship.

On December 6, 2023 1:21:15 PM GMT+01:00, Sjors Provoost ***@***.***> wrote:
>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.
>