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

💬 TheCharlatan commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813734342)
Updated 5ae18914805f0a78dde85cfe2a7876c4e52c0fe7 -> 7f5a35cf4b31f176ff9138d21f3ec677ae7d1bcf ([install_multiprocess_deps_1](https://github.com/TheCharlatan/bitcoin/tree/install_multiprocess_deps_1) -> [install_multiprocess_deps_2](https://github.com/TheCharlatan/bitcoin/tree/install_multiprocess_deps_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/install_multiprocess_deps_1..install_multiprocess_deps_2))

* Dropped the line adding capnproto to `doc/dependencies.md`. This can be
...

💬 TheCharlatan commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049461372)
Dropped it for now.

🤔 w0xlt reviewed a pull request: "torcontrol: Fix addrOnion outdated comment"
(https://github.com/bitcoin/bitcoin/pull/32282#pullrequestreview-2776615727)
ACK https://github.com/bitcoin/bitcoin/pull/32282/commits/bcaa23a2b7090c355f7b048ad8cae719c23dea43

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2813750592)
Rebased 1c299f70833188654b9cf3936d63d370856a408d -> e219f15976f50e0a703bc696b6445feb9950e65d ([chainman_flush_destructor_3](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_3) -> [chainman_flush_destructor_4](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_3..chainman_flush_destructor_4))

* Fixed conflict with #32154

👍 l0rinc approved a pull request: "feefrac: avoid integer overflow in temporary"
(https://github.com/bitcoin/bitcoin/pull/32300#pullrequestreview-2776583546)
ACK 6dd574444598240d2622e06e402548312123e5ef

If you think it's useful, we could hard code the behavior via a unit test based on the fuzz finding

💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049446540)
The new code:

```C++
quot + ((mod > 0) - (mod && round_down));
```
Compiles to
```ASM
test eax, eax
setne al
setg bl
movzx eax, al
and eax, ecx
*sub ebx, eax*
movsx rbx, ebx
*add rbx, rdx*
```
(`-` before `+`)

while the old one:
```C++
quot + (mod > 0) - (mod && round_down);
```
Compiles to
```ASM
test eax, eax
setg bl
*add rbx, rcx*
test eax, eax
setne al
movzx eax, al
and rax, rdx
*sub rbx, rax*
```
(`+` before
...

💬 sipa commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049481561)
I don't think the compiled result matters; both assembly listings are correct implementations for both versions of the source code. Integer overflow is not an issue on x86 assembly, it's an issue in C++ semantics that forbid it for signed integers.

Also, I think that just `quot - (mod && round_down) + (mod > 0)` would suffer from an identical but opposite problem: if `quot` is the minimum valid `int64_t`, and both `(mod && round_down)` and `(mod > 0)` are true, you'd get an underflow.

👋 l0rinc's pull request is ready for review: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296)

💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049493466)
Given that this tricked most of us, would it make sense to cover it with unit tests which demonstrate the behavior difference before/after?

💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2813787118)
concept ACK, cool idea and will review. Does this close https://github.com/bitcoin/bitcoin/issues/5029 (adding unix sockets to the bitcoind rpc api) or does it only apply to `bitcoin-node`? Or is IPC protocol different from RPC and must be defined in the client?

💬 sipa commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049496234)
Yeah I think that makes sense, so we don't have to rely on the fuzz corpus.