💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746321)
added an optional sync argument, only unused during reorg test

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746418)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746514)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746680)
great idea, added

👍 Sjors approved a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2282985518)
ACK e59097a0a56ecaffdb38aaa94e232f5b45881897

You can see it in action in `sv2_connman_tests` in https://github.com/Sjors/bitcoin/pull/50.

I didn't test the `CNetMessage` functionality and `Eof()` method.

Maybe split 187ba684a9d5e63d09b43511b7128cce9edbedf3 into one commit that moves the implementation and another that splits the class.

💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1745516413)
187ba684a9d5e63d09b43511b7128cce9edbedf3: shouldn't `Sock{INVALID_SOCKET}` be preserved in the new `ZeroSock` constructor?

💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1745496410)
187ba684a9d5e63d09b43511b7128cce9edbedf3 any particular reason for moving this?

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746765)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1745746912)
elaborated a bit, let me know if it helps

💬 hebasto commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331991030)
> The alternative [#30787 (comment)](https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298) seems fine as well.

This PR change is minimal.

👍 hebasto approved a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822#pullrequestreview-2283396791)
re-ACK a7a4e11db8722c85175bcc4d9f73a713e9e61513.

💬 kevkevinpal commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331995116)
lgtm ACK [a7a4e11](https://github.com/bitcoin/bitcoin/pull/30822/commits/a7a4e11db8722c85175bcc4d9f73a713e9e61513)

did `time cmake -B build` and got the following results

PR30822 ([a7a4e11](https://github.com/bitcoin/bitcoin/pull/30822/commits/a7a4e11db8722c85175bcc4d9f73a713e9e61513))
```
real 0m13.740s
user 0m6.798s
sys 0m4.696s
```

master (d661e2b1b771abafb0b152842d775d3150032230)
```
real 0m25.583s
user 0m17.078s
sys 0m5.127s
```

💬 maflcko commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2331996771)
review ACK a7a4e11db8722c85175bcc4d9f73a713e9e61513

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2332011275)
Rebased

👍 ryanofsky approved a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2283417302)
Code review ACK 766881e33d13ca2887f7ed179cdcc6bc007a6325. I like this approach, seems more generic and lightweight.

💬 ryanofsky commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1745761110)
In commit "build: Produce a usable static kernel library" (7739fa69462f070c62345f2f14fcf817a512d59a)

Seems ilke in theory this could install the same libraries more than once because it is just recursively adding dependencies without deduplicating them. For example if kernel library depends on util and crypto, and util library depends on crypto, crypto library could be listed twice. May be worth iterating over a unique version of the list so the install step is cleaner if it does contain dupl
...

📝 fanquake opened a pull request: "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type"
(https://github.com/bitcoin/bitcoin/pull/30824)
`FORTIFY_SOURCE` should be used if `ENABLE_HARDENING=ON` and optimisations are being used. This should not be coupled to any particular build type, because even if the build type is `Debug`, optimisations might still be in use.

Fixes: #30800.
Also somewhat of a followup to https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742257436.

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#issuecomment-2332049650)
Not sure about the inline source formatting. cc @hebasto.

💬 fanquake commented on issue "cmake: incorrect assumption that `Debug` build type will not use optimisations":
(https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2332050710)
> Not sure if this makes sense, but if the problem is that specifying _FORTIFY_SOURCE without optimizations triggers a compiler warning, maybe there should just be a check that drops _FORTIFY_SOURCE when it triggers the warning. That might avoid problems if there is a discrepancy between the way our cmake code detects optimizations and the way _FORTIFY_SOURCE implementation does.

That could be a good approach, but there's also the possiblity that FORTIFY will warn if the selected level will b
...

💬 TheCharlatan commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1745778510)
I was doing this, but removed it again from the patch. I feel like if there are duplicates, it is a bug in our library topology. But it could still make sense to guard against them.