👍 rkrux approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450922162)
tACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2

Successful make, unit and functional tests. Definitely a value-add, I like how the problem a user faces has been described in the PR description, and then how this feature solves it.

I reviewed the range diff along with reviewing again the PR as whole.
```
git range-diff f383db7...878b6c8
```

I tested it on mainnet on a quite active address `bc1qryhgpmfv03qjhhp2dj8nw8g4ewg08jzmgy3cyx` for blocks `870930` and `870931`. Sharing an observati
...

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851782089)
Nesting `outspk` worked out well, I was not a fan of the special check for `desc` in the loop above earlier but at that moment seemed unavoidable.

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851787388)
Nit: Although only one usage right now, but making `201` dependent on `200` above in the setup seems more forward facing: https://github.com/bitcoin/bitcoin/pull/30708/files#diff-dfe8598636b25e8bef111dc785f4cfc330e936191d9adb95b66add813a926839R23.

```python
GENERATED_BLOCKS = 200
...
...
'height': GENERATED_BLOCKS + 1,
```

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851877237)
Thinking out loud, not necessary to be a change atm: The crux of these 4 lines is to return a `CBlock` given `chainman.m_blockman, *blockindex` . `block_data` and `block_stream` are not used beyond these 4 lines and seem internal to this intent, a sign these can be encapsulated in a function if we anticipate this pattern to be replicated.

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851772090)
Can also test here for duplicated addresses at the end of `test_multiple_addresses`.

```python
# Duplicated descriptors do not return duplicated results
result = node.getdescriptoractivity([blockhash], [f"addr({addr_1})", f"addr({addr_1})"], True)
assert_equal(len(result['activity']), 1)
```

💬 l0rinc commented on pull request "Make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#issuecomment-2490989399)
ACK 2fff10a656edb8bec7d45433bf431fd233d0fd81

💬 l0rinc commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2490998686)
Having no fully end-to-end tests sounds a bit dangerous, since it's part of the fundamental behavior of Bitcoin, not sure we should completely mock that away (i.e. certain bugs could creep in that work locally only - no idea how that would be possible, but that's exactly the point).

💬 fanquake commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491001441)
Which tests are you seeing this from? Neither the unit nor functional tests should be making outside connections. This has certainly been discussed/avoided previously so may be a recent regression.

💬 vasild commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491003878)
I have the idea to log non local traffic with `iptables` and fail the CI. But I do not understand why nothing is logged - I added some quick and dirty commands to `ci/test/03_test_script.sh`:

```sh
iptables -A OUTPUT -m addrtype \! --dst-type LOCAL -j LOG --log-prefix "eeeee1" --log-level emerg
iptables -A OUTPUT -j LOG --log-prefix "eeeee2" --log-level emerg
... generate some traffic with telnet and ping ...
# confirm some packets matched:
iptables -v -x -n -L
...
2024-11-21T11:38:4
...

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851965340)
Good point. Just tested that that's indeed the case. I'll mention that in the commit message.

cc @hebasto

🤔 hebasto reviewed a pull request: "build: Enable -Wbidi-chars=any"
(https://github.com/bitcoin/bitcoin/pull/31315#pullrequestreview-2451227488)
Post-merge ACK fa7857ccda5d82739bba16eeb7244433978d28f3. This compiler option is recommended in the [Compiler Options Hardening Guide for C and C++](https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html).

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2491006682)
309bd56d97f87f973f45897fc00b1bd2fc5cff1a -> 563278b6a125f1a3d2111d4ebd74b9cc16d83ec5 ([submitblock_prechecks_2](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_2) -> [submitblock_prechecks_3](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/submitblock_prechecks_2..submitblock_prechecks_3))


* Added release notes documenting the submitblock change w.rt. duplicate block handling and pruning.

💬 vasild commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491017207)
@l0rinc the current end to end tests do communicate locally via the `lo` interface.

@fanquake Honestly, I forgot which test was that. During latest coredev I investigated that briefly with @maflcko or @0xB10C (?) and we observed the non local traffic with `tcpdump(1)`. Now, my point is to enforce this in the CI, even if such test does not exist currently - has been fixed since coredev or my investigation back then was wrong.

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1851973626)
I decided to add a release note for now. I think the changes in this PR are small enough that we can hash the changes out wholesale and it is also not a particularly pressing change. Still not wholly convinced, but thinking about it some more I don't think somebody was relying on this behaviour beforehand either.

💬 maflcko commented on pull request "refactor: Prepare compile-time check of bilingual format strings":
(https://github.com/bitcoin/bitcoin/pull/31295#discussion_r1851976250)
It shouldn't reproduce. This commit is just taken from the other pulls, so that clang-tidy is happy with them.

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851979781)
Added

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2491024996)
Rebased, added comment and expanded commit message to note the GUI improvement.

💬 maflcko commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491034618)
I think it makes sense to enforce this in CI (and have a tracking issue or pull for that). Otherwise, it is easy to regress, like in https://github.com/bitcoin/bitcoin/pull/30529#issuecomment-2252854223

However, if there is a violation of this, or a bug, it would be good to add exact steps to reproduce, or an explanation. Claiming that this is broken on "Ubuntu 28.04 LTS" doesn't seem helpful to uncover the bug, if there is one.

Related: https://github.com/bitcoin/bitcoin/issues/30030

💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1851990186)
Actually, fixed. You can drop this now.

👍 theStack approved a pull request: "test: Deduplicate assert_mempool_contents()"
(https://github.com/bitcoin/bitcoin/pull/31338#pullrequestreview-2451281918)
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638

Thanks for following up!