💬 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?
(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
(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"
(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
...
(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?
(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.
(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.
(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.
(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
(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.
(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.
(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
(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
(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
...
(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
...
(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.
(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.
(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
(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.
(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.
(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.