💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1747300351)
can we reduce the duplication between line 39 and this set of values?

💬 achow101 commented on pull request "test: Work around boost compilation error":
(https://github.com/bitcoin/bitcoin/pull/30834#issuecomment-2334311818)
Backported fa9d7d5d205ada8915cbbc29599ab8e7bf1fffe0 in #30827

💬 achow101 commented on pull request "[28.x] Further backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/30827#discussion_r1747314219)
Backported fa9d7d5d205ada8915cbbc29599ab8e7bf1fffe0

💬 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_r1747327065)
I have [added](https://github.com/bitcoin/bitcoin/compare/ab875e14b7ed02e6e4bdc607d67e28bc10216a27..14080bb53d649ed1c99f121f377d2c8f2019c99e) an `std::byte` overload in a separate commit, so now we have:
```C++
CScript& operator<<(const std::span<const std::byte> b) LIFETIMEBOUND
{
return *this << std::bit_cast<std::span<uint8_t>>(b);
}
```
which delegates to:
```C++
CScript& operator<<(const std::span<const unsigned char> b) LIFETIMEBOUND
{
AppendDataSize(b.size());
Appe
...

💬 hebasto commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#issuecomment-2334333003)
766881e33d13ca2887f7ed179cdcc6bc007a6325

While installing `libbitcoinkernel.pc`, why ignore CMake's configs for downstream projects using CMake?

💬 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}) {

...