💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223272055)
I don't mind writing an `Obfuscation` object directly, it does make more sense to make that symmetric with the read.
And if you still don't like reading back what we wrote (which would exercise the same route that we'd take otherwise, seems better to me), we can also just assign it directly.
Done in [`62aa7b9` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/62aa7b9fabf03872fe0905076b6d0b275747d188)

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223281010)
Done in [`fee3048` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/fee3048fcc4972ba02127a2a4ef75524c5be275d)

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223283857)
Done in [`fee3048` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/fee3048fcc4972ba02127a2a4ef75524c5be275d)

👍 l0rinc approved a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3044142789)
ACK 89b5b607e3380ff2cf03d8199c70e655e8c265cb

Consider unifying the way your refer to the first block that fulfills the assumevalid condition

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2223351892)
Commit message is inconsistent with comments: `assumedvalid block` is written in your code as `assumed valid block`. I would be fine with `assumevalid block` as well, as long as they're consistent.

nit: sentence ends in `full block yet` - we could add a fullstop.

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2223343368)
To be consistent with previous line:
```suggestion
// if the previous IBD run was interrupted before it downloaded the assumed valid block.
```

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2223342500)
```suggestion
// If no blocks were imported, ActivateBestChain will have nothing to do
```

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