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