💬 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.
(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.
(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)
(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.
(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.
(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?
(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
(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
...
(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
...
(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.
(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.
(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`.
(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.
(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`.
(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
...
(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
...
💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1867640087)
912377ac4999467be7dfd51481c38972fb1475dd: maybe call it `-maintainers` to reduce confusion with `-unsigned`?
A more generic term might also be handy in the future if e.g. we want to include an OTS timestamp that commits to the (pre codesigning, pgp signed?) guix hashes (with all architectures).
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1867640087)
912377ac4999467be7dfd51481c38972fb1475dd: maybe call it `-maintainers` to reduce confusion with `-unsigned`?
A more generic term might also be handy in the future if e.g. we want to include an OTS timestamp that commits to the (pre codesigning, pgp signed?) guix hashes (with all architectures).
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1867682422)
> I'm not sure I understand the reason for the suppression
The reason of the suppression is the correctness of our code.
> please see my original suggestion in [#31306 (comment)](https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700)
https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1861679662
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1867682422)
> I'm not sure I understand the reason for the suppression
The reason of the suppression is the correctness of our code.
> please see my original suggestion in [#31306 (comment)](https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700)
https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1861679662
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867697664)
`GetNewNodeId()` is private. Not worth weakening the encapsulation for this test.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867697664)
`GetNewNodeId()` is private. Not worth weakening the encapsulation for this test.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867701118)
Done
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867701118)
Done
💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1867703633)
Thanks!
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1867703633)
Thanks!