💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1918284288)
b70e83da4210e8098cbf3fc3b2289fc710828455: let's echo the specified sighash value, otherwise the error is confusing in this default case.

We should probably also echo what's in the PSBT.

I think it's a good safety feature that the default behaviour is _not_ to just mimic what's in the PSBT but to insist on `DEFAULT` / `ALL`. The error message could warn against the temptation to call the RPC again with the "right" value.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1918377236)
e94bb4e5fc6dea67844a8495b948b679649b7b65: maybe do this inside the `psbtx.tx->vin` loop below and set `SIGHASH_ALL` or `SIGHASH_DEFAULT` depending on whether it's taproot.

And then replace the magic buried in `MutableTransactionSignatureCreator::CreateSig` with an `assert`.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1918368175)
e94bb4e5fc6dea67844a8495b948b679649b7b65: maybe use `SIGHASH_ALL` here, since the legacy wallet doesn't support Taproot.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1918345572)
0970178f620acd695c49526d2b375eae489072c1: can we make this `nodiscard` and then explicitly ignore the result in `rpc/rawtransaction.cpp`?

💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2595488085)
> const

`uint8_t` is just an example. It can be applied to `const` as well. (There is an argument to apply const to references where it makes sense, because it is part of the interface there and "spreads". Though for objects, it is mostly style). Historically, I don't think a change has ever been done and merged to add `const` to an object. Also, the style guide doesn't even mention it (one way or the other).

💬 kevkevinpal commented on pull request "Update leveldb subtree to latest upstream":
(https://github.com/bitcoin/bitcoin/pull/31671#issuecomment-2595502071)
Concept ACK [910a11f](https://github.com/bitcoin/bitcoin/pull/31671/commits/910a11fa66305f90b0f3a8aa9d2055b58a2d8d80)

I think it makes sense to update to the leveldb latest upstream, I also agree with @l0rinc there might be some other changes that might be worth considering.

---

I also just validated that this change only includes these 3 changes
https://github.com/bitcoin-core/leveldb-subtree/pull/40,
https://github.com/bitcoin-core/leveldb-subtree/pull/45,
https://github.com/bit
...

💬 Sjors commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1918446438)
Done

💬 Sjors commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#discussion_r1918446669)
Done

💬 Sjors commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2595527953)
Rebased, picked a different `future` timestamp to better distinguish the griefing attack test case from the boundary value check.

I also renamed `t` to `wall_time` for clarity.

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