🤔 musaHaruna reviewed a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2776493428)
tested ACK [77e553](https://github.com/bitcoin/bitcoin/commit/77e553ab6a0a98d065884a83490f28eec6df0e23)
Code reviewed and built successfully

💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2049393357)
Hmm, this is only an issue because `hash_or_height` expects a JSON object, not a string (and a text without quotes is no valid JSON object), so regular string args don't need this. I think the current naming is confusing, maybe renaming the function "`string_to_json_for_cli`" and calling `json.dumps(text)` inside instead of adding quotes (in case the cli is used) would be better to convey why this is done? Or is there a better solution?

💬 fanquake commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049407063)
In that case, adding this seems premature, as this document should represent the state of the release binaries.

💬 ryanofsky commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049410669)
It looks like this should be changed to "no" to be consistent with the rest of the table since qt, sqlite, etc also have "no". I would think of these all as runtime dependences, but IIUC the distinction this is making is that all of these are statically linked but freetype and fontconfig are dynamically linked.

💬 sipa commented on issue "Wallet should be able to store multiple transactions with same txid":
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813655649)
Maybe this is clearer:
a. If a transaction with the same txid is in the mempool, then the wallet should store just that one.
b. If no transaction with the same txid is in the mempool, then the wallet should store:
1. The version that was last in the mempool.
2. And, if different from (1), the version that has been confirmed.

If no version of the transaction has been confirmed, b.1 should be rebroadcast.

🤔 glozow reviewed a pull request: "Make TxGraph fuzz tests more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32191#pullrequestreview-2776524353)
utACK 2835216ec09cc2d86b091824376b15601e7c7b8a

💬 ryanofsky commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049413728)
Does also seem ok to drop this to avoid implying it is enabled in release binaries. Could add it in #31802

🚀 glozow merged a pull request: "Make TxGraph fuzz tests more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32191)

🤔 w0xlt reviewed a pull request: "doc: better document NetEventsInterface and the deletion of "CNode"s"
(https://github.com/bitcoin/bitcoin/pull/32278#pullrequestreview-2776535326)
ACK https://github.com/bitcoin/bitcoin/pull/32278/commits/e44e669378f2c63b09eda68797039b73047febff

Clear improvement in documentation.

📝 sipa opened a pull request: "feefrac: avoid integer overflow in temporary"
(https://github.com/bitcoin/bitcoin/pull/32300)
In `FeeFrac::Div(__int128 n, int32_t d, bool round_down)` in src/util/feefrac.h, the following line computes the result:

```c++
return quot + (mod > 0) - (mod && round_down);
```

The function can only be called under conditions where the result is in range, and thus doesn't involve any integer overflow. However, the intermediary result computed by just `quot + (mod > 0)` may still overflow if it's going to be correct by the `- (mod && round_down)` that follows.

Fix this by bal
...

💬 jonatack commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2813679527)
> My preference would be to either remove it or keep it short

Same, suggest removing the section completely. It's very handhold-y, and the few people who might need to read this information are probably the least likely to do so.

💬 sipa commented on issue "feefrac_mul_div: Integer-overflow in FeeFrac::Div":
(https://github.com/bitcoin/bitcoin/issues/32294#issuecomment-2813680575)
See #32300.

💬 jonatack commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r2049428352)
Prefer the current version of this line to the proposed change. It is clearer, shorter, and more grammatically correct.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049429573)
Also, another nit; could batch the db writes and fail if commit fails.

💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2813692117)
> the intermediary result computed by just `quot + (mod > 0)` may still overflow if it's going to be corrected by the `- (mod && round_down)` that follows.

How will it be corrected if rounding up?

💬 sipa commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2813695190)
> How will it be corrected if rounding up?

In that case, `quot + (mod > 0)` cannot overflow (if it did, the result isn't in range of `int64_t`, and the function would not be allowed to be called).

🤔 1440000bytes reviewed a pull request: "feefrac: avoid integer overflow in temporary"
(https://github.com/bitcoin/bitcoin/pull/32300#pullrequestreview-2776576116)
ACK https://github.com/bitcoin/bitcoin/pull/32300/commits/6dd574444598240d2622e06e402548312123e5ef

Code and motivation looks good. I couldn't find any instance of such changes to introduce bug.

💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2813710389)
review ACK 6dd574444598240d2622e06e402548312123e5ef

💬 EniRox001 commented on issue "Mistake in `feature_pruning.py` functional test?":
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2813731772)
I’ll investigate this further and determine the best way to implement a fix.

💬 jlest01 commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2813732282)
Concept ACK, as it can make communication between the CLI and the node simpler and configuration-free.