📝 billymcbip opened a pull request: "test: Improve code coverage for pubkey checks"
(https://github.com/bitcoin/bitcoin/pull/34099)
Cover these branches in `IsCompressedOrUncompressedPubKey` and `IsCompressedPubKey`:
- `Non-canonical public key: invalid length for uncompressed key`
- `Non-canonical public key: invalid length for compressed key`
- `Non-canonical public key: invalid prefix for compressed key`

See the missed branches here: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html

`script_tests` succeed on my end.

💬 billymcbip commented on pull request "test: Improve code coverage for pubkey checks":
(https://github.com/bitcoin/bitcoin/pull/34058#issuecomment-3670079572)
@rkrux I created a new PR with the exact same changes: https://github.com/bitcoin/bitcoin/pull/34099. I hope the code coverage job will run correctly on that PR.

👍 rkrux approved a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3592744915)
lgtm, code review ACK 31c8ef3d9c885fbe810d5454ba1041ee87efa917

Nice, I missed reviewing the predecessor PR.

💬 Chand-ra commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670127663)
> The change is not about removing `.count()` everywhere. But to change it in cases where the `count()` statement is used to count the number of occurrences to indicate if something is in the collection. The example you reference eg `bool has_received{last_recv.count() != 0};` is a "correct" usage of `count()` (is it not empty)

Makes sense! I seem to have mistaken `count()` in `std::chrono` with the ones for container classes.

ACK [31c8ef3](https://github.com/bitcoin/bitcoin/pull/34095/com
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3670158639)
code review diff ACK 89372213048adf37a47427112a1ff836ee84c50e

Github diff seems broken - https://github.com/bitcoin/bitcoin/compare/662270f5b8d96434faec9e6f55f566f4f2ba1261..89372213048adf37a47427112a1ff836ee84c50e

`git fetch origin 89372213048adf37a47427112a1ff836ee84c50e && git range-diff 662270f5b8d96434faec9e6f55f566f4f2ba1261...89372213048adf37a47427112a1ff836ee84c50e` is also very verbose and confusing, so ended up checking out old branch, rebasing it locally and soft resetting it on
...

📝 anuragchvn-blip opened a pull request: "doc: Use multipath descriptors in descriptors.md and linked test"
(https://github.com/bitcoin/bitcoin/pull/34100)
Updates documentation and `wallet_miniscript_decaying_multisig_descriptor_psbt.py` to use single multipath descriptors with `<0;1>` syntax instead of separate external/internal descriptors.

## Changes
- **doc/descriptors.md**: Update examples (lines 70-71) to use `/<0;1>/*` multipath syntax
- **doc/descriptors.md**: Update Basic Multisig Example instructions (line 179) to use single multipath descriptor
- **test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py**: Refactor to us
...

💬 l0rinc commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670192777)
Thanks for checking @Chand-ra - your mentioned examples are not "contains" alternatives but "exists" alternatives for `chrono::duration` (which is orthogonal to this change and doesn't seem to exist in C++20 anyway).

> Looks like GitHub nuked all historic commits and historic CI runs here?

I did push a few versions while CI was running, maybe that was confusing - is there anything you need me to do here?

💬 rkrux commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2631043584)
Thanks for the suggestions, updated PR to accommodate both of them.

💬 maflcko commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3670243771)
Actually, parsing the conf file should clear the old/stale datadir value:

```
src/common/config.cpp- // If datadir is changed in .conf file:
src/common/config.cpp: ClearPathCache();
src/common/config.cpp- if (!CheckDataDirOption(*this)) {
src/common/config.cpp- error = strprintf("specified data directory \"%s\" does not exist.", GetArg("-datadir", ""));
src/common/config.cpp- return false;
src/common/config.cpp- }
```

Can you reproduce this on Linux?

💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670247073)
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master):
```bash
ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s

+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ loca
...

💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670255939)
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master):
```bash
ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s

+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ loca
...

💬 rkrux commented on pull request "test: Improve code coverage for pubkey checks":
(https://github.com/bitcoin/bitcoin/pull/34099#issuecomment-3670257467)
Nice, the coverage is available now here: https://corecheck.dev/bitcoin/bitcoin/pulls/34099.

I can see the PR description mentioned functions of the interpreter file has increased coverage! I don't know why the previous PR didn't have the coverage available.

⚠️ fanquake opened an issue: "build: build broken with older supported Boost"
(https://github.com/bitcoin/bitcoin/issues/34101)
Broken by #33192.

Debian Bookworm
GCC 12.2.0
Boost 1.74.0 (our minimum supported is currently 1.73.0)
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Debug
<snip>
/bitcoin/src/txrequest.cpp: In member function 'void TxRequestTracker::Impl::ReceivedInv(NodeId, const GenTxid&, bool, std::chrono::microseconds)':
/bitcoin/src/txrequest.cpp:583:35: error: 'boost::multi_index::multi_index_container<{anonymous}::Announcement, {anonymous}::Announcement_Indices>::index<{anonymous}::ByPeer>::type' {aka 'class
...

💬 fanquake commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3670275347)
This has broken building against our minimum supported versions of Boost: #34101.

📝 fanquake converted_to_draft a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095)
Replace the last few instances of `.count() != 0` and `.count() == 0` and `.count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* `boost::multi_index_container` (w)tx checks in `CTxMemPool::exists()`;
* other bare `std::unordered_set` and `std::map` count calls.

With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent f
...

💬 fanquake commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670288853)
Moved to draft for now, given these refactors have broken our minimum version requirements (#34101). That should be resolved before doing anything else here.

💬 hebasto commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670300267)
Is it a good time to consider bumping of the Boost minimum supported version?

💬 0xB10C commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2631122111)
done.

💬 0xB10C commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2631122670)
Dropped it.

💬 0xB10C commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#issuecomment-3670317642)
Addressed comments by mzumsande

💬 fanquake commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670335345)
Looks like it might have been added in Boost 1.78.0: https://github.com/boostorg/multi_index/issues/35.