💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1747277470)
This would be a great test to include. Seeing `test_framework.authproxy.JSONRPCException: mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)` when uncommented.

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1747291150)
```diff
- assert result['activity'][0]['value'] == 1.0
+ assert result['activity'][0]['amount'] == 1.0
```

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1747344172)
Could use `test_framework/messages.py:tx_from_hex()` here, e.g.:

```diff
- signed = CTransaction()
- signed.deserialize(BytesIO(bytes.fromhex(rawtx_2)))
+ signed = tx_from_hex(rawtx_2)
```

🤔 stickies-v reviewed a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2286282737)
Force-pushed to [add back in some previously removed unit tests](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745868679) to help make this PR easier to review, and [added an extra conversion test](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746191005). Test-only changes, so hopefully pretty straightforward to re-review. Thanks, everyone!

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1747332332)
Okay, I've re-added the "string constructor" tests. I think they can be removed later, but that's obviously not critical either. Thanks for explaining your concerns.

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1747133662)
I don't think `BOOST_CHECK_EQUAL(arith_uint256{num}, num);` is a `conversion` test, so I'm going to leave that out here, but I like the `BOOST_CHECK_EQUAL(UintToArith256(uint256{num}), num);` suggestion, thanks!

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1747350282)
> but I'm missing simple unit tests that can document the behavior of the method in a trivial way

If we assume that `GetHex()` is a complete representation of each class' internal state, then feel like it's reasonable (even if suboptimal) to rely on using that method do assess the correctness of the conversion function, if `GetHex()` is well unit tested. Also note that we have tests that don't rely on `GetHex()` / `FromHex()` at all, i.e.:

```
for (uint8_t num : {0, 1, 0xff}) {

...

💬 maflcko commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747371536)
Ah yes, according to https://eel.is/c++draft/expr.const.cast#6

I still have a slight preference to keep the const unless removing it has a clear benefit.

💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747373351)
Very good points, in this version I can indeed add back the `const`, thanks!
Also added a simple test to make sure `CScript() << _hex_v_u8` and `CScript() << _hex` produce the same `CScript()` (can be removed if it passes CI and is deemed superfluous)
See: https://github.com/bitcoin/bitcoin/compare/14080bb53d649ed1c99f121f377d2c8f2019c99e..03bdefff8c6663da895d0603469dee15aa4ad35d

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1747378139)
Agree, in C++23 we can just use `.contains` which would simplify this slightly

🤔 hebasto reviewed a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2286670859)
Approach ACK 766881e33d13ca2887f7ed179cdcc6bc007a6325.

💬 hebasto commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747381222)
style nit: All other CMake code uses the `if()` command without an additional space.

💬 maflcko commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747385054)
The last push looks fine, but I still have a slight preference to use UCharSpanCast or std::as_bytes, which preserve constness by default.

💬 hebasto commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747393816)
```suggestion
set(LIBS_PRIVATE "")
```
if there is no intention to let the user set it as a cache variable.

(same for `all_target_static_link_libs` a few lines above)

Why this local variable name capitalized?

💬 hebasto commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747397241)
style: A shorter alternative:
```suggestion
string(APPEND LIBS_PRIVATE " -l${lib}")
```

👍 hebasto approved a pull request: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827#pullrequestreview-2286729300)
ACK 37a6739203ea3da394acef38b18a794e67543a90.

💬 luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1747435819)
These are identical now. `wine_runtime` seems no longer needed at all?

⚠️ jlest01 opened an issue: "Misleading index error message"
(https://github.com/bitcoin/bitcoin/issues/30836)
When running node with the `txindex` parameter for the first time, we see the following message in the log:
```ERROR: Commit: Failed to commit latest txindex state```

This misleads the user into believing that something is wrong, but the indexing process normally begins shortly thereafter.
It appears that the reason for this message is that there is no index yet. Since `src/index/base.h` is common to all indexes, this message may appear in all indexes.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1747533292)
Hmm there are a lot of entry points to `FlushStateToDisk`, and many places where `SetBestBlock` is called. I think this PR should leave this as is to not complicate review.

I was thinking we could remove the `full_flush_completed` variable if we didn't have this check.

📝 ariard opened a pull request: "Add NODE_TXRELAY_V2."
(https://github.com/bitcoin/bitcoin/pull/30837)
This is the top commit from #30572, where the new node service bit support commit is extracted on its own.

See the corresponding BIP draft for motivation: https://github.com/bitcoin/bips/pull/1663

Dissociating this change from halting the processing of unrequested transaction, allow the node service bit support to be used for further policies and mechanisms, beyond this mechanism only.