💬 willcl-ark commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2672463328)
Closing for now until I can circle back.

💬 maflcko commented on pull request "ci: run in worktrees":
(https://github.com/bitcoin/bitcoin/pull/31787#issuecomment-2672476623)
For reference, I can't review this, because I don't know if all the bash refactors/behavior changes here are correct and best practise, or if they could fail (and when) on error or on ancient versions of bash used in macOS.

💬 achow101 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#issuecomment-2672488147)
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02

💬 i-am-yuvi commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1964227405)
It might be helpful to consider giving a `Concept ACK` or `NACK` before the review. I came across this idea [here](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review).

💬 fanquake commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2672502510)
@furszy you might want to review here?

💬 maflcko commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#issuecomment-2672504845)
lgtm ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02

🚀 achow101 merged a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302)

💬 darosior commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1964234048)
You are right, i missed how the genesis block's `nBits` does not correspond to the `powLimit` of `7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff` but instead encodes a rounded down target of `7fffff0000000000000000000000000000000000000000000000000000000000`. This means that all values from `7fffff0000000000000000000000000000000000000000000000000000000001` to `7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff` would have the highest bit unset but also fail in the
...

💬 fanquake commented on pull request "test: fix TestShell initialization and reset()":
(https://github.com/bitcoin/bitcoin/pull/31415#issuecomment-2672524118)
@willcl-ark want to take a look here?

✅ fanquake closed a pull request: "refactor: convert ContainsNoNUL to ContainsNUL"
(https://github.com/bitcoin/bitcoin/pull/31301)

💬 fanquake commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#issuecomment-2672526840)
Closing for now.

💬 achow101 commented on pull request "contrib: fix BUILDDIR in gen-bitcoin-conf script and gen-manpages.py":
(https://github.com/bitcoin/bitcoin/pull/31742#issuecomment-2672527071)
ACK 63a8791e15c3ffb44b84ab3e85db62d7997d25fd

🚀 achow101 merged a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script and gen-manpages.py"
(https://github.com/bitcoin/bitcoin/pull/31742)

💬 ajtowns commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2672536510)
> Concept NACK. This introduces dead code and i don't think the justification for this meets the bar for touching consensus code.

I think it would make more sense to carry this patch as part of the 64 bit arithmetic patchset. If there's consensus to merge/activate 64 bit arithmetic, it could make sense at that point to split this into its own PR to make review easier, but without that motivation, I don't think this change makes much sense on its own. So Concept NACK from me too for now, unles
...

✅ fanquake closed a pull request: "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script"
(https://github.com/bitcoin/bitcoin/pull/29589)

💬 fanquake commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2672549367)
Closing for now.

💬 1440000bytes commented on pull request "Implement OP_CHECKTEMPLATEVERIFY":
(https://github.com/bitcoin/bitcoin/pull/29280#issuecomment-2672554342)
> Closing for now, given #29198 was closed.

https://github.com/bitcoin/bitcoin/pull/29198 was closed because some reviewers preferred separate pull request for each opcode as it would make it easier to review, test etc.

Related question in a meeting: https://bitcoincore.reviews/bitcoin-inquisition-45#l-112

Related comments: https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889817551

https://delvingbitcoin.org/t/lnhance-bips-and-implementation/376/12

🚀 fanquake merged a pull request: "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally"
(https://github.com/bitcoin/bitcoin/pull/31662)

💬 fanquake commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2672564901)
Opened https://github.com/google/oss-fuzz/pull/13081.

💬 fanquake commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2672598280)
@achow101 can you weigh in here? The changes seem to indicate that deprecation was targetted for `29.x`, for removal in `30.x`, but it doesn't seem like this is going into `29.x`.