💬 maflcko commented on pull request "fuzz: add mempool_dag fuzzer for transaction dependency testing":
(https://github.com/bitcoin/bitcoin/pull/33038#discussion_r2223364583)
this is just copy-pasted from `src/test/fuzz/tx_pool.cpp`, same for the function above, with slight modifications.

it would be good to explain what exactly you are trying to test and why the existing fuzz targets are insufficient. Then, it would be good to provide coverage reports to show you have achieved your goal. Finally, it would be good to put this in the existing `src/test/fuzz/tx_pool.cpp` file, so that shared code can be re-used.

💬 achow101 commented on pull request "test: check tx is final when there is no locktime":
(https://github.com/bitcoin/bitcoin/pull/33030#issuecomment-3104091020)
ACK 065e42976a70738770af256da810ddc1316a4496

💬 l0rinc commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3104099469)
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2223414637)
Thanks, will do this on the next push

🚀 achow101 merged a pull request: "test: check tx is final when there is no locktime"
(https://github.com/bitcoin/bitcoin/pull/33030)

💬 achow101 commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3104201511)
ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948

The test isn't that slow for me, but this is still a significant improvement.

🚀 achow101 merged a pull request: "tests: speed up coins_tests by parallelizing"
(https://github.com/bitcoin/bitcoin/pull/32945)

👍 brunoerg approved a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3044342437)
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

`GetAddresses{Unsafe}` sounds good and it's better than simply just changing the documentation.

💬 maflcko commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-3104270978)
review ACK 5d82d92aff7c11ce17ee809c060e37f73a8a687a 🐀

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 5d82d92aff7c
...

💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2223486495)
Isn't it a stronger assertion to show the invariant holds for all cases? Otherwise what's the point of any of the fuzz assertions?

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2223488388)
I think you're right. I will make those more descriptive.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2223489610)
Thanks for pointing this out. I will fix this.

💬 ishaanam commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2223343037)
In bc9538823f52c7274d11ae28fa2b2f7236daa69c "relax child-with-unconfirmed-parents rule "

Maybe instead of just deleting the definition of `child-with-unconfirmed-parents`, a definition of `child-with-parents` could be added?

💬 ishaanam commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2223506244)
In c8d96e23f734e275dfee840df49b1f4cc78cd4b9 " test: add chained 1p1c propagation test "

nit: it is a bit confusing to reuse the `low_fee_parent` and `high_fee_child` variables here.

💬 ishaanam commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2223378883)
In bc9538823f52c7274d11ae28fa2b2f7236daa69c " relax child-with-unconfirmed-parents rule "
nit for precision:
```suggestion
// Since a submitted package must be child-with-parents (all of the transactions are a parent
```

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3104340209)
Yes @josibake, it's a slightly tricky problem. Rather than rush to a solution, I'd like to discuss it next meeting.

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223539466)
> > Write may throw, but Read does not
>
> Did that change in this PR?

Yes, you are adding a call to `Read()` in this line, which does not check the return code. In the previous master, there was just a simple `m_obfuscation = new_key;` (Stuff like this is why I prefer to keep the code as-is, when possible, unless there is a reason to change and no downsides :)

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480)
in the follow-up we don't have another `Read` anymore and `Write` has a constant `return true` (wanted to get rid of this in one of my PRs before)

👋 l0rinc's pull request is ready for review: "refactor,test: follow-ups to multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/33039)

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3104464694)
<ins>_Updates_</ins>:
- Addressed [feedback](https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3024249604) from @stickies-v:
- [Updated](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204543796) `getdescriptoractivity` test for required params;
- [Corrected](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204560989) variables naming within RPC `getdescriptoractivity`;
- [Updated](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208503121)
...