💬 Sjors commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2333554993)
re-utACK e225f7cbc3e6842f8e7f1c482c2aacd810e99c1b

I plan to test this with https://github.com/Sjors/bitcoin/pull/48 but atm the rebases involved are a bit too much headache.

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333558016)
> In tinyformat there is no such thing, so I'll leave this as-is for now.

My motivation is not to reach parity with *tinyformat*, it is to at least cover some of what the prior linter did:

https://github.com/bitcoin/bitcoin/blob/c3af4b1ec3fdb308404199ddd0df5170793a2c39/test/lint/run-lint-format-strings.py#L230-L253

An alternative to making it an error is to simply make invalid chars not count, which is closer to the old behavior. Would that be okay?

If you don't want to bring over at
...

💬 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_r1746733558)
Not a big deal, but diff was already quite large.

💬 maflcko commented on pull request "test: Pin and document TEST_DIR_PATH_ELEMENT, SeedRand::FIXED_SEED":
(https://github.com/bitcoin/bitcoin/pull/30748#issuecomment-2333560129)
> IMO, `TEST_DIR_PATH_ELEMENT` would be clearer as `TESTS_DIR_NAME` and `"test_common bitcoin"` would be clearer as `"bitcoin tests"` but current names seem fine too.

Looks like https://github.com/bitcoin/bitcoin/pull/30737 may or may not change the path layout, so any clarifications could in theory be done there.

💬 fanquake commented on issue "ci: failure in win64 unit tests":
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2333563239)
I wonder if it was an intermittent `wine` related issue, but the output was missing. Similar to something like: https://cirrus-ci.com/task/6309656165875712.

🚀 fanquake merged a pull request: " bench: Remove redundant logging benchmarks "
(https://github.com/bitcoin/bitcoin/pull/30790)

💬 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#issuecomment-2333566726)
@vasild it's probably easier to reverse those two commits: first move the implementation out, and then split the class.

🤔 TheCharlatan reviewed a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2285497604)
Concept ACK

💬 TheCharlatan commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746748313)
How about making the parameter `&&` to avoid accidental copies?

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746787689)
It was deliberately added in other to allow caller to decide whether to move or make a copy see https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746002806

💬 TheCharlatan commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746797435)
Ah, I see. Please resolve :)

💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2333640042)
> @vasild it's probably easier to reverse those two commits: first move the implementation out, and then split the class.

`e995ffa5c3...e7cf8e8fc6`: done, same as before, the cumulative diff is identical before and after:

```sh
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e995ffa5c~3..e995ffa5c)
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e7cf8e8fc~3..e7cf8e8fc)
$
```

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1746805357)
Thanks

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333643144)
Thanks for clarifying! I think not counting "invalid" ones would be wrong, because tinyformat treats them as valid. Silently not counting them is another bug in the Python code, because it will lead to a `tinyformat: Too many conversion specifiers in format string`. See https://godbolt.org/z/4ncfqvnjY

I didn't really want to give a full list of all bugs in the Python code, but I've modified the PR summary to mention this bug as well. Let me know if this sounds acceptable.

💬 fanquake commented on pull request "ci: Use clang-19 in msan tasks":
(https://github.com/bitcoin/bitcoin/pull/30639#issuecomment-2333665854)
Given that final has been pushed back a bit, also happy to put this in at `rc4`, and bump it again later (if needed).

💬 maflcko commented on issue "ci: failure in win64 unit tests":
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2333669861)
Yeah, possibly. I wonder why there is a presumed increase in those errors recently.

💬 maflcko commented on pull request "ci: Use clang-19 in msan tasks":
(https://github.com/bitcoin/bitcoin/pull/30639#issuecomment-2333672420)
I'd say to keep testing minimal my preference would be to just touch it once.

(The aarch64 build has to be done manually every time)

💬 maflcko commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2333681810)
Would be nice to change the title from "... transactions v2" to something else. Otherwise, it may be a bit confusing in the context of "v2 transactions" (transactions with version number set to `2`, not `3`), or "v2 transport" (the P2P thing).

💬 fanquake commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2333693862)
Fixing this looks good. I guess going forward it currently has the caveat that it's expected to work most correctly in the CI (on the same hardware used by the CI). i.e this it fail for any dev trying to run the script locally, on aarch64:
```bash
[ 25%] Built target bitcoin_crypto_arm_shani
[100%] Built target bitcoin_crypto
gmake: *** No rule to make target 'bitcoin_crypto_x86_shani'. Stop.
```
or if a dev compiles on x86_64, but doesn't match the CI config, they may see incorrect outpu
...

🚀 fanquake merged a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415)