💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944720234)
Changed. This is now `SockMan::Id` and in `net.h`: `using NodeId = SockMan::Id;`. In SockMan comments change the word "node" to "connection". This caused a lot of mechanical renames within this PR, but it looks better now, thanks!

`SockMan::GetNewNodeId()` renamed to
`SockMan::GetNewId()`

`SockMan::EventIOLoopCompletedForNode()` renamed to
`SockMan::EventIOLoopCompletedForOne()`

`SockMan::EventIOLoopCompletedForAllPeers()` renamed to
`SockMan::EventIOLoopCompletedForAll()`

`SockMa
...

💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2639841215)
Some historical context. The 2 hour future check used to be in `CheckBlockHeader`, but was moved in #8068 (Compact Blocks) in order to "prevent yet more includes and the crazy-looking GetAdjustedTime call from within the GetBlock stuff.": https://github.com/bitcoin/bitcoin/pull/8068/files#r66835690

> In order to avoid checking the merkle tree twice (which is incredibly expensive), I had to call CheckBlock, which required three #includes (including main.h) to blockencodings.cpp, despite them b
...

💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639854388)
> Why only depends though? If the issue is generally mixing compilers/flags, then using Clang + system libs on any (gcc-based) Linux system (potentially) has the same problem.

Yes this issue is not specific to depends, and could theoretically could happen if, for example an ubuntu library package was compiled with gcc and exposed a function with an empty struct parameter, and you tried to use the library with clang.

The point of having an ABI is to prevent issues like this and allow different
...

💬 arejula27 commented on pull request "test: expect that files may disappear from /proc/PID/fd/":
(https://github.com/bitcoin/bitcoin/pull/31614#issuecomment-2639920298)
ACK [`b2e9fdc`](https://github.com/bitcoin/bitcoin/pull/31614/commits/b2e9fdc00f5c40c241a37739f7b73b74c2181e39)

The issue is that the process of getting the list of files in the directory and then reading each one is not atomic. This can lead to race conditions. The proposed solution doesn’t fully fix the problem but avoids its consequences, preventing the test from failing when it shouldn’t.

Following @theuni’s idea, I tried exploring other solutions using `scandir`:
```pyhton
for item
...

💬 maflcko commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2639922568)
corecheck is back up and integrated into the DrahtBot comment, so is anything left to be done here?

💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639932130)
Submitted patch in https://github.com/capnproto/capnproto/pull/2235

💬 kevkevinpal commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2639939670)
ACK [39e15f6](https://github.com/bitcoin/bitcoin/pull/31805/commits/39e15f6ca4fd44cc459d80b7a1540c03923906dd)

I agree with @maflcko maybe instead of using pruned node we can refer to the node as "network limited node"

📝 glozow opened a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810)
Part of orphan resolution project, see #27463.

Definitions:
- **Announcement** is a unique pair (wtxid, nodeid). We can have multiple announcers for the same orphan since #31397.
- **Size** is the weight of an orphan. I'm calling it "size" and "bytes" because I think we can refine it in the future to be memusage or be otherwise more representative of the orphan's actual cost on our memory. However, I am open to naming changes.

This is part 1/2 of a project to also add limits on orphan si
...

💬 instagibbs commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2639947796)
I did some *very* light fuzzing against `CFeeRate` results to make sure the "gap" between the two values is never huge, would there be some value in adding coverage to confirm that results aren't wildly off from what we have today if we use it more widely?

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944816545)
Changed to `EventI2PStatus()` and instead of `bool` it now takes an enum with (currently) two possible values: `START_LISTENING` and `STOP_LISTENING`. Will be in next push.

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2639972637)
> would there be some value in adding coverage to confirm that results aren't wildly off from what we have today if we use it more widely?

Sure, happy to add a commit for tests with that.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944828644)
Fixed.

💬 instagibbs commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2640044728)
https://github.com/instagibbs/bitcoin/commit/ac2e55980bb4841a55b9396070af62cf3925e721

Bounding difference to "100 sats", seems to work ok to a 500BTC fee, feel free to take or leave it since these numbers are kind of made up.

concept ACK anyways

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944876407)
Yes, done.

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944879162)
I think libevent might actually use `45` for the `poll()` loop:

https://github.com/libevent/libevent/blob/112421c8fa4840acd73502f2ab6a674fc025de37/http-internal.h#L17-L20

What's the best way to determine this constant? Sockman-based HTTP certainly gets through the functional test suite a lot faster when this value is reduced.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944883037)
Yes, the I2P session contains a socket:

https://github.com/bitcoin/bitcoin/blob/d6c229d8bd4a6203a7255c140aa35c59fb20378b/src/i2p.h#L251-L260

💬 dergoegge commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944888846)
nit: Sanity checking is always better to do after a loop like this, as it doesn't increase the oracle power and just makes the test slower

💬 theuni commented on pull request "kernel: Avoid duplicating symbols in the kernel and downstream users":
(https://github.com/bitcoin/bitcoin/pull/31807#issuecomment-2640083370)
> I do believe that this case is covered by the [ODR](https://en.cppreference.com/w/cpp/language/definition):
>
> > There can be more than one definition in a program of each of the following: ... inline variable ...
> > If all these requirements are satisfied, _the program behaves as if there is only one definition in the entire program_. Otherwise, the program is ill-formed, no diagnostic required.
>
> (highlighted by me).

The ODR defines what happens "in a program". Symbol resolutio
...

📝 theuni converted_to_draft a pull request: "kernel: Avoid duplicating symbols in the kernel and downstream users"
(https://github.com/bitcoin/bitcoin/pull/31807)
The possibility of this happening came up in the discussion here: https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2635223309, and it turned that we already had a case of it in our code.

tl;dr: Certain symbols when defined in headers may end up existing in both the shared lib and the application using it, leading to subtle bugs. More info can be found [here](https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg503669.html).

The solution is generally to avoid defining static
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1944896725)
Done.