💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223289049)
Done!

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223297800)
I've re-tweaked the error descriptions to make them more generic and to don't depend on the name of the parameter specified in the RPC (otherwise we'd need to pass to the function the argument name defined in the RPC). Please let me know what you think.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223298338)
Done!

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223298762)
Done!

📝 l0rinc opened a pull request: "refactor,test: follow-ups to multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/33039)
Follow up for https://github.com/bitcoin/bitcoin/pull/31144
Applied the remaining comments in separate commits - except for the last one where I could group them.
Please see the commit messages for more context.

🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3043917032)
Added a follow-up with the remaining nits, see: https://github.com/bitcoin/bitcoin/pull/33039

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223278879)
Done, together with the bit_cast -> reinterpret_cast change and comment renames, see [`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_r2223178526)
I'm usually a fan of small and focused tests that complement each other - instead of end-to-end ones that test everything. But I don't mind merging these either, done something similar in a separate follow-up PR: [`a17d820` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/a17d8202c36abf8a17fb8736e05f318422a3c7fb)

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223255423)
Added it to the test suite: [`2dea045` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/2dea0454254180d79464dc6afd3312b1caf369a7)

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223239857)
Sure, done in a separate PR: [`e5b1b7c` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/e5b1b7c5577ee36b5bcfb6c02b92da88455411e9)

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223234425)
Done in a separate PR, thanks: [`298bf95` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/298bf9510578263a1439513729e5ff955a453437)

💬 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