💬 fanquake commented on pull request "build: Use CMake's default permissions in macOS `deploy` target":
(https://github.com/bitcoin/bitcoin/pull/30838#issuecomment-2337875369)
Concept ACK - looks like switching this to use CMakes default permissions, rather than ours, does fix the issue, but I guess could use more discussion given: https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2336699084.

💬 l0rinc commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r1750101002)
Based on the message and
> - `LogError(fmt, params...)` should be used in place of `LogInfo` for
severe problems that require the node (or a subsystem) to shut down
entirely (e.g., insufficient storage space).

and the discussion in https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750090426, this looks like a `LogError` instead (will throw in https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L895-L898 if `nullopt` is returned).

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750113102)
Thanks, used `LogError` in [`90aa839` (#30849)](https://github.com/bitcoin/bitcoin/pull/30849/commits/90aa839456163a48e675f601ff6a05330aa5c5c6)

⚠️ willcl-ark opened an issue: "Increasing self-hosted runner raw performance"
(https://github.com/bitcoin/bitcoin/issues/30852)
_disclaimer_: The following should **not** replace us investigating and fixing the root causes of timeouts and intermittent test runtime performance.

Now seems an opportune time to open a discussion on some investigation I have been doing into our self-hosted runners, as our CI has been struggling again recently.

I wanted to see what the cost/benefit implications would be on upgrading our self-hosted runners would look like. Hooking up a single [Hetzner AX52](https://www.hetzner.com/dedica
...

💬 l0rinc commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/30845#issuecomment-2337973235)
Manually recopied https://github.com/bitcoin-core/secp256k1/commit/2f2ccc469540fde6495959cec061e95aab033148 into https://github.com/bitcoin/bitcoin/tree/ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f/src/secp256k1 - seems to be done correctly, no changes detected by git.

ACK ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2337987820)
> each job run[s] on average 3-5x faster

Nice find. I didn't expect such a difference, but your results look promising to give this a try.

💬 maflcko commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750175818)
Maybe I am missing something, but commit a2a6f320878ddbedfca856d512d862fb46df4b23 says:

```
> "LogError(fmt, params...) should be used ... for severe problems that require the node ... to shut down entirely".

Since this will throw a `TX_PREMATURE_SPEND` in `MemPoolAccept::PreChecks`, we'll use `LogError` instead.
```

`TX_PREMATURE_SPEND` may happen in normal operation (for example, it happens in the tests) and does not require the node to shut down entirely. Also, the node does not sh
...

💬 ryanofsky commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2338011842)
> I'm still confused. Why should we "avoid writing the cache" for variables such as `WITH_ZMQ`? Using cache variables is a standard CMake method that allows the user to set their values.

Sorry that was not clear. I should have said that "force writing the cache" and overwriting values set by users should be avoided. Ideal behavior is that user can run depends build and it will set default cache values. Then they can run `ccmake` or plain `cmake` and change settings freely and those values wil
...

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750222723)
> Also, changing the log severity is not a "refactor".

K, dropping this commit in that case

⚠️ ouziel-slama opened an issue: "No ZMQ `sequence` notifications on Regtest"
(https://github.com/bitcoin/bitcoin/issues/30853)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

No message received in the `sequence` feed when a transaction is added to the mempool.

### Expected behaviour

We should receive a ZMQ notification when a new transaction enters the mempool. This works fine on mainnet.

### Steps to reproduce

configure Bictoin Core with:
```
[regtest]
zmqpubrawtx=tcp://0.0.0.0:29332
zmqpubhashtx=tcp://0.0.0.0:29332
zmqpubsequence=tcp://0.0.0.0:29332
...

👍 real-or-random approved a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30845#pullrequestreview-2289919879)
utACK https://github.com/bitcoin/bitcoin/pull/30845/commits/ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f no blockers from libsecp256k1 side, and these commits touch only build/docs/tests and are thus particularly harmless

💬 pinheadmz commented on issue "No ZMQ `sequence` notifications on Regtest":
(https://github.com/bitcoin/bitcoin/issues/30853#issuecomment-2338139218)
There is a test for zmq sequence (`interface_zmq.py`) so it must be working on regtest. Compare your code to the `test_sequence()` block?

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750286691)
> nit: Seems like this isn't needed so I would have preferred if that was left out or a separate commit because the file isn't touched at all otherwise.

This is needed to prevent a race condition. Since commit fafc73bc036792923ca64a112684abaf78f3b48e, `nLocalServices` can be accessed from different threads. Therefore, we need to store the value once it's read and use it here and few lines below (in the `InitializeNode` call), to maintain consistency in this flow. Otherwise, `use_v2transport`
...

💬 sipa commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2338162908)
> I wonder if this issue could be caused by number of calls we make to `fwrite` because of the write buffering in xor path of `AutoFile::write()`, maybe platforms other than windows are optimizing our writes into batches behind the scenes? I don't know much about how I/O works behind the scenes, just a guess.

I have not checked, but this would be my guess.

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750305246)
> If this is self-contained and you need to wipe everything this could have been done in a separate test line `p2p_assumeutxo.py` for example and you copy over the setup from this file here but it seems like there isn't much needed.

I started there (see me crying about the `feature_assumeutxo.py` code organization here https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2281468112), but ended up moving it inside this all-in-one file because I wasn't happy copy pasting the costly
...

🤔 l0rinc reviewed a pull request: "fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests"
(https://github.com/bitcoin/bitcoin/pull/29572#pullrequestreview-2289988052)
I'd prefix the PR with `test:` to obviate that it doesn't touch production code.
And the PR description isn't convincing enough to me.

💬 l0rinc commented on pull request "fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests":
(https://github.com/bitcoin/bitcoin/pull/29572#discussion_r1750307637)
This was addressed in https://github.com/bitcoin/bitcoin/pull/30849/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R454 without an assert/Assert

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2338212801)
Guix build (x86_64) note that macOS builds (probably) wont match:
```bash
b3bcd9f7508b35b3f4a187e9f1e4c87648bf3897575c0c4db8664f1fe08c2cf4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/SHA256SUMS.part
356c7b9493887fc839e6b7951a3396085bfea33efce91070f972a69f60aac1f4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-be4f78275fa6-aarch64-linux-gnu-debug.tar.gz
e2887613ca5d1f929294487f86bd36ab6af0739d12625c6c661a35781c4f1523 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-b
...

💬 fanquake commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/30845#issuecomment-2338219423)
Guix Build (aarch64) note that macOS builds (probably) wont match:
```bash
d5dce4d500967a50a4f5717aabb50bac198ce3a1524b8d8bfdfc015a7bc2ddd4 guix-build-ff54395de421/output/aarch64-linux-gnu/SHA256SUMS.part
90eaee7f778ed489884dccdc049341ee218372aaf782c6ed43a503d16433c34f guix-build-ff54395de421/output/aarch64-linux-gnu/bitcoin-ff54395de421-aarch64-linux-gnu-debug.tar.gz
10190f271c5181c9c956fd36aee9f22f36c80ca844a805902bbc0a078ca0fe56 guix-build-ff54395de421/output/aarch64-linux-gnu/bitcoin-
...

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750343042)
> I think we should allow assumeUTXO snapshot customization on regtest. It would make testing much simpler and self-contained.

How would it make it simpler? It's another thing to keep track of in the test code and the implementation code. If we need more snapshot heights we can always add them but I haven't felt the need for it so far when I have added new coverage.