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

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

👍 laanwj approved a pull request: "feefrac: add support for evaluating at given size"
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2598982998)
Code review ACK f6aa28cf8fad6a3240498b500524bb380855b18e, mul/division code looks correct, testing and fuzzing coverage seems very good

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944917982)
Thanks, updated.

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2640132380)
Updated per review feedback (thanks!)

> It's good to clarify the description, because the term `NODE_NETWORK_LIMITED` has confused me before into assuming that a node that isn't limited doesn't use the flag. But instead it signals the minimum capability, and `NODE_NETWORK` is added when ready to signal full capability. Too late to rename them.

Well said -- same for me.

👍 instagibbs approved a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2598861794)
ACK 76af0b4d993c3a5a4a9c37f40d85b9996b38e7a0

Agree with https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944888846

and up for litigating the naming of size vs weight.

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944832866)
can we at least internally call it weight, even if externally we don't?

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944908395)
extra period?

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944903683)
nit: can assert number of announcers must be non-0 while we're here.

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944911351)
can use wtxid here

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944911842)
can use `wtxid` here

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944927518)
while we're here could also assert `m_total_announcements >= m_total_orphan_size`