💬 stickies-v commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918312609)
Ah yes, that's true, `test_runner` itself shouldn't even work in that case.

💬 fanquake commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2595310187)
> if we start using syntax features and behavior that are only available in versions greater than the minimum?

This should never be the case, otherwise the minimum required version isn't actually the minimum required version.

🤔 stickies-v reviewed a pull request: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2555656322)
Approach ACK c0127439597ee9d09b8e117be7d6226a68b45721

I'm not familiar enough with symlinks and build toolchains to have an opinion on c0127439597ee9d09b8e117be7d6226a68b45721, but it seems reasonable.

💬 stickies-v commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918282849)
Making this check conditional on platform is undocumented in the commit message, and seems orthogonal to the goal of testing errors individually. I think it would be best to split this out in a separate commit and document it?

💬 l0rinc commented on pull request "Update leveldb subtree to latest upstream":
(https://github.com/bitcoin/bitcoin/pull/31671#issuecomment-2595315830)
Could you share more about how these changes were selected. I noticed there have been many (useless) refactorings in LevelDB since the last stable version we’re using. While I completely agree that cherry-picking every update may not align with our goals (especially for many of the high-risk, low-reward refactors), a few targeted updates addressing critical issues might still be worth considering:
* [fb644cb](https://github.com/google/leveldb/commit/fb644cb44539925a7f444b1b0314f402a456c5f4): Fi
...

💬 tdb3 commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2595341267)
> I didn't know about #30941. Let me know if there's anything there that you'd like me to add here. After that I'll address the other feedback, in particular I should probably pick a higher number for `future` to better distinguish the wall clock scenario from the boundary condition check.

Since the test approach has changed, I'm not seeing more to add.

💬 fanquake commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918336563)
In c0127439597ee9d09b8e117be7d6226a68b45721:

> because it depends on the build toolchain:

Even if this is the case, is the best option to just to do nothing? Shouldn't we atleast test the known behaviour for our release build, so we'd catch if this changes (is fixed?) in future? Otherwise my understand is if this changes again, it's just going to be hidden/ignored.

💬 maflcko commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918354618)
I guess the check could be limited to exclude the msvc build?

🚀 fanquake merged a pull request: "refactor: Avoid UB in SHA3_256::Write"
(https://github.com/bitcoin/bitcoin/pull/31655)

🤔 Sjors reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2555658830)
Concept ACK

The `descriptorprocesspsbt` documentation for `sighashtype` currently says:

> The signature hash type to sign with if not specified by the PSBT.

Let's change that to:

> The signature hash type to sign with. If the PSBT already specifies a hash type, it must match. Otherwise it is added to the PSBT.

Some comments based on partial code review.

💬 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_r1918339465)
b70e83da4210e8098cbf3fc3b2289fc710828455: in case of a hardware wallet (external signer) I would want this to fail before any call to the device is made.

It's a bit hard to tell if that's the case, maybe this specific check can be refactored into its own function? That would make things easier to follow in general.

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