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

💬 w0xlt commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2813801840)
Concept ACK.

💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2813819778)
> concept ACK, cool idea and will review. Does this close #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?

It uses capnproto, so it may or may not achieve goals of #5029 depending on what they are. This PR does make it possible to call RPC methods over a unix socket, but in order to do that you need to use capnproto which is not available everywhere. For example there wouldn't
...

🤔 w0xlt reviewed a pull request: "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2776748288)
ACK https://github.com/bitcoin/bitcoin/pull/32023/commits/55b931934a34bab11446e8eed7bdaef92bb056de

nit: for clarity, the commit can be split into two, one for changes that do not affect behavior and the other for behavior change.

💬 instagibbs commented on pull request "BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/32247#issuecomment-2813871503)
I spent a bit of time writing tests (reading directly from the BIP) for feature_taproot.py: https://github.com/instagibbs/bitcoin/commit/ca28fa2e46104018b5eebf43e816e4e1470d4681

This should hopefully give some nice coverage of border conditions, including sigops budget and maybe inspired people to add more if they want to contribute in a positive way.

https://github.com/instagibbs/bitcoin/tree/2025-03-bip348-inq-28 this was an alternative implementation of the core logic, but I think it ma
...