👍 ryanofsky approved a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2711009132)
Code review ACK fa0cf040baa74b161b4b00ddea95bfb5aac0bb53. Only change since last review is removing a nearby todo comment that was vague

👍 ryanofsky approved a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2711035936)
Code review ACK 226d81f8b708ea1c3a39ec58446e291fe7440fdd. New test was added since last review, which seems very cleanly written and fixes some missing coverage.

💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010554425)
I tried to be smart and also use `-1` instead of `cold_pk` in `recover_leaf`, but that doesn't work. Not sure why.

💬 jurraca commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2010558964)
I mean specifically in the case where many ranges are unmapped by accident, not just ranges we expect to be unmapped. For example, if your ASmap was generated incorrectly and only has half the ranges it should, then `--fill` would be reassigning many ranges to an incorrect AS, right?

💬 jurraca commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2010561791)
yea i'll revert this change, don't think it's relevant here after all.

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2010561981)
Well, yes, but then the asmap file would already be incorrect before filling too.

🤔 jonatack reviewed a pull request: "doc: clarify the documentation of `Assume` assertion"
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2711113250)
ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed

💬 jonatack commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010603277)
Perhaps keep the "In production" at the beginning of the sentence.

```
In production builds, however, if the compiler can prove that
an expression inside `Assume` is side-effect-free, it may optimize the call away,
skipping its evaluation.
```

💬 jurraca commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2010610678)
the impact on the node will be different though, if a peer is unmapped, it will just be another peer to use, if it is mapped incorrectly, it may be rejected when it was fine to connect to. I'd have to run tests to get an idea of impact, but mostly it seems like a potential "footgun" for users.

How about [fa52e60](https://github.com/bitcoin/bitcoin/pull/32110/commits/fa52e606de7a12921619b56a631cd194e2366cc1) ?

💬 hebasto commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#discussion_r2010614869)
I found this code hard to read. Wouldn't it better:
```suggestion
if(SANITIZERS MATCHES "(^|,)fuzzer($|,)")
```
?

style nit: The surrounding code has no space between `if` and `(`.

💬 hebasto commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#discussion_r2010617246)
Maybe avoid reusing the `SANITIZE_FLAG` variable?

style-nit: The surrounding code mostly uses `ALL_CAPS` naming for user-faced variables, not for internal ones.

💬 mzumsande commented on issue "Add a `indexesdir` option to hold the indexes directory":
(https://github.com/bitcoin/bitcoin/issues/32099#issuecomment-2748876625)
With the current size of indexes this looks like a reasonable option to me - even if symlinks work too.

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