⚠️ fanquake reopened an issue: "brew: serfloat_tests tests fail on Linux"
(https://github.com/bitcoin/bitcoin/issues/28941)
While upgrading sqlite to 3.44.1, we've found some regression test failure


<details>
<summary>test failure log</summary>

```
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/25.1/bin/test_bitcoin
Running 557 test cases...
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [3933824172291540 != 0]
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1522707584769707 != 0]
t
...

💬 fanquake commented on issue "brew: serfloat_tests tests fail on Linux":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-2150440807)
Reopening as this is still failing, see: https://github.com/Homebrew/homebrew-core/pull/169269#issuecomment-2150438892.

Can be reproduced with:
```bash
docker pull docker.io/homebrew/brew:latest
docker run -it homebrew/brew

brew install bitcoin --head --debug # ignore the 2 patch failures
brew test bitcoin

/home/linuxbrew/.linuxbrew/Cellar/bitcoin/HEAD-ff7d205/bin/test_bitcoin
Running 610 test cases...
test/serfloat_tests.cpp(95): error: in "serfloat_tests/double_serfloat_tests":
...

💬 dergoegge commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1627820274)
```suggestion
// Copyright (c) The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
```

💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1628090346)
Done (through `.rbegin()` instead as your `std::reverse_iterate` suggestion didn't compile).

📝 sr-gi opened a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233)
`m_is_inbound` cannot be changed throughout the life of a `Peer`. However, we are currently storing it in `CNodeState`, which requires locking `cs_main` in order to access it. This can be moved to the outside scope and only require `m_peer_mutex`.

This is a refactor in preparation for Erlay reworks.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should
...

💬 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.