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

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