💬 fjahr commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2748679517)
utACK 2906b183169bc78b37449a818717249c2d1cb7a1

Reviewed the code and changes look good to me.

💬 jsarenik commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2748705614)
Thank you for feedback, @maflcko ! In the end of this post I unclude shell script. The node is started always with `persistmempool=0` and I have also `mempool.dat` file symlinked to `/dev/null` just to remind me on a filesystem level.

Yes, my chainstate always contains the last-recent block when starting import.

Now I tried to artificially change priority of each of the transactions before importing them and it worked:

```
2025-03-24T16:06:18Z Imported mempool transactions from file: 40560 su
...

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

🤔 hodlinator reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2710922010)
Code review 8284229a28c09c585356dcf7e4bddbc8f2a23755

Thanks for restoring the former test!

Only semi-blocker for me is the consensus/policy question (see inline comment).

💬 hodlinator commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010515623)
nit: This might be more precise & helpful?
It took me a few minutes to re-grok the difference between the bytes derived from `XOnlyPubKey` vs compressed `CPubKey`.
```suggestion
// -> segwit version 1 with an non-standard program size
// (CPubKey::COMPRESSED_SIZE = 33 bytes in this test case)
```

💬 hodlinator commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010502733)
Would have been nice if `IsPayToAnchor` was `constexpr` and provably side-effect-free so that compilers could optimize the call away (#32100).

💬 hodlinator commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010480303)
Should this be:
```suggestion
// TxoutType::WITNESS_V0_{KEY,SCRIPT}HASH with incorrect program size (-> policy-invalid, i.e. non-standard)
```
?

Seems like the terminology could be out of sync with the comment below (mentioning "policy" instead of "consensus"). Or is it that segwit v0 program sizes are stricter and v1 program sizes are more flexible, consensus-wise?

💬 Kixunil commented on issue "Migrate from BTC/kvB to sat/vB on RPC and startup options":
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2748727277)
I don't like reducing the precision. The bitcoin price is rising, people will care more about smaller steps.

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