💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867430106)
Would rather not change to quote paths in all log messages of this function as it would touch even more lines.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867438656)
I've updated that commit to better document that I'm overriding the 2 methods (unfortunately our *.python-version* is 3.10.14, python 3.12 includes `@override`).

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867460705)
PR does not contain a test for this as I thought it was straightforward enough to skip and I didn't want to add more things to review/maintain. If we pass `-nopid` we simply do not:
- Create the file
- Write the PID
- Set the `bool` to remember deleting the file
- (Delete the file)

https://github.com/bitcoin/bitcoin/blob/95a0104f2e9869799db84add108ae8c57b56d360/src/init.cpp#L165-L192

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867433277)
Thanks, agree that it's a bit redundant in an assert, leftover from previous comment. Updated.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867461728)
Moved back the log message in latest push to clarify.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867463418)
Cheers, replaced hashes with commit message titles.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867468510)
Having a latent *bitcoin.conf* is not ideal and something the user should correct if they want to run with `-noconf` long-term to reduce potential for confusion ("Why is my change to *bitcoin.conf* not getting picked up?!!" until realizing `-noconf` was used). But changed to `LogInfo` for now.

✅ willcl-ark closed a pull request: "build: special instruction check script"
(https://github.com/bitcoin/bitcoin/pull/26693)

💬 willcl-ark commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2514215309)
Closing for now, as there's not much interest currently. Would be happy to re-open in the future.

💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867506582)
I don't think this works reliably? See:

```
This output will be buffered if written to a file or pipe, so a program reading from the file or pipe may not see packets for an arbitrary amount of time after they are received. Use the -U flag to cause packets to be
written as soon as they are received.

💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867509291)
Why is this needed? The file isn't cleared anyway (and exited if it isn't clear), so might as well just run the processes and never kill them?

Also, it doesn't seem to be working anyway on some of the tasks?

💬 maflcko commented on issue "scripts: check for .text.startup sections":
(https://github.com/bitcoin/bitcoin/issues/18603#issuecomment-2514249729)
https://github.com/bitcoin/bitcoin/pull/26693 was closed

⚠️ hebasto opened an issue: "qa: Broken `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/issues/31409)
On the master branch @ ebe4cac38bf6c510b00b8e080acab079c54016d6, the `wallet_multiwallet.py` test has several issues:

1. This code: https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/test/functional/wallet_multiwallet.py#L132-L140 checks for the "Error scanning" message in the `debug.log` caused by processing the `no_access` directory. However, the same message can also be generated when parsing the `self_walletdat_symlink` directory. As a result, the current imp
...

💬 laanwj commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2514359474)
What was the blocker here?

Is the problem "how to check just-generated .o files" in the CI/GUIX build?

> This sounds very cool (and more practically-useful than the check here perhaps).

Revisiting this, i'm not sure. Checking the final binary would be more practical with integration in the process (it could run with the other symbol/security checks), but with optimizations like LTO it's not necessarily true that the different init functions end up in different places. So there's somethi
...

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2514411310)
> It's unclear to me whether the standalone binaries need to be notarized too.

Do you mean the binaries in `unsigned.{zip,tar.gz}` archives? I think it's fine not to.

👍 laanwj approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2475532101)
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196

i have checked that behavior stays the same with regard to adding DIRTY and FRESH flags, i did not check all the test changes in detail but overall LGTM.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867660305)
`CNode::addr` needs to be `CAddress` because at least `PeerManagerImpl::PushNodeVersion()` is using `addr.nServices`.

`CNode::addrBind` indeed does not need to be `CAddress`. I changed it to `CService`.

💬 maflcko commented on issue "qa: Broken `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/issues/31409#issuecomment-2514465610)
I guess it would be good to separate the no_access and symlink unit tests into two test cases, not combine them into one. This would allow to skip just one of them, if needed on Windows.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867664745)
"Peer" is a good description for this, but that is already used in `net_processing`.

🤔 Sjors reviewed a pull request: "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries"
(https://github.com/bitcoin/bitcoin/pull/31407#pullrequestreview-2475544913)
In order to properly test this, you would have to provide the detached signatures and staple for this PR.

Reviewers then need to _download_ it from some website. If you obtain the file via SSH from your own guix machine, macOS tries to be smart about it (at least my Intel mac used to do that).

```sh
HOSTS="x86_64-w64-mingw32 x86_64-apple-darwin arm64-apple-darwin" ./contrib/guix/guix-build
...
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort
...