💬 l0rinc commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2595534932)
> Though for objects, it is mostly style

It's not purely a stylistic change - it clarifies which parameter remains unchanged (i.e., it isn’t a "return value").

It seems there’s a strong preference against this change, so I don't mind adjusting the approach or closing the PR altogether.

👋 alfonsoromanz's pull request is ready for review: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455)

💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2595610458)
If there is a good reason to use `const` for an object (like it being exposed in a larger scope, or it preventing a bug that would otherwise be uncaught), it can be used. However, the change here affects a scope that encompasses at most the next line (or the line after the next line), so it couldn't be smaller. Also, it is hard to see what kind of bug this would be preventing. Either the line of code is correct (with respect to const), or if it wasn't, the existing tests wouldn't be passing anyw
...

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2595615741)
Pushed b1affc74c6c0cd65799e065da5744b02761ef738 to address the latest feedback provided by @fjahr. Also updated the docs as mentioned here https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1899762694

👍 hodlinator approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2555213032)
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc

- Excellent with added fuzz test.
- Added support for shifting negative numbers.
- Adopting `size_t` earlier for some cases, reducing churn.

Succeeded building on 32-bit (CentOS).

---

Nit: commit message 65cde3621dbb9ac7d210d4926e7601c4adf5f498

```diff
- kernel: Move default cache constants to caches
+ kernel: Move default cache constants to caches.h
```

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1918077532)
nit: Naked `else` without braces is contrary to developer-notes.md:

https://github.com/bitcoin/bitcoin/blob/98939ce7b7441cc95b8fda4d502f182aab1d6a32/doc/developer-notes.md#L76-L79

Consider using ternary operator:
```suggestion
auto check = [](W value) -> std::optional<W> { return (value >= min && value <= max) ? std::optional{value} : std::nullopt; };
```

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1918159934)
In thread 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 doesn't tolerate it somehow (bu
...

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1918258200)
nit in commit 8826cae285490439dc1f19b25fa70b2b9e62dfe8:
(We're assigning what seems to be an `int64_t` to `size_t filter_index` here. I tried adding curlies on the right side of the assignment and compiling for 32-bit but was unable to trigger narrowing errors. Maybe the compiler is clever enough to see that `(MAX_FILTER_INDEX_CACHE << 20) < std::numeric_limits<size_t>::max()`).

💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1918034685)
mega-nit in case you re-touch:
```suggestion
// Avoid undefined C++ behaviour if shift is >= number of bits in T.
```

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918532671)
Yes, failing `Bind` when the network address fails to set makes more sense.

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918533974)
Whoops, so clear where i copied the code from now 😄

💬 maflcko commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2595671323)
Yeah, I suggested this for reasons other than the CI fix, because it could be useful outside the CI as well. But no strong opinion.

💬 Detroitin commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2595678247)
> > Anyone have an asic to reorg that?
>
> MARA hash on the way! Luxor also said they are willing to help as well. Shout out to Nick to even turning me onto this problem.

I have/had some pointed, probably 120TH and mempool displayed 20-25TH/s so when I see the hashrate that is being displayed on mempool. space, THERE IS ALOT! WELL OVER 1 PH/S. and MARA you are the only other miner who has a pool with statistics showing?? what ive seen so far is this guy I grabbing his nuts flipping us off.
...

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918551760)
For sake of completeness. i think it's uglier to fall through to the parent class, which would do an actual `getsockopt` call on an invalid socket if in the future the called code would use `GetSockOpt`.

i kind of wish there was a purely virtual `BaseSock` and `PosixSock` and `TestSock` etc were a subclass of this, so every subclass would be forced to implement everything.

💬 maflcko commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2595683931)
This needs a test and the tests fixed

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918560208)
Done in followup, and also think we can `EraseTx` here. See #31666

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918567089)
Yes. It makes no sense, it doesn't even use `m_socket`.
i copied this destructor from `StaticContentsSock`, which i think also does it redundantly.

💬 fanquake commented on pull request "depends: Qt 5.15.16":
(https://github.com/bitcoin/bitcoin/pull/30774#issuecomment-2595719301)
Guix build (x86_64):
```bash
e80422975315244b3365bf9eddb9e6e5d9da08f4b1a5fba34430c6f3021c6b8a guix-build-31a0e5f0905b/output/aarch64-linux-gnu/SHA256SUMS.part
db53cd0057bf0048534e3daea2e39130cdf64a140becaa5c6ff4f98fdfa5936a guix-build-31a0e5f0905b/output/aarch64-linux-gnu/bitcoin-31a0e5f0905b-aarch64-linux-gnu-debug.tar.gz
fa061afbd1dac25c739a63a1644f2af1ad5dab36ea5e4245a7b0bb9008ca4f79 guix-build-31a0e5f0905b/output/aarch64-linux-gnu/bitcoin-31a0e5f0905b-aarch64-linux-gnu.tar.gz
f286a74
...

🚀 fanquake merged a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397)

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918601053)
Ok, will just make it do nothing and return -1. That's just as valid, no need fo the memset.