💬 maflcko commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2596012787)
The msan CI failure seems unrelated, so I've rerun it and left a comment here: https://github.com/bitcoin/bitcoin/pull/31397/files#r1918744397

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1918750243)
> In thread [#31483 (comment)](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915356827):
>
> > I see. So the max amount you can safely shift is without overflowing is not `std::numeric_limits<W>::digits - 1` but actually `std::numeric_limits<W>::digits - std::numeric_limits<T>::digits` since that is how much free space there is to the right of T's bits in W.
>
> Would appreciate if someone were to ELI5 this explanation. I'm thinking it must be `clamp(widen(i) << shift)` that d
...

💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596016591)
> If this change is made, `depends/README.md` should be updated as well. Same for `ci/test/00_setup_env.sh` and `.github/workflows/ci.yml`.

I have force-pushed to include updates to those two files, along with a few more places; there are still several results to `git grep pkg-config`: mostly from old release notes, which should probably be left untouched; a few from other distros, where I've not tested things; and a few invocations of pkg-config which should work with either version, so I've
...

💬 sipa commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2596019759)
Concept ACK

💬 maflcko commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596024083)
> along with a few more places

You can't change the others, as they are fixed subtrees. (See the CI failure).

💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596030806)
The automated checks are now reporting two instances of:

```
FAIL: subtree directory was touched without subtree merge
```

from the places were I fixed a typo in a comment. I'm not familiar with this specific linter setup, and this isn't supposed to be a merge commit, so could someone please point me towards some documentation or just explain directly why this check fails?

💬 l0rinc commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2596035012)
I'm not sure these arguments would hold in other cases (e.g. no compiler safety needed since the tests would fail otherwise), but I've removed the consts as well, basically only leaving @ryanofsky's changes.

💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596041403)
> > along with a few more places
>
> You can't change the others, as they are fixed subtrees. (See the CI failure).

I force-pushed again to remove the modifications to the two Dockerfile scripts.

👍 hodlinator approved a pull request: "doc: Improve dependencies documentation"
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2556503320)
re-ACK 05a008c1d07a38f589536661822d5450aa4e9438

- Thanks to @rkrux for [spotting the formatting issue](https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913559740) I missed.
- SQLite now categorized as `(wallet)`. :+1:

💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1918760332)
The plot thickens - reference to lower version than minimum under *depends/*:
https://github.com/bitcoin/bitcoin/blob/df8bf657450d5383870d40790ea9f4fdb03c360d/depends/hosts/linux.mk#L44-L45

I agree with the author in https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589465760 that it's fine to limit the scope of the current PR.

📝 adlai opened a pull request: "doc: fix minor typos in comments"
(https://github.com/bitcoin/bitcoin/pull/31673)
In the unrelated PR #31621 the linter reported a few typos, that are fixed in this commit. I used the "doc" prefix as it only modifies comments, so none of the more significant prefixes seem appropriate.

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918779247)
Nah it's not a big deal i should probably find a way to run the linter stuff locally to prevent having to play ping pong like this.

💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2596067990)
The example holds also for the template: Externally they are exactly the same, so they could at most affect the function body, which is 2 or 3 lines. I just don't see what type of issue the change is trying to prevent, or what benefit it brings to developers or users.

(For reference, the commit that drops the unused casts (https://github.com/bitcoin/bitcoin/commit/f23d6a572d62ff640a28718ff0771c3f0f87f365) has already been merged.)

💬 maflcko commented on pull request "doc: fix minor typos in comments":
(https://github.com/bitcoin/bitcoin/pull/31673#issuecomment-2596072482)
lgtm ACK b30cc71e853cbaaec66e7e23d7d1a32468079ee0

💬 maflcko commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918808035)
> i should probably find a way to run the linter stuff locally

In theory it became a little bit easier after cmake, but it still requires a configure with `clang`. The you can possibly use the `clang-tidy-diff` approach explained in the dev notes. Though, for me it is rare enough to hit, that I am just playing the ping-pong. :sweat_smile:

📝 theuni opened a pull request: "init: Lock blocksdir in addition to datadir"
(https://github.com/bitcoin/bitcoin/pull/31674)
This probably should've been included in #12653 when `-blocksdir` was introduced. Credit TheCharlatan for noticing that it's missing.

This guards against 2 processes running with separate datadirs but the same blocksdir. I didn't add `walletdir` as I assume sqlite has us covered there.

It's not likely to happen currently, but may be more relevant in the future with applications using the kernel. Note that the kernel does not currently do any dir locking, but it should.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918811904)
Taken!

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918812960)
fixed, even better I clarified that it also included the block header and txs count.

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813151)
Done

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813433)
Done