💬 luke-jr commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2150551869)
>A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.

How do you propose resolving this? It's not really a circular dependency, just equivocated as such due to the CI test stripping file extensions, but maybe there's a better approach that just fixing CI?

👍 ryanofsky approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2099552897)
Code review ACK 9617e42a7b12a05ed8a0815c5a6537e1614727f5. I didn't follow all the previous discussion, but I like the place this PR ended up accepting simple "owner" "group" and "all" settings and avoiding more complicated corner cases.

I left some suggestions which are mostly not important, though I do think it would be better to try to stay more backwards compatible and avoid setting permissions when it is not requested.

💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1627997774)
In commit "rpc: add default 600 permissions for cookie file" (35df85346438258e1bb5f736e9c4d48cf59e6547)

This commit message is confusing because it sounds like it changing the permissions of the cookie file, but the diff only shows it adding a constant declaration and not changing any code?

It seems like it would be better if this commit were combined with the third commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a) where the new constant is actu
...

💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1628110701)
In commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)

I think it would be good to change `fs::perms` parameter to `std::optional<fs::perms>` and not call `fs::permissions()` unless `-rpccookieperms` option is specified, for backwards compatibility.

It is easy to imagine a new fs::permissions(DEFAULT_COOKIE_PERMS) call having bad side effects on platforms like windows that use acls, or maybe failing on filesystems that do not support permissions.
...

💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1628108133)
In commit "init: add option for rpccookie permissions" (ae7ec049e5c8d2361c7f47cad3e8c13f912f1c2a)

It would be nice to drop this ifdef special case for windows. Even though windows uses acls, the fs::permissions call should still be able to do the requested things by setting acls for the "Users" and "Everyone" groups on windows. I don't think we need to go out of our way to second guess the library and operating system here. If the operating system or filesystem do not support permissions, or
...

🤔 murchandamus reviewed a pull request: "wallet: re-activate "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-2099857752)
Concept ACK

💬 murchandamus commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r1628175898)
It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the `preset_inputs` not also be reduced to their effective amount in line 1139?

📝 theuni opened a pull request: "Enable clang-tidy checks for self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30234)
See comment here: https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148229582

Our code failed these checks in two places, which have been fixed up here. Though these appear to have been harmless, adding the check avoids the copy in the self-assignment case so there should be no downside.

Additionally, minisketch failed the check as well. See https://github.com/sipa/minisketch/pull/87

After fixing up the violations, turn on the aggressive clang-tidy check.

Note that I only l
...

💬 hebasto commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2150635749)
Concept ACK.

I've created a similar branch this morning following the discussion in https://github.com/bitcoin/bitcoin/pull/30161 :)

👍 TheCharlatan approved a pull request: "guix: bump time-machine to f0bb724211872cd6158fce6162e0b8c73efed126"
(https://github.com/bitcoin/bitcoin/pull/30231#pullrequestreview-2100071418)
ACK 2599655c1fb8e7d0b8407d2be249977372cb73ff

Guix builds (aarch64):
```
766de337912412943b38f6f0dd100bafaa858bf2ad43bbc56de9de86b59f99c9 guix-build-2599655c1fb8/output/aarch64-linux-gnu/SHA256SUMS.part
a426d7189067919d418e164b5770c580bc6e94c3aa5320b2980c65762055ec92 guix-build-2599655c1fb8/output/aarch64-linux-gnu/bitcoin-2599655c1fb8-aarch64-linux-gnu-debug.tar.gz
c70f267474be9121d70e217c966c6c359ff12efb369a98e787dabc63221e2a47 guix-build-2599655c1fb8/output/aarch64-linux-gnu/bitcoin-
...

💬 furszy commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r1628301329)
> It seems that both in lines 1137 and 1139 the preset_inputs are counted with their full amount rather than their effective amount. Should the `preset_inputs` not also be reduced to their effective amount in line 1139?

Thats because `preset_inputs.total_amount` could either be the full amount or the effective one (see `PreSelectedInputs`). I didn't want to introduce more changes within this PR but we should split the amounts (probably replacing `PreSelectedinputs` usage for `CoinsResult`).

📝 theuni opened a pull request: "build: warn on self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30235)
Belt-and suspenders after #30234. Self-assignment should be safe _and_ discouraged.

We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore.

💬 theuni commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2150803796)
@maflcko Hmm, how best to ignore the tidy false-positive in a leveldb header?

> ./leveldb/include/leveldb/status.h:106:24: error: operator=() does not handle self-assignment properly

It looks like we could add `ExcludeHeaderFilterRegex` to our tidy config, but that's not wired up until clang-tidy-19.

💬 fanquake commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150813579)
```bash
test/uint256_tests.cpp:273:7: error: explicitly assigning value of variable of type 'arith_uint256' to itself [-Werror,-Wself-assign-overloaded]
v /= v;
~ ^ ~
test/uint256_tests.cpp:277:7: error: explicitly assigning value of variable of type 'arith_uint256' to itself [-Werror,-Wself-assign-overloaded]
v -= v;
~ ^ ~
2 errors generated.
```

💬 theuni commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150869143)
Ran into this: https://github.com/llvm/llvm-project/issues/42469

Worked around it by disabling the warning for the specific tests rather than removing them.

💬 theuni commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2150905605)
Turns out [clang fixed this in v17](https://github.com/llvm/llvm-project/commit/c5302325b2a62d77cf13dd16cd5c19141862fed0). Updated the comment and commit message to reflect that.

👍 TheCharlatan approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2100282452)
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4

📝 theuni opened a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236)
Noticed while looking at the `-wno-*` flags in #30235.

This was disabled in #18738 due to the combo of old gcc and qt. We no longer support the affected gcc, and the old qt should no longer be relevant to us anyway.

See old fixes in:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88136
and
https://bugreports.qt.io/browse/QTBUG-75210
and
https://codereview.qt-project.org/c/qt/qtbase/+/245434

💬 mzumsande commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2150971292)
Not sure if it's helpful to do anything in between releases, but if it's still broken in August, I'd suggest to remove it for 28.0 (a new seeder will be added in #30007, so the total number wouldn't change).

🤔 ryanofsky reviewed a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2100288867)
Sorry for not seeing this earlier. I think I found a potential problem with this change, so I left a comment below and would be curious to find out more.