💬 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.
(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)
(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?
(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?
(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.
(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.
(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
...
(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.
(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
...
(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
...
💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049555771)
The simplest way I could find is to call it directly (not sure what the fuzzer did, don't have access):
```C++
BOOST_AUTO_TEST_CASE(feefrac_div_round_down_overflow)
{
#ifdef __SIZEOF_INT128__
constexpr auto divisor{2};
constexpr auto expected_result{std::numeric_limits<int64_t>::max()};
constexpr auto overflow_numerator{__int128{expected_result} * divisor + 1};
BOOST_CHECK_EQUAL(FeeFrac::Div(overflow_numerator, divisor, /*round_down=*/true), expected_result);
#else
...
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049555771)
The simplest way I could find is to call it directly (not sure what the fuzzer did, don't have access):
```C++
BOOST_AUTO_TEST_CASE(feefrac_div_round_down_overflow)
{
#ifdef __SIZEOF_INT128__
constexpr auto divisor{2};
constexpr auto expected_result{std::numeric_limits<int64_t>::max()};
constexpr auto overflow_numerator{__int128{expected_result} * divisor + 1};
BOOST_CHECK_EQUAL(FeeFrac::Div(overflow_numerator, divisor, /*round_down=*/true), expected_result);
#else
...
🤔 jonatack reviewed a pull request: "torcontrol: Fix addrOnion outdated comment"
(https://github.com/bitcoin/bitcoin/pull/32282#pullrequestreview-2776775370)
ACK bcaa23a2b7090c355f7b048ad8cae719c23dea43
(https://github.com/bitcoin/bitcoin/pull/32282#pullrequestreview-2776775370)
ACK bcaa23a2b7090c355f7b048ad8cae719c23dea43
💬 maflcko commented on pull request "refactor: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2813903196)
No strong opinion, but I don't see the point. This splits code that was nicely contained over three places, makes it verbose and harder to understand with no visible end-to-end benefit.
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2813903196)
No strong opinion, but I don't see the point. This splits code that was nicely contained over three places, makes it verbose and harder to understand with no visible end-to-end benefit.
💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049569201)
> (not sure what the fuzzer did, don't have access):
The issue I created includes the base64 encoded file that the fuzz engine on oss-fuzz produced
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049569201)
> (not sure what the fuzzer did, don't have access):
The issue I created includes the base64 encoded file that the fuzz engine on oss-fuzz produced
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2049571166)
1. The intent behind the code on master is to delete the nodes at specific points, otherwise `CNode::Release()` would do `if (--nRefCount == 0) delete this` (and had different logic around `m_nodes_disconnected`). You might be correct in that having been okay, nobody has been able to prove otherwise in this PR so far.
2. I agree that `shared_ptr`-usage in this PR not being exposed in the public interface of `CConnman` is a mitigating factor.
3. I bet you've checked all angles I would check
...
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2049571166)
1. The intent behind the code on master is to delete the nodes at specific points, otherwise `CNode::Release()` would do `if (--nRefCount == 0) delete this` (and had different logic around `m_nodes_disconnected`). You might be correct in that having been okay, nobody has been able to prove otherwise in this PR so far.
2. I agree that `shared_ptr`-usage in this PR not being exposed in the public interface of `CConnman` is a mitigating factor.
3. I bet you've checked all angles I would check
...
✅ l0rinc closed a pull request: "refactor: use original log string when no suspicious chars found"
(https://github.com/bitcoin/bitcoin/pull/31923)
(https://github.com/bitcoin/bitcoin/pull/31923)
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049582778)
This suggestion is not grammatically correct and doesn't make sense. The original is fine.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049582778)
This suggestion is not grammatically correct and doesn't make sense. The original is fine.
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049583367)
I don't think deduplicating this is really all that useful nor makes it easier to read.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049583367)
I don't think deduplicating this is really all that useful nor makes it easier to read.
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049584260)
I will leave this as is as there is no pubnonce object to tie the size to.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049584260)
I will leave this as is as there is no pubnonce object to tie the size to.
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588706)
Done
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588706)
Done
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588850)
Done
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049588850)
Done