💬 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_r1747339084)
Probably fine here, but IIRC it is UB to cast away const from something that is always const.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1747342326)
Yes, but it will be more code and complexity, so I'll leave this as-is for now.

💬 sipa commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747344345)
I believe casting away constness on itself is never UB, but *modifying* a const value (through a pointer/reference whose constness was cast away) is UB.

🤔 tdb3 reviewed a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2286432258)
Approach ACK

Thanks for the useful RPC. Love that for some use cases it enables the option to avoid ancillary software (e.g. electrum server).

Left some initial comments for now (mainly for tests). Planning to circle back.

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1747223007)
Rather than generate 101 new blocks, maybe the test could take advantage of the pre-mined test framework chain here (since `setup_clean_chain` defaults to `False`, with 200 blocks generated)?

From test_framework/wallet.py:
```
# When the pre-mined test framework chain is used, it contains coinbase
# outputs to the MiniWallet's default address in blocks 76-100
```

Probably not an exciting speed increase, but every bit counts.

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1747235621)
nit: `0000...` is astronomically unlikely to hit, but not sure off the top of my head if it is invalid. Maybe could use `invalid_blockhash = "ffff..."` instead, which seems appears to be above the powLimit for regtest.

https://github.com/bitcoin/bitcoin/blob/bbf95c0cc57147827b9f4577c641b12dd4170e78/src/kernel/chainparams.cpp#L541

I could be mistaken.

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1747224502)
Could use `assert_raises_rpc_error` here instead of the try/except.
Something similar to:
```python
assert_raises_rpc_error(-5, "Block not found", node.getdescriptoractivity, [invalid_blockhash], [f"addr({addr_1})"], True)
```

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

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