👍 ryanofsky approved a pull request: "refactor: Make node_id a const& in RemoveBlockRequest"
(https://github.com/bitcoin/bitcoin/pull/31282#pullrequestreview-2711167793)
Code review ACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc. Code changes all look good but I'm a little confused about purpose of the third commit, so left a question about that

💬 ryanofsky commented on pull request "refactor: Make node_id a const& in RemoveBlockRequest":
(https://github.com/bitcoin/bitcoin/pull/31282#discussion_r2010629486)
In commit "ci: Use G++ in valgrind tasks" (fa21f83d2983d97006ec1e3c47634dc0fe0349dc)

I think it would be helpful if this commit message give some hint about why this change is being made. Currently no reason is given and even looking at this PR and understanding a little bit about the bug and trying to read discussion at https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-2047861599 I don't think I get it. If "Currently, valgrind is not usable on a default build with GCC" according t
...

💬 ryanofsky commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2748922698)
re: https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2748719049

@hebasto thanks for the review! I can try to implement your suggested changes but since they are all about code style, maybe it would be faster if you could just post a diff with your preferred change and I can apply it? I could try to figure out how to integrate your suggestions, but since they go against my own preferences for having fewer variables and not repeating code I think it would be harder for me to figure ou
...

🚀 ryanofsky merged a pull request: "wallet, rpc: deprecate settxfee and paytxfee"
(https://github.com/bitcoin/bitcoin/pull/31278)

💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010662585)
Mmm, I think I'm still missing something. Pushed a commit 43e2d9792bae4894563625f8ae86c3e504d44004 that robs the vault by simply changing the destination address and witness at withdrawal time.

💬 kevkevinpal commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2748955320)
> > why not just modify `initialize_package_rbf` to use this statement
>
> Why would that be better? Once an off-by-two is added in a future patch, the fuzz target once again will crash, whereas with the patch in this pull, it will work fine?

ahh ok I see now that makes sense to me

🤔 jonatack reviewed a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2711259714)
Concept ACK.

Part of the commit message of fa41685f438bba00761423d464bbb8dc5ea7ddf6 seems incorrect (maybe was pasted from fa3fb3c23fae287b161112b9b04cf0937a1051c6 without updating, not sure).

💬 jonatack commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2010672448)
fa41685f438bba00761 For the 3 conditionals dropped in this commit, any reason to keep the scope braces?

💬 jonatack commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2010685447)
For all `GetArg` calls in this commit. Perhaps I am confused but should be const ref? Would also be more clear to continue to specify the type here.

```suggestion
if (const std::string& port{args.GetArg(port_option)}) {
```

💬 jonatack commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r2010690560)
FWIW these two lines are from daec955fd68b (https://github.com/bitcoin/bitcoin/pull/9380)

💬 marcofleon commented on pull request "refactor: Enforces Txid and Wtxid types in RelayTransaction":
(https://github.com/bitcoin/bitcoin/pull/32104#issuecomment-2749104911)
Lgtm as a standalone change. It's the same change I implement in fa8400494e3c411a55375e71abaaa2637a22e216 as part of the complete refactor. I actually missed all the `tx.GetHash()` and `tx.GetWitnessHash()` in `ProcessMessage` so good catch there.

My [final refactor](https://github.com/bitcoin/bitcoin/compare/master...marcofleon:bitcoin:workingtxidtypesafety) does end up altering `RelayTransaction` in a more involved way, but that includes changes (switching `GenTxid` to a variant) that haven
...

💬 johnnyasantoss commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-2749116445)
We've experienced this bug, using a clean ubuntu installation. We had a transaction scanner running for some transactions on a new block, but by a mistake we ran the scanner on all txs on the block. Ideally core should never crash but start rejecting requests and stop enqueuing work on the rpc queue.

```
2025-03-20T18:46:57Z [libevent:warning] Error from accept() call: Too many open files
2025-03-20T18:46:57Z [libevent:warning] Error from accept() call: Too many open files
2025-03-20T18:46:57Z
...

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2010820383)
This file has some strange formatting that does not show up in Github. My editor (Vim) just shows a character that seems like a translation error.

> 5 sat

I opened the file with `xxd` and the translation is `35e2 80af 7361 742f`. `35` is the hex for `5` in ascii, and `73` is the hex for `s`. It looks like `80` is extended hex for the euro sign and `e2` and `af` also have extended hex meanings. There are more of these weird encodings thought the file.

💬 mzumsande commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#discussion_r2010827029)
updated, thanks!

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2010835264)
That seems like a good idea, but that’s out of scope for this PR. I have had a refactor of BnB on my todo list for a long time, and it would be part of that, probably.

💬 mzumsande commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#issuecomment-2749245050)
[4a76a83](https://github.com/bitcoin/bitcoin/commit/4a76a8300f295e8eab0ea8ab1ea1580450f131c8) to [adcbb2a](https://github.com/bitcoin/bitcoin/commit/adcbb2a32e60aa49faff0c41033adcc149336220):

- Rebased onto master because the CI failed, as far as I can see unrelated to the PR.
- Added test by @stratospher, thanks! (with minor changes, no need to re-format `block.sha256`, can just use `block.hash`).
- Added a commit that replaces `IsValid()` by a comparison with `BLOCK_FAILED_MASK`. `IsValid
...

🤔 sipa reviewed a pull request: "contrib: document asmap-tool commands more thoroughly"
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2711540347)
ACK fa52e606de7a12921619b56a631cd194e2366cc1

💬 1440000bytes commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#issuecomment-2749251585)
Concept ACK

> I don't think we should tell users to just google for utilities like this. Nor should we link to some project that we're not reviewing. And if we don't tell users where it is, then only people who've heard of rust-bitcoin will use it. I wouldn't consider that a fix of #19151.

Agree. This could be a useful tool for bitcoin core users.

🤔 ryanofsky reviewed a pull request: "[IBD] batch block reads/writes during `AutoFile` serialization"
(https://github.com/bitcoin/bitcoin/pull/31551#pullrequestreview-2711550079)
Code review 2c073b90ae977af1ac01164a894edd978529fdc6. I think the new BufferedFile approach implemented is reasonable and a little nicer than the [previous](https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2722752125) approach which made lower-level calls in block storage code to allocate and pass buffers. But the need to hardcode and compute sizes in 8782d6ed09bbb947426c1ed54ad4d5188326764d and 2c073b90ae977af1ac01164a894edd978529fdc6 still seems to make the storage code more awkward
...

🚀 ryanofsky merged a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656)